[alsa-devel] [alsa-lib][RFC PATCH] ucm: reset config id of condition items
From: Libin Yang libin.yang@intel.com
UCMv2 supports "If" statement and will merge the same items with compound_merge(). If the items have the same id, it will fail to add the config items. And the id of the item in an array is automatically generated with the increased number. It is probably that some items to be merged have the same id. Let's add prefix in the id to avoid such situation.
For example:
If.seq1 { Condition { Type ControlExists Control "name='PGA1.0 1 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA1.0 1 Master Playback Volume' 50" ] } }
If.seq2 { Condition { Type ControlExists Control "name='PGA2.0 2 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA2.0 2 Master Playback Volume' 50" ] } }
If.seq3 { Condition { Type ControlExists Control "name='PGA3.0 3 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA3.0 3 Master Playback Volume' 50" ] } }
If seq1, seq2 and seq3 conditions are true, UCM will fail to initialize.
This patch rename the config id to avoid the same id conflict.
Signed-off-by: Libin Yang libin.yang@intel.com --- include/conf.h | 2 +- src/conf.c | 21 +++++++++++++++++++++ src/ucm/ucm_cond.c | 28 ++++++++++++++++++++++++---- 3 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/include/conf.h b/include/conf.h index 456b272..adb3d84 100644 --- a/include/conf.h +++ b/include/conf.h @@ -139,7 +139,7 @@ int snd_config_imake_safe_string(snd_config_t **config, const char *key, const c int snd_config_imake_pointer(snd_config_t **config, const char *key, const void *ptr);
snd_config_type_t snd_config_get_type(const snd_config_t *config); - +int snd_config_of_array(const snd_config_t *config); int snd_config_set_id(snd_config_t *config, const char *id); int snd_config_set_integer(snd_config_t *config, long value); int snd_config_set_integer64(snd_config_t *config, long long value); diff --git a/src/conf.c b/src/conf.c index 50d0403..43d565b 100644 --- a/src/conf.c +++ b/src/conf.c @@ -435,6 +435,8 @@ struct _snd_config { char *id; snd_config_type_t type; int refcount; /* default = 0 */ + /* member of an array */ + int of_array; union { long integer; long long integer64; @@ -1123,6 +1125,7 @@ static int _snd_config_make(snd_config_t **config, char **id, snd_config_type_t *id = NULL; } n->type = type; + n->of_array = 0; if (type == SND_CONFIG_TYPE_COMPOUND) INIT_LIST_HEAD(&n->u.compound.fields); *config = n; @@ -1316,6 +1319,8 @@ static int parse_array_def(snd_config_t *parent, input_t *input, int *idx, int s default: unget_char(c, input); err = parse_value(&n, parent, input, &id, skip); + /* this is a member of an array */ + n->of_array = 1; if (err < 0) goto __end; break; @@ -1784,6 +1789,22 @@ snd_config_type_t snd_config_get_type(const snd_config_t *config) }
/** + * \brief Returns the of_array of a configuration node. + * \param config Handle to the configuration node. + * \return of_array of the node + * + * The returned value indicates whether the node is a member of an array. + * + * \par Conforming to: + * LSB 3.2 + */ +int snd_config_of_array(const snd_config_t *config) +{ + assert(config); + return config->of_array; +} + +/** * \brief Returns the id of a configuration node. * \param[in] config Handle to the configuration node. * \param[out] id The function puts the pointer to the id string at the diff --git a/src/ucm/ucm_cond.c b/src/ucm/ucm_cond.c index 22b418d..725a69e 100644 --- a/src/ucm/ucm_cond.c +++ b/src/ucm/ucm_cond.c @@ -347,14 +347,15 @@ static void config_dump(snd_config_t *cfg) } #endif
-static int compound_merge(const char *id, +static int compound_merge(const char *id, const char *cname, snd_config_t *dst, snd_config_t *src, snd_config_t *before, snd_config_t *after) { snd_config_iterator_t i, next; snd_config_t *n, *_before = NULL, *_after = NULL; const char *s; - int err; + char s1[32]; + int err, cnt;
if (snd_config_get_type(src) != SND_CONFIG_TYPE_COMPOUND) { uc_error("compound type expected for If True/False block"); @@ -387,8 +388,22 @@ static int compound_merge(const char *id, return -EINVAL; }
+ cnt = 0; snd_config_for_each(i, next, src) { n = snd_config_iterator_entry(i); + /* + * If n is an array member, n->id is automatically generated. + * It is prossible that n->id is used by other array member, + * which will be merged with this one. So let's add prefix + * to the id to avoid the conflict. + */ + if (snd_config_of_array(n)) { + err = snd_config_get_id(n, &s); + if (err < 0) + return err; /* FIXME: this will never happen */ + snprintf(s1, sizeof(s1), "%s-%d-%s", cname, cnt++, s); + snd_config_set_id(n, (const char *)s1); + } err = snd_config_remove(n); if (err < 0) return err; @@ -422,7 +437,7 @@ int uc_mgr_evaluate_condition(snd_use_case_mgr_t *uc_mgr, { snd_config_iterator_t i, i2, next, next2; snd_config_t *a, *n, *n2, *parent2, *before, *after; - const char *id; + const char *id, *cname; int err;
if (uc_mgr->conf_format < 2) { @@ -437,6 +452,10 @@ int uc_mgr_evaluate_condition(snd_use_case_mgr_t *uc_mgr,
snd_config_for_each(i, next, cond) { n = snd_config_iterator_entry(i); + err = snd_config_get_id(n, &cname); + if (err < 0) + return err; /* FIXME: this will never happen */ + before = after = NULL; err = if_eval_one(uc_mgr, n, &a, &before, &after); if (err < 0) @@ -469,7 +488,8 @@ __add: err = snd_config_search(parent, id, &parent2); if (err == -ENOENT) goto __add; - err = compound_merge(id, parent2, n2, before, after); + err = compound_merge(id, cname, parent2, n2, + before, after); if (err < 0) return err; }
On 4/22/20 8:58 PM, libin.yang@linux.intel.com wrote:
From: Libin Yang libin.yang@intel.com
UCMv2 supports "If" statement and will merge the same items with compound_merge(). If the items have the same id, it will fail to add the config items. And the id of the item in an array is automatically generated with the increased number. It is probably that some items to be merged have the same id. Let's add prefix in the id to avoid such situation.
For example:
If.seq1 { Condition { Type ControlExists Control "name='PGA1.0 1 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA1.0 1 Master Playback Volume' 50" ] } }
If.seq2 { Condition { Type ControlExists Control "name='PGA2.0 2 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA2.0 2 Master Playback Volume' 50" ] } }
If.seq3 { Condition { Type ControlExists Control "name='PGA3.0 3 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA3.0 3 Master Playback Volume' 50" ] } }
If seq1, seq2 and seq3 conditions are true, UCM will fail to initialize.
This patch rename the config id to avoid the same id conflict.
The example confuses me completely, I checked three times and the seq1, seq2 and seq3 parts configure different controls.
Can you clarify what the conflict is and what id you were referring to?
Dne 23. 04. 20 v 15:06 Pierre-Louis Bossart napsal(a):
On 4/22/20 8:58 PM, libin.yang@linux.intel.com wrote:
From: Libin Yang libin.yang@intel.com
UCMv2 supports "If" statement and will merge the same items with compound_merge(). If the items have the same id, it will fail to add the config items. And the id of the item in an array is automatically generated with the increased number. It is probably that some items to be merged have the same id. Let's add prefix in the id to avoid such situation.
For example:
If.seq1 { Condition { Type ControlExists Control "name='PGA1.0 1 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA1.0 1 Master Playback Volume' 50" ] } }
If.seq2 { Condition { Type ControlExists Control "name='PGA2.0 2 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA2.0 2 Master Playback Volume' 50" ] } }
If.seq3 { Condition { Type ControlExists Control "name='PGA3.0 3 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA3.0 3 Master Playback Volume' 50" ] } }
If seq1, seq2 and seq3 conditions are true, UCM will fail to initialize.
This patch rename the config id to avoid the same id conflict.
The example confuses me completely, I checked three times and the seq1, seq2 and seq3 parts configure different controls.
Can you clarify what the conflict is and what id you were referring to?
The arrays in the ALSA configs are represented like:
User syntax:
name [ value0 value1 ]
Internal tree:
name.0 value0 name.1 value1
or
name { 0 value0 1 value1 }
(all three syntaxes are equal, the array just removes the indexes for the readability)
This patch tries to change name.0 to something like name.unique-0 or so which is not so much pretty.
You can just declare the new sequences like this to avoid clash:
EnableSequence.seq3.cset "name='PGA3.0 3 Master Playback Volume' 50"
Jaroslav
On 4/23/20 12:36 PM, Jaroslav Kysela wrote:
Dne 23. 04. 20 v 15:06 Pierre-Louis Bossart napsal(a):
On 4/22/20 8:58 PM, libin.yang@linux.intel.com wrote:
From: Libin Yang libin.yang@intel.com
UCMv2 supports "If" statement and will merge the same items with compound_merge(). If the items have the same id, it will fail to add the config items. And the id of the item in an array is automatically generated with the increased number. It is probably that some items to be merged have the same id. Let's add prefix in the id to avoid such situation.
For example:
If.seq1 { Condition { Type ControlExists Control "name='PGA1.0 1 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA1.0 1 Master Playback Volume' 50" ] } }
If.seq2 { Condition { Type ControlExists Control "name='PGA2.0 2 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA2.0 2 Master Playback Volume' 50" ] } }
If.seq3 { Condition { Type ControlExists Control "name='PGA3.0 3 Master Playback Volume'" } True { EnableSequence [ cset "name='PGA3.0 3 Master Playback Volume' 50" ] } }
If seq1, seq2 and seq3 conditions are true, UCM will fail to initialize.
This patch rename the config id to avoid the same id conflict.
The example confuses me completely, I checked three times and the seq1, seq2 and seq3 parts configure different controls.
Can you clarify what the conflict is and what id you were referring to?
The arrays in the ALSA configs are represented like:
User syntax:
name [ value0 value1 ]
Internal tree:
name.0 value0 name.1 value1
or
name { 0 value0 1 value1 }
(all three syntaxes are equal, the array just removes the indexes for the readability)
This patch tries to change name.0 to something like name.unique-0 or so which is not so much pretty.
You can just declare the new sequences like this to avoid clash:
EnableSequence.seq3.cset "name='PGA3.0 3 Master Playback Volume' 50"
Wow, I had no idea. Thanks for enlightening us :-)
Hi Jaroslav,
Can you clarify what the conflict is and what id you were referring to?
The arrays in the ALSA configs are represented like:
User syntax:
name [ value0 value1 ]
Internal tree:
name.0 value0 name.1 value1
or
name { 0 value0 1 value1 }
(all three syntaxes are equal, the array just removes the indexes for the readability)
This patch tries to change name.0 to something like name.unique-0 or so which is not so much pretty.
You can just declare the new sequences like this to avoid clash:
EnableSequence.seq3.cset "name='PGA3.0 3 Master Playback Volume' 50"
I tried your suggestion, the code like: If.seq1p { Condition { Type ControlExists Control "name='PGA1.0 1 Master Playback Volume'" } True { EnableSequence.seq1p.cset "name='PGA1.0 1 Master Playback Volume' 50" } }
It seems not work. When I run "alsaucm -c sof-soundwire open sof-soundwire" It shows: ALSA lib parser.c:470:(parse_sequence) error: string type is expected for sequence command ALSA lib parser.c:1257:(parse_verb) error: failed to parse verb enable sequence ALSA lib parser.c:1370:(parse_verb_file) error: HiFi.conf failed to parse verb ALSA lib main.c:962:(snd_use_case_mgr_open) error: failed to import sof-soundwire use case configuration -22 alsaucm: error failed to open sound card sof-soundwire: Invalid argument
My understanding is that "EnableSequence.seq1p.cset" will create a new snd_config_t "seq1p" between "EnableSequence" and "cset". This will cause executing EnableSequence failure.
Best Regards, Libin
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Dne 24. 04. 20 v 5:22 Yang, Libin napsal(a):
Hi Jaroslav,
Can you clarify what the conflict is and what id you were referring to?
The arrays in the ALSA configs are represented like:
User syntax:
name [ value0 value1 ]
Internal tree:
name.0 value0 name.1 value1
or
name { 0 value0 1 value1 }
(all three syntaxes are equal, the array just removes the indexes for the readability)
This patch tries to change name.0 to something like name.unique-0 or so which is not so much pretty.
You can just declare the new sequences like this to avoid clash:
EnableSequence.seq3.cset "name='PGA3.0 3 Master Playback Volume' 50"
I tried your suggestion, the code like: If.seq1p { Condition { Type ControlExists Control "name='PGA1.0 1 Master Playback Volume'" } True { EnableSequence.seq1p.cset "name='PGA1.0 1 Master Playback Volume' 50" } }
It seems not work. When I run "alsaucm -c sof-soundwire open sof-soundwire" It shows: ALSA lib parser.c:470:(parse_sequence) error: string type is expected for sequence command ALSA lib parser.c:1257:(parse_verb) error: failed to parse verb enable sequence ALSA lib parser.c:1370:(parse_verb_file) error: HiFi.conf failed to parse verb ALSA lib main.c:962:(snd_use_case_mgr_open) error: failed to import sof-soundwire use case configuration -22 alsaucm: error failed to open sound card sof-soundwire: Invalid argument
My understanding is that "EnableSequence.seq1p.cset" will create a new snd_config_t "seq1p" between "EnableSequence" and "cset". This will cause executing EnableSequence failure.
Oops. I was too quick in the idea. The configuration is the tree inside memory and [] is just a special case where the ids are replaced with the continuous integer range, thus:
EnableSequence [ cmd1 arg1 cms2 arg2 ]
is expanded to:
EnableSequence { 0 cmd1 1 arg1 2 cmd2 3 arg2 }
or
EnableSequence.0 cmd1 EnableSequence.1 arg1 EnableSequence.2 cmd2 EnableSequence.3 arg2
So if you like to add a new sequence which is outside the declared array, then you need to add this:
EnableSequence { cmdid3 cmd3 argid3 arg3 }
or (maybe more readable):
EnableSequence { cmdid3=cmd3 argid3=arg3 }
or:
EnableSequence.cmdid3 cmd3 EnableSequence.argid3 arg3
The ids names can be anything but they must be unique in the block (tree leaf).
The declaration order matters in this context (_arg_ must be right behind _cmd_ for the sequences). Note that the functions which works on top of the configuration tree (like the in-place evaluation - If blocks) are executed on top of this tree (which is parsed at first)! Keep it in the mind.
Jaroslav
Hi Jaroslav,
EnableSequence [ cmd1 arg1 cms2 arg2 ]
is expanded to:
EnableSequence { 0 cmd1 1 arg1 2 cmd2 3 arg2 }
or
EnableSequence.0 cmd1 EnableSequence.1 arg1 EnableSequence.2 cmd2 EnableSequence.3 arg2
So if you like to add a new sequence which is outside the declared array, then you need to add this:
EnableSequence { cmdid3 cmd3 argid3 arg3 }
or (maybe more readable):
EnableSequence { cmdid3=cmd3 argid3=arg3 }
or:
EnableSequence.cmdid3 cmd3 EnableSequence.argid3 arg3
The ids names can be anything but they must be unique in the block (tree leaf).
The declaration order matters in this context (_arg_ must be right behind _cmd_ for the sequences). Note that the functions which works on top of the configuration tree (like the in-place evaluation - If blocks) are executed on top of this tree (which is parsed at first)! Keep it in the mind.
Yes, it works with your new suggestion now.
Do you think this patch is necessary or not? With this patch we can use the style of: EnableSequence [ cmd1 arg1 cms2 arg2 ]
Regards, Libin
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Dne 01. 05. 20 v 15:44 Yang, Libin napsal(a):
Hi Jaroslav,
EnableSequence [ cmd1 arg1 cms2 arg2 ]
is expanded to:
EnableSequence { 0 cmd1 1 arg1 2 cmd2 3 arg2 }
or
EnableSequence.0 cmd1 EnableSequence.1 arg1 EnableSequence.2 cmd2 EnableSequence.3 arg2
So if you like to add a new sequence which is outside the declared array, then you need to add this:
EnableSequence { cmdid3 cmd3 argid3 arg3 }
or (maybe more readable):
EnableSequence { cmdid3=cmd3 argid3=arg3 }
or:
EnableSequence.cmdid3 cmd3 EnableSequence.argid3 arg3
The ids names can be anything but they must be unique in the block (tree leaf).
The declaration order matters in this context (_arg_ must be right behind _cmd_ for the sequences). Note that the functions which works on top of the configuration tree (like the in-place evaluation - If blocks) are executed on top of this tree (which is parsed at first)! Keep it in the mind.
Yes, it works with your new suggestion now.
Do you think this patch is necessary or not? With this patch we can use the style of: EnableSequence [ cmd1 arg1 cms2 arg2 ]
The patch changes the consistency in the id values for the array representation in memory (so you cannot address them). I tried to move this change to the right place (UCM conditions):
https://github.com/perexg/alsa-lib/commits/array-merge
Could you check this tree? The last commit implements the merge operation for the block from the condition tree to the parent tree. Also the "before" and "after" hints should be accepted, too.
Jaroslav
Hi Jaroslav,
Yes, it works with your new suggestion now.
Do you think this patch is necessary or not? With this patch we can use the style of: EnableSequence [ cmd1 arg1 cms2 arg2 ]
The patch changes the consistency in the id values for the array representation in memory (so you cannot address them). I tried to move this change to the right place (UCM conditions):
https://github.com/perexg/alsa-lib/commits/array-merge
Could you check this tree? The last commit implements the merge operation for the block from the condition tree to the parent tree. Also the "before" and "after" hints should be accepted, too.
I did the smoke test on your patch. It works.
Regards, Libin
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
participants (4)
-
Jaroslav Kysela -
libin.yang＠linux.intel.com -
Pierre-Louis Bossart -
Yang, Libin