[PATCH] ucm: change msleep to usleep

[PATCH] ucm: change msleep to usleep

[Lu Guanqun]

In file src/ucm/parser.c:
	if (strcmp(cmd, "usleep") == 0) {
string `usleep' is compared, however, in the comment and example conf file,
`msleep' is used, it's better to unify them all.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 src/ucm/parser.c              |    8 ++++----
 test/ucm/TestHDA/TestHDA.conf |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index 23b67bc..a56f1e6 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -792,7 +792,7 @@ static int parse_modifier_name(snd_use_case_mgr_t *uc_mgr,
  *	EnableSequence [
  *		cset "name='Master Playback Switch',index=2 0,0"
  *		cset "name='Master Playback Volume',index=2 25,25"
- *		msleep 50
+ *		usleep 50
  *		cset "name='Master Playback Switch',index=2 1,1"
  *		cset "name='Master Playback Volume',index=2 50,50"
  *	]
@@ -800,14 +800,14 @@ static int parse_modifier_name(snd_use_case_mgr_t *uc_mgr,
  *	DisableSequence [
  *		cset "name='Master Playback Switch',index=2 0,0"
  *		cset "name='Master Playback Volume',index=2 25,25"
- *		msleep 50
+ *		usleep 50
  *		cset "name='Master Playback Switch',index=2 1,1"
  *		cset "name='Master Playback Volume',index=2 50,50"
  *	]
  *
  *      # Optional transition verb
  *      TransitionSequence."ToCaseName" [
- *		msleep 1
+ *		usleep 1
  *      ]
  *
  *	# Optional TQ and ALSA PCMs
@@ -1106,7 +1106,7 @@ static int parse_controls(snd_use_case_mgr_t *uc_mgr, snd_config_t *cfg)
  *	cset "name='Master Mono Playback Volume',index=1 0"
  *	cset "name='PCM Switch',index=2 1,1"
  *      exec "some binary here"
- *      msleep 50
+ *      usleep 50
  *	........
  * ]
  *
diff --git a/test/ucm/TestHDA/TestHDA.conf b/test/ucm/TestHDA/TestHDA.conf
index 41dd74c..0a75e21 100644
--- a/test/ucm/TestHDA/TestHDA.conf
+++ b/test/ucm/TestHDA/TestHDA.conf
@@ -7,7 +7,7 @@ SectionUseCase."Case1" {
 
 SectionDefaults [
 	exec "my prg"
-	msleep 1
+	usleep 1
 	cdev "hw:0"
 	cset "name='PCM Playback Volume' 50%"
 ]

Re: [PATCH] ucm: change msleep to usleep

[Takashi Iwai]

At Fri, 19 Aug 2011 15:22:17 +0800,
Lu Guanqun wrote:
> 
> In file src/ucm/parser.c:
> 	if (strcmp(cmd, "usleep") == 0) {
> string `usleep' is compared, however, in the comment and example conf file,
> `msleep' is used, it's better to unify them all.

Don't we scale the value appropriately (although it's just a demo)?


Takashi

> 
> Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
> ---
>  src/ucm/parser.c              |    8 ++++----
>  test/ucm/TestHDA/TestHDA.conf |    2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index 23b67bc..a56f1e6 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -792,7 +792,7 @@ static int parse_modifier_name(snd_use_case_mgr_t *uc_mgr,
>   *	EnableSequence [
>   *		cset "name='Master Playback Switch',index=2 0,0"
>   *		cset "name='Master Playback Volume',index=2 25,25"
> - *		msleep 50
> + *		usleep 50
>   *		cset "name='Master Playback Switch',index=2 1,1"
>   *		cset "name='Master Playback Volume',index=2 50,50"
>   *	]
> @@ -800,14 +800,14 @@ static int parse_modifier_name(snd_use_case_mgr_t *uc_mgr,
>   *	DisableSequence [
>   *		cset "name='Master Playback Switch',index=2 0,0"
>   *		cset "name='Master Playback Volume',index=2 25,25"
> - *		msleep 50
> + *		usleep 50
>   *		cset "name='Master Playback Switch',index=2 1,1"
>   *		cset "name='Master Playback Volume',index=2 50,50"
>   *	]
>   *
>   *      # Optional transition verb
>   *      TransitionSequence."ToCaseName" [
> - *		msleep 1
> + *		usleep 1
>   *      ]
>   *
>   *	# Optional TQ and ALSA PCMs
> @@ -1106,7 +1106,7 @@ static int parse_controls(snd_use_case_mgr_t *uc_mgr, snd_config_t *cfg)
>   *	cset "name='Master Mono Playback Volume',index=1 0"
>   *	cset "name='PCM Switch',index=2 1,1"
>   *      exec "some binary here"
> - *      msleep 50
> + *      usleep 50
>   *	........
>   * ]
>   *
> diff --git a/test/ucm/TestHDA/TestHDA.conf b/test/ucm/TestHDA/TestHDA.conf
> index 41dd74c..0a75e21 100644
> --- a/test/ucm/TestHDA/TestHDA.conf
> +++ b/test/ucm/TestHDA/TestHDA.conf
> @@ -7,7 +7,7 @@ SectionUseCase."Case1" {
>  
>  SectionDefaults [
>  	exec "my prg"
> -	msleep 1
> +	usleep 1
>  	cdev "hw:0"
>  	cset "name='PCM Playback Volume' 50%"
>  ]
> 

Re: [PATCH] ucm: change msleep to usleep

[Lu Guanqun]

Hi Takashi,

On Fri, Aug 19, 2011 at 03:40:37PM +0800, Takashi Iwai wrote:
> At Fri, 19 Aug 2011 15:22:17 +0800,
> Lu Guanqun wrote:
> > 
> > In file src/ucm/parser.c:
> > 	if (strcmp(cmd, "usleep") == 0) {
> > string `usleep' is compared, however, in the comment and example conf file,
> > `msleep' is used, it's better to unify them all.
> 
> Don't we scale the value appropriately (although it's just a demo)?

No.

This is where the value gets set in src/ucm/parser.c:

	if (strcmp(cmd, "usleep") == 0) {
		curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
		err = snd_config_get_integer(n, &curr->data.sleep);
		if (err < 0) {
			uc_error("error: usleep requires integer!");
			return err;
		}
		continue;
	}       

This is where it gets used in src/ucm/main.c:

	case SEQUENCE_ELEMENT_TYPE_SLEEP:
		usleep(s->data.sleep);
		break;

I don't see it gets scaled somewhere...

I agree it's a demo, but as the documentation for UCM is very limited,
the wrong variable will get users into confusion.

> 
> 
> Takashi
> 
> > 
> > Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
> > ---
> >  src/ucm/parser.c              |    8 ++++----
> >  test/ucm/TestHDA/TestHDA.conf |    2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> > index 23b67bc..a56f1e6 100644
> > --- a/src/ucm/parser.c
> > +++ b/src/ucm/parser.c
> > @@ -792,7 +792,7 @@ static int parse_modifier_name(snd_use_case_mgr_t *uc_mgr,
> >   *	EnableSequence [
> >   *		cset "name='Master Playback Switch',index=2 0,0"
> >   *		cset "name='Master Playback Volume',index=2 25,25"
> > - *		msleep 50
> > + *		usleep 50
> >   *		cset "name='Master Playback Switch',index=2 1,1"
> >   *		cset "name='Master Playback Volume',index=2 50,50"
> >   *	]
> > @@ -800,14 +800,14 @@ static int parse_modifier_name(snd_use_case_mgr_t *uc_mgr,
> >   *	DisableSequence [
> >   *		cset "name='Master Playback Switch',index=2 0,0"
> >   *		cset "name='Master Playback Volume',index=2 25,25"
> > - *		msleep 50
> > + *		usleep 50
> >   *		cset "name='Master Playback Switch',index=2 1,1"
> >   *		cset "name='Master Playback Volume',index=2 50,50"
> >   *	]
> >   *
> >   *      # Optional transition verb
> >   *      TransitionSequence."ToCaseName" [
> > - *		msleep 1
> > + *		usleep 1
> >   *      ]
> >   *
> >   *	# Optional TQ and ALSA PCMs
> > @@ -1106,7 +1106,7 @@ static int parse_controls(snd_use_case_mgr_t *uc_mgr, snd_config_t *cfg)
> >   *	cset "name='Master Mono Playback Volume',index=1 0"
> >   *	cset "name='PCM Switch',index=2 1,1"
> >   *      exec "some binary here"
> > - *      msleep 50
> > + *      usleep 50
> >   *	........
> >   * ]
> >   *
> > diff --git a/test/ucm/TestHDA/TestHDA.conf b/test/ucm/TestHDA/TestHDA.conf
> > index 41dd74c..0a75e21 100644
> > --- a/test/ucm/TestHDA/TestHDA.conf
> > +++ b/test/ucm/TestHDA/TestHDA.conf
> > @@ -7,7 +7,7 @@ SectionUseCase."Case1" {
> >  
> >  SectionDefaults [
> >  	exec "my prg"
> > -	msleep 1
> > +	usleep 1
> >  	cdev "hw:0"
> >  	cset "name='PCM Playback Volume' 50%"
> >  ]
> > 

-- 
guanqun

Re: [PATCH] ucm: change msleep to usleep

[Takashi Iwai]

At Fri, 19 Aug 2011 16:13:06 +0800,
Lu Guanqun wrote:
> 
> Hi Takashi,
> 
> On Fri, Aug 19, 2011 at 03:40:37PM +0800, Takashi Iwai wrote:
> > At Fri, 19 Aug 2011 15:22:17 +0800,
> > Lu Guanqun wrote:
> > > 
> > > In file src/ucm/parser.c:
> > > 	if (strcmp(cmd, "usleep") == 0) {
> > > string `usleep' is compared, however, in the comment and example conf file,
> > > `msleep' is used, it's better to unify them all.
> > 
> > Don't we scale the value appropriately (although it's just a demo)?
> 
> No.

Then taking over the value as is doesn't look correct.


Takashi

Re: [PATCH] ucm: change msleep to usleep

[Liam Girdwood]

On 19/08/11 08:40, Takashi Iwai wrote:
> At Fri, 19 Aug 2011 15:22:17 +0800,
> Lu Guanqun wrote:
>>
>> In file src/ucm/parser.c:
>> 	if (strcmp(cmd, "usleep") == 0) {
>> string `usleep' is compared, however, in the comment and example conf file,
>> `msleep' is used, it's better to unify them all.
> 
> Don't we scale the value appropriately (although it's just a demo)?
> 

msleep is more useful here for pop prevention, but I suppose usleep could be useful too.

Could you redo to support both usleep and msleep ?

Thanks

Liam

[PATCH] ucm: add another sequence 'msleep'

[Lu Guanqun]

Thus, we have two sleep statements:
    msleep <milliseconds>
    usleep <microseconds>

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 src/ucm/parser.c    |   11 +++++++++++
 src/ucm/ucm_local.h |    2 +-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index 23b67bc..b93d832 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -316,6 +316,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "msleep") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
+			err = snd_config_get_integer(n, &curr->data.sleep);
+			if (err < 0) {
+				uc_error("error: msleep requires integer!");
+				return err;
+			}
+			curr->data.sleep *= 1000L;
+			continue;
+		}
+
 		if (strcmp(cmd, "exec") == 0) {
 			curr->type = SEQUENCE_ELEMENT_TYPE_EXEC;
 			err = parse_string(n, &curr->data.exec);
diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
index 0522bf5..aa4d493 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -57,7 +57,7 @@ struct sequence_element {
 	struct list_head list;
 	unsigned int type;
 	union {
-		long sleep; /* Sleep time in msecs if sleep element, else 0 */
+		long sleep; /* Sleep time in macroseconds if sleep element, else 0 */
 		char *cdev;
 		char *cset;
 		char *exec;

[PATCH v2] ucm: add another sequence 'msleep'

[Lu Guanqun]

Thus, we have two sleep statements:
    msleep <milliseconds>
    usleep <microseconds>

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 src/ucm/parser.c    |   11 +++++++++++
 src/ucm/ucm_local.h |    2 +-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index 23b67bc..b93d832 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -316,6 +316,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "msleep") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
+			err = snd_config_get_integer(n, &curr->data.sleep);
+			if (err < 0) {
+				uc_error("error: msleep requires integer!");
+				return err;
+			}
+			curr->data.sleep *= 1000L;
+			continue;
+		}
+
 		if (strcmp(cmd, "exec") == 0) {
 			curr->type = SEQUENCE_ELEMENT_TYPE_EXEC;
 			err = parse_string(n, &curr->data.exec);
diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
index 0522bf5..03d3ace 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -57,7 +57,7 @@ struct sequence_element {
 	struct list_head list;
 	unsigned int type;
 	union {
-		long sleep; /* Sleep time in msecs if sleep element, else 0 */
+		long sleep; /* Sleep time in microseconds if sleep element, else 0 */
 		char *cdev;
 		char *cset;
 		char *exec;

Re: [PATCH v2] ucm: add another sequence 'msleep'

[Liam Girdwood]

On 22/08/11 06:35, Lu Guanqun wrote:
> Thus, we have two sleep statements:
>     msleep <milliseconds>
>     usleep <microseconds>
> 
> Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
> ---
>  src/ucm/parser.c    |   11 +++++++++++
>  src/ucm/ucm_local.h |    2 +-
>  2 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index 23b67bc..b93d832 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -316,6 +316,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
>  			continue;
>  		}
>  
> +		if (strcmp(cmd, "msleep") == 0) {
> +			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
> +			err = snd_config_get_integer(n, &curr->data.sleep);
> +			if (err < 0) {
> +				uc_error("error: msleep requires integer!");
> +				return err;
> +			}
> +			curr->data.sleep *= 1000L;
> +			continue;
> +		}
> +
>  		if (strcmp(cmd, "exec") == 0) {
>  			curr->type = SEQUENCE_ELEMENT_TYPE_EXEC;
>  			err = parse_string(n, &curr->data.exec);
> diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
> index 0522bf5..03d3ace 100644
> --- a/src/ucm/ucm_local.h
> +++ b/src/ucm/ucm_local.h
> @@ -57,7 +57,7 @@ struct sequence_element {
>  	struct list_head list;
>  	unsigned int type;
>  	union {
> -		long sleep; /* Sleep time in msecs if sleep element, else 0 */
> +		long sleep; /* Sleep time in microseconds if sleep element, else 0 */
>  		char *cdev;
>  		char *cset;
>  		char *exec;
> 

Acked-by: Liam Girdwood <lrg@ti.com>

Re: [PATCH v2] ucm: add another sequence 'msleep'

[Takashi Iwai]

At Mon, 22 Aug 2011 11:39:28 +0100,
Liam Girdwood wrote:
> 
> On 22/08/11 06:35, Lu Guanqun wrote:
> > Thus, we have two sleep statements:
> >     msleep <milliseconds>
> >     usleep <microseconds>
> > 
> > Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
> > ---
> >  src/ucm/parser.c    |   11 +++++++++++
> >  src/ucm/ucm_local.h |    2 +-
> >  2 files changed, 12 insertions(+), 1 deletions(-)
> > 
> > diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> > index 23b67bc..b93d832 100644
> > --- a/src/ucm/parser.c
> > +++ b/src/ucm/parser.c
> > @@ -316,6 +316,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
> >  			continue;
> >  		}
> >  
> > +		if (strcmp(cmd, "msleep") == 0) {
> > +			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
> > +			err = snd_config_get_integer(n, &curr->data.sleep);
> > +			if (err < 0) {
> > +				uc_error("error: msleep requires integer!");
> > +				return err;
> > +			}
> > +			curr->data.sleep *= 1000L;
> > +			continue;
> > +		}
> > +
> >  		if (strcmp(cmd, "exec") == 0) {
> >  			curr->type = SEQUENCE_ELEMENT_TYPE_EXEC;
> >  			err = parse_string(n, &curr->data.exec);
> > diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
> > index 0522bf5..03d3ace 100644
> > --- a/src/ucm/ucm_local.h
> > +++ b/src/ucm/ucm_local.h
> > @@ -57,7 +57,7 @@ struct sequence_element {
> >  	struct list_head list;
> >  	unsigned int type;
> >  	union {
> > -		long sleep; /* Sleep time in msecs if sleep element, else 0 */
> > +		long sleep; /* Sleep time in microseconds if sleep element, else 0 */
> >  		char *cdev;
> >  		char *cset;
> >  		char *exec;
> > 
> 
> Acked-by: Liam Girdwood <lrg@ti.com>

Applied now.  Thanks.


Takashi