linux-usb-gadgets / libusbgx

C library encapsulating the Linux kernel USB gadget configfs userspace API functionality
GNU General Public License v2.0
216 stars 72 forks source link

Add support for UAC2 function #22

Closed johnkeeping closed 6 years ago

johnkeeping commented 6 years ago

Add support for the USB Audio Class 2 function that creates an ALSA audio device exposed as a USB gadget function.

This addresses issue #9.

kopasiak commented 6 years ago

After some testing I found some bugs in import()/export() but they were very simple to fix so I just did this.

There was also some inconsistency in set() routines because other functions pass there attr union directly not pointer to union. Thus I made uac2 compatible with this convention.

Please review and let me know if it works for you

johnkeeping commented 6 years ago

The import/export change looks good, I'm not using that functionality so didn't test it.

The union change relies on a GCC extension which is only available when compiling as C. I included that header in a C++ project and it doesn't compile; I think it would be better to change the other headers to pass the value as pointer-to-union rather than rely on GCC-specific behaviour (although it is also supported by Clang).

kopasiak commented 6 years ago

This can be easily fixed by adding a suitable constructor. Take a look at uac2.h now

johnkeeping commented 6 years ago

I realised that there's a simpler way to solve it by inserting braces around the value:

    return usbg_f_uac2_set_attr_val(af, USBG_F_UAC2_C_CHMASK,
                    (union usbg_f_uac2_attr_val){c_chmask});

C++ sees this as a uniform initializer and it's also valid C, in both cases initializing the first member of the union. I've pushed that change, what do you think?

kopasiak commented 6 years ago

To be honest I'm not a fan of this solution. It works only when the first field is of the same type (or at least compatible) as you initializer. So when you add for example a char *test field in the beginning of the union you get:

in C:

In file included from gadget-uac2.c:21:0: ../include/usbg/function/uac2.h: In function ‘usbg_f_uac2_set_c_chmask’: ../include/usbg/function/uac2.h:139:35: warning: initialization makes pointer from integer without a cast [-Wint-conversion] (union usbg_f_uac2_attr_val){c_chmask});

in C++:

/usr/local/include/usbg/function/uac2.h: In function ‘int usbg_f_uac2_set_c_chmask(usbg_f_uac2, int)’: /usr/local/include/usbg/function/uac2.h:139:43: error: invalid conversion from ‘int’ to ‘char’ [-fpermissive] (union usbg_f_uac2_attr_val){c_chmask});

and if you add it in at other position you get a warning about casting pointer to integer in function which sets the value of the new field. Not to mention that those errors depends on types which are used inside the union...

johnkeeping commented 6 years ago

Yeah, you're right. It works nicely for uac2 because everything's an int but if types are different then we need specific constructors. It's really annoying that C++ doesn't support designated initializers because that would give a really neat way of writing this.

I've updated the branch back to your "Take a look at uac2.h now" version.

kopasiak commented 6 years ago

After looking at this problem once again I realized that we can do a simple dereference to fix this. For example:

static inline int usbg_f_uac2_set_c_chmask(usbg_f_uac2 *af, int c_chmask)
{
    return usbg_f_uac2_set_attr_val(af, USBG_F_UAC2_C_CHMASK,
                    *((union usbg_f_uac2_attr_val *)&c_chmask));
}

so instead of writing those nasty constructors which compiles only in C++ we can just go through the pointer and dereference. What do you think?

johnkeeping commented 6 years ago

Nice! That's much better than the previous versions. I've pushed that implementation, so hopefully this is good to merge now.

kopasiak commented 6 years ago

Merged. Thank you for this contribution:)