Consideration to support remotely allocated USB interface storage

kammce commented 1 year ago

Related area

extending class driver

Hardware specification

N/A

Is your feature request related to a problem?

Currently, a project needs to select the number of class descriptors at compile time. In most use cases, this is great as it is well known the exact number of each type of USB device an MCU wants to be. But what about cases where this isn't known? In my case, I'd like to make prebuilt binaries of this library for multiple architectures. These binaries can be linked into any kind of application. The number of interfaces won't be known at the compile time of the binary.

Describe the solution you'd like

Utilize a negative number for the configuration macros like CFG_TUD_HID to denote "remotely allocated interface buffers." Like so:

//------------- CLASS -------------//
#define CFG_TUD_HID               -1
#define CFG_TUD_CDC               -1
#define CFG_TUD_MSC               -1
#define CFG_TUD_MIDI              -1
#define CFG_TUD_VENDOR            -1

In doing this, a new set of functions are added to the build. For example, HID will look like this:

if CFG_TUD_HID >= 0
CFG_TUD_MEM_SECTION tu_static hidd_interface_t _hidd_itf[CFG_TUD_HID];
tu_static uint8_t _hidd_itf_size = CFG_TUD_HID;
#else
tu_static hidd_interface_t* _hidd_itf = NULL;
tu_static uint8_t _hidd_itf_size = 0;
// Must be called before calling any other hid related APIs
void set_hid_interface_buffer(hidd_interface_t* hidd_itf, uint8_t hidd_size) {
    _hidd_itf = hidd_itf;
    _hidd_itf_size = hidd_size;
}
#endif

// ... other code ...

// Usage in helpers
static inline uint8_t get_index_by_itfnum(uint8_t itf_num)
{
    for (uint8_t i=0; i < _hidd_itf_size; i++ )
    {
        if ( itf_num == _hidd_itf[i].itf_num ) return i;
    }

    return 0xFF;
}

This idea will cost the program one additional byte (ignoring padding) for each interface to accomodate the new _hidd_itf_size variable. Macros could be used to eliminate the additional byte, but I think the cost is small enough to keep the code simple and with less macros.

The caller of set_hid_interface_buffer has the responsible for ensuring that the interface buffer is aligned correctly for their system's needs.

I have checked existing issues, dicussion and documentation

[X] I confirm I have checked existing issues, dicussion and documentation.

kammce commented 1 year ago

I will also like to add that I'd be interested in working on this, when I get free time. 😄

hathach commented 1 year ago

But what about cases where this isn't known? In my case, I'd like to make prebuilt binaries of this library for multiple architectures. These binaries can be linked into any kind of application. The number of interfaces won't be known at the compile time of the binary.

can you elaborate with examples, I don't see why we need to make any changes for this.

kammce commented 1 year ago

Sure. Please correct me if there is already a way to support remotely allocated storage for this project or if this isn't necessary to support multiple USB ports at runtime. To clarify my goal, I would like to build a prebuilt binary version of this library for the set of processors I plan to deploy to. A small example of those would be Cortex-M0+, Cortex-M3 and Cortex-M4 processors. Having these singular prebuilt binaries means that I can link them to any application that wants to support USB, without having to recompile TinyUSB each time for each project, making it portable to other systems using that processor.

The change would allow the application to, at runtime, decide how many USB ports it will be using for each class. For example, I could see an example where one MCU does everything on a single USB so all of the class configs can be set to 1 and thats all that is needed. In another example an MCU has two ports but only plans to use both of them for HID, but one of them is just for keyboard and the other one is for a mouse. Each application would statically allocate their own buffers and both can use the same exact prebuilt binary because they can both call set_hid_interface_buffer(). Whereas currently, for the two USB port example, we would have to build with CFG_TUD_HID = 2, resulting in different binaries.

Thank you for your time in reading my issue. Sorry if any of this is confusing.

kammce commented 1 year ago

I was looking through the code today and I think the cdc_dual_ports is a great example. In the tusb_config.h, CFG_TUD_CDC is set to 2, which sets the size of https://github.com/hathach/tinyusb/blob/92457ec99f1690b772ef9fa6b348256701c7fcf7/src/class/cdc/cdc_device.c#L86. Now if I wanted to make prebuilt binaries for Cortex M3 processors to support multiple applications, I would need to make a prebuilt binary for CFG_TUD_CDC 1, CFG_TUD_CDC 2, CFG_TUD_CDC 3, CFG_TUD_CDC 4, etc and also do each combination for the other configurations.

Now the current answer looks like, "Simply recompile this code each time for each application with their own tusb_config.h file." Which is a fine strategy, but this library could support both. I also believe that many of the other configurations can be set to a default that supports most applications with a single prebuilt binary.

hathach commented 1 year ago

CFG_TUD_HID is maximum HID interfaces tinyusb can support, it does not dictact number of HID in use. That is dictacted by configuration descriptors. What is your max HID for all cases, just define CFG_TUD_HID to that number. If it isn't terribly high number, it won't cost much of sram.

kammce commented 1 year ago

CFG_TUD_HID is maximum HID interfaces tinyusb can support, it does not dictact number of HID in use. That is dictacted by configuration descriptors. What is your max HID for all cases, just define CFG_TUD_HID to that number. If it isn't terribly high number, it won't cost much of sram.

I'm building a library that others will consume that uses this project, therefore I cannot know what circumstances a user of the library will find themselves in and thus I cannot define a hardcoded value for the class options. I used HID and CDC as examples. An application may need that additional SRAM. An application may need more than the maximum I set. I'm more looking for a way to make this library more configurable at runtime to eliminate the need to hard code a limit every possible CFG_TUD_* configuration.

hathach commented 1 year ago

oh, I can see your point now. This will require all the changes across all drivers, but there is a few gotchas

internal buffer also has other resitriction on endpoint alignment and address range depending on mcus.
The internal interface typedef struct won't be made public, therefore the set interface is probably just supply buffer + size and get the number of inteface it got in return. Maybe we can have an get size function. But that would be needed for every class driver.
The set_buffer/interface() must be done before driver is initialized, i.e before tusb is inited. We can also use an weak callback e.g tud_hid_user_buffer_cb() which is only required/called when CFGTUD* = -1. This way, application don't have to worry which place it should be called.

This is kind of neat, but rarely required, since most project compile tinyusb with sources. If you need this feature, you will probably need to implement it yourself. Though feel free to make it as (draft) PR, I will try to help/suggest whenever I could.

kammce commented 1 year ago

Awesome! With your blessing, I'll start work on this. Thank you for considering this feature.

hathach commented 1 year ago

Awesome! With your blessing, I'll start work on this. Thank you for considering this feature.

please start with HID only as draft PR, before applies to other drivers, just to make sure you don't have to change too many places. Also if possible be sure to test it on your lib with actual hardware as well.

kammce commented 2 weeks ago

I'm actually going to close this issue. My team decided to go another route. Thank you for your time last year. Good luck with the project!

hathach / tinyusb