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%"

]

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;

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