[alsa-lib,RFC] ucm: reset config id of condition items

Commit Message

[e-mail] April 23, 2020, 1:58 a.m. UTC

From: Libin Yang <[e-mail]>

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 <[e-mail]>
---
 include/conf.h     |  2 +-
 src/conf.c         | 21 +++++++++++++++++++++
 src/ucm/ucm_cond.c | 28 ++++++++++++++++++++++++----
 3 files changed, 46 insertions(+), 5 deletions(-)

Comments

Pierre-Louis Bossart April 23, 2020, 1:06 p.m. UTC | #1

On 4/22/20 8:58 PM, [e-mail] wrote:
> From: Libin Yang <[e-mail]>
> 
> 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?

Jaroslav Kysela April 23, 2020, 5:36 p.m. UTC | #2

Dne 23. 04. 20 v 15:06 Pierre-Louis Bossart napsal(a):
> 
> 
> On 4/22/20 8:58 PM, [e-mail] wrote:
>> From: Libin Yang <[e-mail]>
>>
>> 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

Pierre-Louis Bossart April 23, 2020, 6:27 p.m. UTC | #3

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, [e-mail] wrote:
>>> From: Libin Yang <[e-mail]>
>>>
>>> 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 :-)

Yang, Libin April 24, 2020, 3:22 a.m. UTC | #4

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 <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Jaroslav Kysela April 29, 2020, 1:47 p.m. UTC | #5

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

Yang, Libin May 1, 2020, 1:44 p.m. UTC | #6

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 <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Jaroslav Kysela May 8, 2020, 10:47 a.m. UTC | #7

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

Yang, Libin May 9, 2020, 8:43 a.m. UTC | #8

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 <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Patch

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;
 			}