| Message ID | 1472516505-8350-1-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 30 Aug 2016 02:21:45 +0200, Takashi Sakamoto wrote: > > I misunderstand TLV packet structure (struct snd_ctl_tlv) nests in its > payload. But actually, the payload stores different type of data. > > This commit corrects a comment in a test program of user control element > set including my misunderstanding. > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > --- > test/user-ctl-element-set.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/test/user-ctl-element-set.c b/test/user-ctl-element-set.c > index 9b9dc59..f6f050a 100644 > --- a/test/user-ctl-element-set.c > +++ b/test/user-ctl-element-set.c > @@ -418,11 +418,11 @@ static int check_tlv(struct elem_set_trial *trial) > int err; > > /* > - * See a layout of 'struct snd_ctl_tlv'. I don't know the reason to > - * construct this buffer with the same layout. It should be abstracted > - * inner userspace library... > + * When transferring threshold level information via TLV feature of > + * ALSA control interface, the first two fields of packet payload > + * consist of data type and data length. > */ > - orig[0] = snd_ctl_elem_id_get_numid(trial->id); > + orig[0] = SNDRV_CTL_TLVT_CONTAINER; > orig[1] = 6 * sizeof(orig[0]); > orig[2] = 'a'; > orig[3] = 'b'; The container TLV type expects other TLVs as its content. So the above looks buggy to me... Takashi
On Aug 30 2016 14:32, Takashi Iwai wrote: > On Tue, 30 Aug 2016 02:21:45 +0200, > Takashi Sakamoto wrote: >> >> I misunderstand TLV packet structure (struct snd_ctl_tlv) nests in its >> payload. But actually, the payload stores different type of data. >> >> This commit corrects a comment in a test program of user control element >> set including my misunderstanding. >> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> >> --- >> test/user-ctl-element-set.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/test/user-ctl-element-set.c b/test/user-ctl-element-set.c >> index 9b9dc59..f6f050a 100644 >> --- a/test/user-ctl-element-set.c >> +++ b/test/user-ctl-element-set.c >> @@ -418,11 +418,11 @@ static int check_tlv(struct elem_set_trial *trial) >> int err; >> >> /* >> - * See a layout of 'struct snd_ctl_tlv'. I don't know the reason to >> - * construct this buffer with the same layout. It should be abstracted >> - * inner userspace library... >> + * When transferring threshold level information via TLV feature of >> + * ALSA control interface, the first two fields of packet payload >> + * consist of data type and data length. >> */ >> - orig[0] = snd_ctl_elem_id_get_numid(trial->id); >> + orig[0] = SNDRV_CTL_TLVT_CONTAINER; >> orig[1] = 6 * sizeof(orig[0]); >> orig[2] = 'a'; >> orig[3] = 'b'; > > The container TLV type expects other TLVs as its content. So the > above looks buggy to me... This is a test program just to serve to myself. So I hope this degree of roughness is allowed... Or could you please construct valid TLV array here? I don't exactly know the way to do it, because I've never touched better documentation about whole design of TLV payload data. Regards Takashi Sakamoto
On Tue, 30 Aug 2016 08:33:24 +0200, Takashi Sakamoto wrote: > > On Aug 30 2016 14:32, Takashi Iwai wrote: > > On Tue, 30 Aug 2016 02:21:45 +0200, > > Takashi Sakamoto wrote: > >> > >> I misunderstand TLV packet structure (struct snd_ctl_tlv) nests in its > >> payload. But actually, the payload stores different type of data. > >> > >> This commit corrects a comment in a test program of user control element > >> set including my misunderstanding. > >> > >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > >> --- > >> test/user-ctl-element-set.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/test/user-ctl-element-set.c b/test/user-ctl-element-set.c > >> index 9b9dc59..f6f050a 100644 > >> --- a/test/user-ctl-element-set.c > >> +++ b/test/user-ctl-element-set.c > >> @@ -418,11 +418,11 @@ static int check_tlv(struct elem_set_trial *trial) > >> int err; > >> > >> /* > >> - * See a layout of 'struct snd_ctl_tlv'. I don't know the reason to > >> - * construct this buffer with the same layout. It should be abstracted > >> - * inner userspace library... > >> + * When transferring threshold level information via TLV feature of > >> + * ALSA control interface, the first two fields of packet payload > >> + * consist of data type and data length. > >> */ > >> - orig[0] = snd_ctl_elem_id_get_numid(trial->id); > >> + orig[0] = SNDRV_CTL_TLVT_CONTAINER; > >> orig[1] = 6 * sizeof(orig[0]); > >> orig[2] = 'a'; > >> orig[3] = 'b'; > > > > The container TLV type expects other TLVs as its content. So the > > above looks buggy to me... > > This is a test program just to serve to myself. So I hope this degree > of roughness is allowed... But it's a clearly wrong choice. Even a random number would be better, as the chance to choose a wrong one is lower than 100% :) > Or could you please construct valid TLV array here? I don't exactly > know the way to do it, because I've never touched better documentation > about whole design of TLV payload data. Just pick anything else than CONTAINER type. As said, CONTAINER type follows other TLVs in its value data, e.g. in the example below, the container contains two TLV data A and B. TLV_CONTAINER length (A+B) TLV_XXX_A length_a value_a... TLV_XXX_B length_b value_b... Takashi
Takashi Sakamoto wrote: > On Aug 30 2016 14:32, Takashi Iwai wrote: >> Takashi Sakamoto wrote: >>> - orig[0] = snd_ctl_elem_id_get_numid(trial->id); >>> + orig[0] = SNDRV_CTL_TLVT_CONTAINER; >>> orig[1] = 6 * sizeof(orig[0]); >>> orig[2] = 'a'; >>> orig[3] = 'b'; >> >> The container TLV type expects other TLVs as its content. So the >> above looks buggy to me... > > This is a test program just to serve to myself. So I hope this degree of roughness is allowed... > > Or could you please construct valid TLV array here? In the kernel, you could use the macros from include/sound/tlv.h. Either copy them into your test program, and use them directly: /* 4 words: */ DECLARE_TLV_DB_MINMAX(orig, -60, +60); /* container with only one subitem; 8 words: */ DECLARE_TLV_CONTAINER(orig_container, TLV_ITEM(SNDRV_CTL_TLVT_CHMAP_FIXED, SNDRV_CHMAP_FL, SNDRV_CHMAP_FR, SNDRV_CHMAP_FC, SNDRV_CHMAP_LFE) ); or create the values by hand: orig[0] = SNDRV_CTL_TLVT_DB_MINMAX; orig[1] = 2 * sizeof(*orig); orig[2] = -60; orig[3] = +60; orig_container[0] = SNDRV_CTL_TLVT_CONTAINER; orig_container[1] = 6 * sizeof(*orig_container); orig_container[2] = SNDRV_CTL_TLVT_CHMAP_FIXED; orig_container[3] = 4 * sizeof(*orig_container); orig_container[4] = SNDRV_CHMAP_FL orig_container[5] = SNDRV_CHMAP_FR; orig_container[6] = SNDRV_CHMAP_FC; orig_container[7] = SNDRV_CHMAP_LFE; Regards, Clemens
On Aug 30 2016 16:24, Clemens Ladisch wrote: > In the kernel, you could use the macros from include/sound/tlv.h. > > Either copy them into your test program, and use them directly: > > /* 4 words: */ > DECLARE_TLV_DB_MINMAX(orig, -60, +60); > /* container with only one subitem; 8 words: */ > DECLARE_TLV_CONTAINER(orig_container, > TLV_ITEM(SNDRV_CTL_TLVT_CHMAP_FIXED, > SNDRV_CHMAP_FL, SNDRV_CHMAP_FR, SNDRV_CHMAP_FC, SNDRV_CHMAP_LFE) > ); > > or create the values by hand: > > orig[0] = SNDRV_CTL_TLVT_DB_MINMAX; > orig[1] = 2 * sizeof(*orig); > orig[2] = -60; > orig[3] = +60; > > orig_container[0] = SNDRV_CTL_TLVT_CONTAINER; > orig_container[1] = 6 * sizeof(*orig_container); > orig_container[2] = SNDRV_CTL_TLVT_CHMAP_FIXED; > orig_container[3] = 4 * sizeof(*orig_container); > orig_container[4] = SNDRV_CHMAP_FL > orig_container[5] = SNDRV_CHMAP_FR; > orig_container[6] = SNDRV_CHMAP_FC; > orig_container[7] = SNDRV_CHMAP_LFE; Mmm. For user-defined control element sets, these macros should be in UAPI. SNDRV_CTL_IOCTL_ELEM_ADD was really long abandoned... Thanks Takashi Sakamoto
On Fri, 02 Sep 2016 16:11:30 +0200, Takashi Sakamoto wrote: > > On Aug 30 2016 16:24, Clemens Ladisch wrote: > > In the kernel, you could use the macros from include/sound/tlv.h. > > > > Either copy them into your test program, and use them directly: > > > > /* 4 words: */ > > DECLARE_TLV_DB_MINMAX(orig, -60, +60); > > /* container with only one subitem; 8 words: */ > > DECLARE_TLV_CONTAINER(orig_container, > > TLV_ITEM(SNDRV_CTL_TLVT_CHMAP_FIXED, > > SNDRV_CHMAP_FL, SNDRV_CHMAP_FR, SNDRV_CHMAP_FC, SNDRV_CHMAP_LFE) > > ); > > > > or create the values by hand: > > > > orig[0] = SNDRV_CTL_TLVT_DB_MINMAX; > > orig[1] = 2 * sizeof(*orig); > > orig[2] = -60; > > orig[3] = +60; > > > > orig_container[0] = SNDRV_CTL_TLVT_CONTAINER; > > orig_container[1] = 6 * sizeof(*orig_container); > > orig_container[2] = SNDRV_CTL_TLVT_CHMAP_FIXED; > > orig_container[3] = 4 * sizeof(*orig_container); > > orig_container[4] = SNDRV_CHMAP_FL > > orig_container[5] = SNDRV_CHMAP_FR; > > orig_container[6] = SNDRV_CHMAP_FC; > > orig_container[7] = SNDRV_CHMAP_LFE; > > Mmm. For user-defined control element sets, these macros should be in UAPI. The macros are for static data, and for user-space, it's better to have functions filling the TLV, IMO. (Also these macros were recently already put in uapi/include/sound.) Takashi
On 2016年09月03日 00:58, Takashi Iwai wrote: > On Fri, 02 Sep 2016 16:11:30 +0200, > Takashi Sakamoto wrote: >> >> On Aug 30 2016 16:24, Clemens Ladisch wrote: >>> In the kernel, you could use the macros from include/sound/tlv.h. >>> >>> Either copy them into your test program, and use them directly: >>> >>> /* 4 words: */ >>> DECLARE_TLV_DB_MINMAX(orig, -60, +60); >>> /* container with only one subitem; 8 words: */ >>> DECLARE_TLV_CONTAINER(orig_container, >>> TLV_ITEM(SNDRV_CTL_TLVT_CHMAP_FIXED, >>> SNDRV_CHMAP_FL, SNDRV_CHMAP_FR, SNDRV_CHMAP_FC, SNDRV_CHMAP_LFE) >>> ); >>> >>> or create the values by hand: >>> >>> orig[0] = SNDRV_CTL_TLVT_DB_MINMAX; >>> orig[1] = 2 * sizeof(*orig); >>> orig[2] = -60; >>> orig[3] = +60; >>> >>> orig_container[0] = SNDRV_CTL_TLVT_CONTAINER; >>> orig_container[1] = 6 * sizeof(*orig_container); >>> orig_container[2] = SNDRV_CTL_TLVT_CHMAP_FIXED; >>> orig_container[3] = 4 * sizeof(*orig_container); >>> orig_container[4] = SNDRV_CHMAP_FL >>> orig_container[5] = SNDRV_CHMAP_FR; >>> orig_container[6] = SNDRV_CHMAP_FC; >>> orig_container[7] = SNDRV_CHMAP_LFE; >> >> Mmm. For user-defined control element sets, these macros should be in UAPI. > > (Also these macros were recently already put in uapi/include/sound.) Ah, sorry, I addressed to macros such as DECLARE_TLV_DB_MINMAX, because the array is preprocessed the macro. > The macros are for static data, and for user-space, it's better to > have functions filling the TLV, IMO. In a point of maintenance, it's preferable that a single file gives the feature. No need to have several files for exactly the same feature. Of course, we can move the macros to UAPI and give inline function APIs for applications to expand with the macros. But, this is a bit dull approach to me. Regards Takashi Sakamoto
diff --git a/test/user-ctl-element-set.c b/test/user-ctl-element-set.c index 9b9dc59..f6f050a 100644 --- a/test/user-ctl-element-set.c +++ b/test/user-ctl-element-set.c @@ -418,11 +418,11 @@ static int check_tlv(struct elem_set_trial *trial) int err; /* - * See a layout of 'struct snd_ctl_tlv'. I don't know the reason to - * construct this buffer with the same layout. It should be abstracted - * inner userspace library... + * When transferring threshold level information via TLV feature of + * ALSA control interface, the first two fields of packet payload + * consist of data type and data length. */ - orig[0] = snd_ctl_elem_id_get_numid(trial->id); + orig[0] = SNDRV_CTL_TLVT_CONTAINER; orig[1] = 6 * sizeof(orig[0]); orig[2] = 'a'; orig[3] = 'b';
I misunderstand TLV packet structure (struct snd_ctl_tlv) nests in its payload. But actually, the payload stores different type of data. This commit corrects a comment in a test program of user control element set including my misunderstanding. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- test/user-ctl-element-set.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)