[PluggableHID] 2nd round of API improvement (WIP)

[cmaglie]

This is a follow up to #3840, an attempt to make the PluggableHID API more flexible before the release.

@NicoHood, please, try to find 10 minutes to look at the commits one at a time. From what I can see they "match" your latest attempts very closely.

This is implemented on AVR only for now (SAM and SAMD will follow).

[NicoHood]

You were right. Your commits all look good to me. I will compare with my version and look if this can be used like this. But its looking very good to me. Sorry that i missed that first.

Have you tested this with real hardware?

afk for some time, excuse me.

[cmaglie]

I haven't tested yet (all those changes should be "equivalent" but... only testing will tell).

afk for some time, excuse me.

np, I'm going away too, will continue tomorrow.

[NicoHood]

• The code works (tested KeyboardSerial example)
• The u8 should be replaced later on all places, also the typedefs
• There are some compiler warnings that needs to be solved.
• I will try yo fork your repo and post some fixes over that now. The API looks better to me now.
• Why dont we make the 3 functions virtual. I think the virtual overhead is then the same but the same function can be used more flexible?
• numEndpoints should be const and declared via constructor. Without its more flexible but can contain more errors if they are not initialized.
• why is interface unsigned and endpoint signed?
• why do we need those wrapper function for the IF and EP? Doesnt inheriting them via protected work good enough?
• I might be wrong with some comments. Its late and I'll give up now.
• Other stuff might be edited later on.

[cmaglie]

The code works (tested KeyboardSerial example)

great, thanks for testing!

The u8 should be replaced later on all places, also the typedefs

I'm going to apply this change.

There are some compiler warnings that needs to be solved.

I'll leave this one as the last task

Why dont we make the 3 functions virtual. I think the virtual overhead is then the same but the same function can be used more flexible?

Yes

numEndpoints should be const and declared via constructor. Without its more flexible but can contain more errors if they are not initialized.

Yes

why is interface unsigned and endpoint signed?

because I've just moved code around and they was defined like that. I'll check if everything can be changed to unsigned.

why do we need those wrapper function for the IF and EP? Doesnt inheriting them via protected work good enough?

Well the reasoning is that IF and EP are assigned after the module has been plugged, they are not structure-like that are assigned directly from the user. Moreover if we make the other fields constant and initialized through constructor we are going to remove this difference. Let's see how it looks I'm going to add more commits in the next few hours.

[NicoHood]

I forgot: You can get rid of the static functions and variables now. Since the Class is now a singleton there is no need for static functions and variables.

I tried to fix that in a local branch, but before editing twice (you and me) I suggest you just add this.
Please initialize the variables via a proper constructor
Class::Class : var(defaultvalue), nextval(default) etc. You know the drill ;D

So I wait for further commits from you. Let me know on any update.
This is getting better and better now :)

[cmaglie]

Ok, just pushed another 4 commits, this should solve 90% of the problems so far.

Still to be done:

• check unsigned vs signed
• fix compiler warnings
• rename fields/class to a more appropriate/comfortable name

[NicoHood]

Those static functions can be removed as well I think.

[NicoHood]

you could make those int8_t and use the interface as a check if the modul exists. Maybe this check can be done in the pluggable class so we have less code duplication/error with bad library coding. (set to -1 if all modules are used)

And we also need to change the modules used limit to the endpoint number, not the modules itself. On a 16u2 there is only one more free endpoint. If your interface (midi usb I suspect) uses 2 endpoints it is not able to work correctly. Thatswhy the check should be done via endpoint count and set the interface to -1 if it doesnt work correct.

If we implement the -1 inside the pluggable class we can remove the bool return value of plug() otherwise we could use this to set the interface to -1.

The 16u2 needs to be patched with the modul/ep limit anyways, but later.

[cmaglie]

Pushed other 2 commits.

And we also need to change the modules used limit to the endpoint number, not the modules itself. On a 16u2 there is only one more free endpoint. If your interface (midi usb I suspect) uses 2 endpoints it is not able to work correctly. Thatswhy the check should be done via endpoint count and set the interface to -1 if it doesnt work correct.

this means that the check:

bool PluggableUSB_::plug(PUSBListNode *node)
{
    if (modulesCount >= MAX_MODULES) {
        return false;
    }

should really be:

bool PluggableUSB_::plug(PUSBListNode *node)
{
    if ((lastEp + node->numEndpoints) >= MAX_MODULES) {
        return false;
    }

and MAX_MODULES should be renamed to MAX_EP, right? In this case I think that we can completely drop modulesCount

[NicoHood]

Yes. We can get rid of the modules count variable and overhead with this check.

You need to define the value then with 6 (since 3 are used by the CDC which is initialized to lastep already.

For the u2 Series please add 4 enpoints.

You also need to edit this file:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/USBCore.cpp#L320-L323

Or (maybe even better) you can use sizeof(_initEndpoints) (minus one for the control ep) and define the MAX_EP somewhere in USBCore.h

To initialize _initEndpoints to zero i'd use something similar to the PinInterrupt PR (if you remember).

Please use #ifndef MAX_EP and then define the values for 32u4 and u2 series.

Edit:
class PUSBListNode has two public sections. Merge them together please.

last 2 commits are looking goood. Getting more and more excited now :)

Edit2:
This should be moved to the .h file, or with the new MAX_EP definition moved to the USBxxx.h file and then no redefinition is needed.
extern uint8_t _initEndpoints[];

Does PUSBListNode *next = NULL; needs to be public?

[cmaglie]

Wait wait wait, the _initEndpoints[] array has 7 elements not 6, so MAX_EP should be 7.

From USBCore:

u8 _initEndpoints[] =
{
        0,

        EP_TYPE_INTERRUPT_IN,           // CDC_ENDPOINT_ACM
        EP_TYPE_BULK_OUT,                       // CDC_ENDPOINT_OUT
        EP_TYPE_BULK_IN,                        // CDC_ENDPOINT_IN

#ifdef PLUGGABLE_USB_ENABLED
        //allocate 3 endpoints and remove const so they can be changed by the user
        0,
        0,
        0,
#endif
};

in PluggableUSB lastEp is set to CDC_FIRST_ENDPOINT + CDC_ENPOINT_COUNTwhere

#define CDC_FIRST_ENDPOINT  1
#define CDC_ENPOINT_COUNT   3

so lastEp is 4. With 3 extra endpoint it goes to 7.
BTW I guess that I'll follow your suggestion to use sizeof.

[NicoHood]

@matthijskooijman HID will be initialzed multiple times. At least if I need to do a fork. This is needed to create multiple gamepads on multiple endpoints (and other cool stuff). I am just wondering about what variable you are talking. Whatever variable this is, if the first adding works (enough endpoints) and the 2nd doesnt, with a shared variable this can cause problems.

Pluggable USB probably does not need to be a singleton/class at all. So we could maybe change this back. As you said the size should be compared.

@cmaglie good catch with the 1 offset. sizeof is still the best solution i guess. Please conside to commend the first entry of _initEndpoints with // Control Endpoint

[matthijskooijman]

I looked casually through all commits, looking good to me!

To initialize _initEndpoints to zero i'd use something similar to the PinInterrupt PR (if you remember).

No need - global variables are zeroed by default (the attachInterrupt stuff need to initialize to &empty or something, not 0).

Yes. We can get rid of the modules count variable and overhead with this check.

I was going to say that you still need the count to interate the modules, but you can of course just loop until next is NULL.

in PluggableUSB lastEp is set to CDC_FIRST_ENDPOINT + CDC_ENPOINT_COUNT

I haven't looked at the code for this, but perhaps it would be worth considering to convert the CDC class to a pluggable class as well? This might simplify some of the code? Even if it is always enabled, the code structure might become more regular? OTOH it could add size to the compiled code, of course.

[cmaglie]

Pushed other 3 commits.

Please have a closer review and also test if possible, because those commits have some code change too.

I'm done for today :-), be back tomorrow.

[matthijskooijman]

I see only 2 commits? Those look good to me.

[cmaglie]

Github orders the history of the pull request based on the commit timestamp (not the push timestamp), so you can find the commit if you reload the page and scroll back in the discussion, or maybe it's more simple to change to the "Commits" tab, btw this is the "missing" one:

cmaglie@ad36833

[NicoHood]

No need - global variables are zeroed by default (the attachInterrupt stuff need to initialize to &empty or something, not 0).

Yeah but we only want to initialize some of them. I just want to be sure the endpoint is not (wrong) configured if it shouldnt be at all.

I was going to say that you still need the count to interate the modules, but you can of course just loop until next is NULL.

We need this check anyways so the modul count is really not needed (like in pluggable HID we also do not count the devices)

I haven't looked at the code for this, but perhaps it would be worth considering to convert the CDC class to a pluggable class as well?

I was thinking of this as well. I also would like to see a whole deactivation of the USB-Core at all. This removes a lot of overhead for the 32u4 which can be used for non usb stuff very good. The u2 Series profits even more here.

TODOs from above:

• class PUSBListNode has two public sections. Merge them together please.
• Does PUSBListNode *next = NULL; needs to be public?

[matthijskooijman]

One more thing I realized wrt the static/non-static thing: AFAIU constructors for global objects are run in arbitrary order. This means that inside a constructor, other objects might not be fully initialized yet. This applies in particular to non-static members, which are always initialized in the constructor, whereas static members and global variables can be initialized by zeroing or copying the .data section (both of which happen before any constructors are called).

In this case, this can cause problems if a module is initialized before the PluggableUSB class, or a HID "submodule" is initialized before the main HID object. Not sure what the exact problems or solutions are, just wanted to throw this out there.

[cmaglie]

omg @matthijskooijman you're right, we hit exactly in the static initialization order fiasco, totally missed that :-(

https://isocpp.org/wiki/faq/ctors#static-init-order

The FAQ propose also doing the initialization using lazy construction through an helper function, for example:

HID_& HID()
{
  static HID_* obj = new HID_();
  return *obj;
}

and replace every occurrence of HID with the call to HID() so:

HID.SendReport(.....);

becomes

HID().SendReport(....);

Very ugly :-( don't know if there are other easier solutions.

[NicoHood]

I got the idea mainly, but not the connection to this API. Is it more a problem to have static members or to not have static members. This is above my c++ knowledge at the moment.

You mean the problem is, if the HID.cpp tried to call the PUSB.plug() function if it does not exist yet? Wouldnt this give a compile error or so? May you give me a real example related to our API?

[cmaglie]

Exactly that, and the compiler doesn't give any warning.

The problem is that static objects are constructed at the very beginning of the sketch (even before main()!) in an undefined order.

So the solution is to use a lazy construction like the one I've posted above, or to not call other (static) object's method from the constructor.

[NicoHood]

A singleton HID_ HID is also a static object?
We have mainly removed the static functions, so isnt this good now?

[cmaglie]

A singleton HID_ HID is also a static object?

yes

[NicoHood]

And what if we just add the hid this pointer to the root node linled list and call the init endpoint functions *that are now inside plug() with the pluggable constructor? Or there the same problem, whether all notes are added or not?

I have no real idea how to fix this. Is there no compiler attribute we can add?

[cmaglie]

Thanks @matthijskooijman, interesting read, I'll give it a try.

In the meantime I've pushed a solution with the lazy-construction trick, it doesn't look so bad in the end.

cmaglie@65b8430

[cmaglie]

@matthijskooijman, I've read about constexpr, very interesting stuff, but I'm not confident enough to propose a solution based on that right now...

I've tried to revert back the various rootList as static: cmaglie@07c7fde
this solves the problem without the need to use HID() instead of HID.
The cons is that the code now is a bit more complex because it we need an init() function that does all the non-static initialization after the construction. Moreover this solution doesn't provide any significant gain in Flash/RAM usage compared to the other one based on the HID() function.

[NicoHood]

I'd vote against the static HID class, since I want to inherit from this and creat multiple HID devices. (with a single module). This is required since not all HID features can be merged in a multireport and needs more than a single endpoint to work properly.
Maybe I need to dupe the code anyways but then I still do have the problem...

I have no idea to solve it though. But I will test the new build now.

[facchinm]

Hi Nico, you can specify multiple endpoints in an alternative implementation of HID library; I am still convinced that that class should remain as simple as possible.
Anyway, Mouse and Keyboard can still be singletons because they are the last link in the chain so no one can call their methods before their constructor is executed

[NicoHood]

Then the solution sounds reasonable.
After burning a new bootloader to my new hardware the HID/USB Core itself seems to work. Only tested the SerialKeyboard example real quick.

I looked at all commits of the PR anyways, looking good so far. This weekend I hope I can test what I can do this these updates now. Anything that is still on the todo list? Is this static thing now 100% fixed?

You can remove this public statement:
https://github.com/cmaglie/Arduino/blob/65b8430fece7a708be740089864835a862678cff/hardware/arduino/avr/cores/arduino/PluggableUSB.h#L49

[NicoHood]

I found a new problem:
If we have multiple HID devices int PluggableUSB_::getDescriptor(int8_t type) will only return the very first HID device since the type is always the same. We need to pass the interface number here instead I guess.

Originally I was trying to remove the interface check to the Pluggable USB instead of the HID class to avoid code duplication, passing a variable and coding errors.

[NicoHood]

Next problem I found is that the API is currently not supporting multiple interfaces per plug. I am not sure if this is really needed to have but there are two options:

• Remove all Interface counts etc that just take flash for nothing
• Make it fully pluggable

It seems that the CDC Serial uses 2 interfaces. Not sure if this can be realized with 2 plugs or better with a real multiinterface API. We should defenitely consider to make the CDC class pluggable as well. This would save us some bytes. Not sure if we still have the static problem here as well.

Once again to the static problem:
Good that we solved it like this, but what If a user wants to create a singleton for a plugged interface? Then the API would look uglz with the Singleton(). I think If you port MidiUSB now to this API it will look ugly. Its a real problem...

I also made some changes to the API (on top of this PR). Please have a look (each commit pls):
https://github.com/cmaglie/Arduino/compare/cmaglie:phid-class...NicoHood:plugfix2?expand=1

[cmaglie]

If we have multiple HID devices int PluggableUSB_::getDescriptor(int8_t type) will only return the very first HID device since the type is always the same. We need to pass the interface number here instead I guess.

This is an interesting one, looking at the sequence of calls starting from the ISR we have:

//  Endpoint 0 interrupt
ISR(USB_COM_vect)
{
[...]
    USBSetup setup;
    Recv((u8*)&setup,8);
[...]
    u8 requestType = setup.bmRequestType;
[...]
    if (REQUEST_STANDARD == (requestType & REQUEST_TYPE))
    {
        u8 r = setup.bRequest;
        u16 wValue = setup.wValueL | (setup.wValueH << 8);
[...]
        else if (GET_DESCRIPTOR == r)
        {
            ok = SendDescriptor(setup);    <---- enters here
        }
[...]
}

static bool SendDescriptor(USBSetup& setup)
{
    int ret;
    u8 t = setup.wValueH;
    if (USB_CONFIGURATION_DESCRIPTOR_TYPE == t)
        return SendConfiguration(setup.wLength);

    InitControl(setup.wLength);
#ifdef PLUGGABLE_USB_ENABLED
    ret = PluggableUSB().getDescriptor(t);     <--- enters here
    if (ret != 0) {
        return (ret > 0 ? true : false);
    }
#endif

int PluggableUSB_::getDescriptor(int8_t type)
{
    PUSBListNode* node;
    for (node = rootNode; node; node = node->next) {
        int ret = node->getDescriptor(type);
        // ret!=0 -> request has been processed
        if (ret)
            return ret;
    }
    return 0;
}

so we don't have the interface number information anywhere in the chain.
Maybe the setup.wIndex contains something that could be of interest here?

It seems that the CDC Serial uses 2 interfaces. Not sure if this can be realized with 2 plugs or better with a real multiinterface API. We should defenitely consider to make the CDC class pluggable as well. This would save us some bytes. Not sure if we still have the static problem here as well.

The pluggable should already handle multiple interfaces, you just need to instantiate the PlugNode with the value of 2 on numIfs.

[NicoHood]

Yes you can set numIFs o 2. But how can we actually call a methode of interface 2? This was too long ago since I looked at the code, but I think there were some parts where not all interfaces can be used. The implementation seemed not 100% completed/compatible to me. Well I have to look again later.

[ArduinoBot]

Build successful. Please test this code using one of the following:
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3896-BUILD-412-linux32.tar.xz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3896-BUILD-412-linux64.tar.xz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3896-BUILD-412-windows.zip
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3896-BUILD-412-macosx.zip

[cmaglie]

But how can we actually call a methode of interface 2? This was too long ago since I looked at the code, but I think there were some parts where not all interfaces can be used. The implementation seemed not 100% completed/compatible to me. Well I have to look again later.

Thanks for looking into this. In the meantime I'm merging this one so we can align the other implementations (for SAM and SAMD) as well, and eventually add the latest bit coming from your analysis.

Looking at the CDC implementation:

• CDC_GetInterface sends the whole USB descriptor altogether (for all the interfaces), without worring too much about interface number.
• CDC_Setup didn't check for iface number but this is an implementation detail, the information is available and we can add it when we'll made CDC itself pluggable.
• all the communication happens inside specific endpoints.

So, in the end, the USB-CDC seems to not suffer the problem above.

[NicoHood]

Meh you deleted the branch. Now I cannot compare my changes real quick...

The multiinterface would be "disabled" with this commit I made:
NicoHood@98f331b

So its better to check the value in the HID class itself.

And yes we should rather use the wIndex value to check the Interface number. So we a) dont need to pass a 2nd value and b) we may remove the interfacetype and use this instead.
https://github.com/abcminiuser/lufa/blob/master/Demos/Device/LowLevel/KeyboardMouse/KeyboardMouse.c#L146

I also got a new idea (pretending the multi interface thing works) is to make the pluggable HID to also plug different Interface or endpoints. Depending if you want to use a multireport device or now. I will try this out myself.

TODO:

• fix the get and send names. Over the whole USBCore and Pluggable, but dont mix
• Remove the 2nd param in getInterface and setup and use wIndex instead
• make the endpoint number uint8_t. I saw some mixing there and unsigned was the better choice.
• I am not sure if the static initialization problem is fixed like that. I am not sure if its still flexible enough with this different syntax. (e.g. USB MIDI). One should try this out.
• Did I miss anything?

I guess you go ahead and fix this yourself since I currently have less time and want to avoid doing the work twice. Your commits look really good and I am really happy to see the change :)

Oh and I would not apply this to SAM right now. Give it some more time, just a tiny bit before we change/revert stuff. I guess we could have some problems with Midi USB. Maybe @facchinm should try to fix this first and see if the PUSB is flexible enough for this. (regarding the static problem especially)

[NicoHood]

Meow.
#3931

To rename the functions is up to you.
I think thats mainly it then. I need some time to work on the HID Core then. Maybe we will backport my additions or leave it like this. I will start working on this once my few commit above are merged.

[NicoHood]

May somebody explain me the static fix once again?

I think you currently cannot add multiple instances of HID like that. Originally I wanted to create a main HID class and inherit from this multiple interfaces. I think this is impossible now.

However I think I can solve this with a multiinterface HID class. You then can plug the HIDDevice to any interface number and give them unique interfaces if desired.

But then it is required to create a flexible endpoint type array and interface number etc which is generated at runtime. The Keyboard/Mouse constructor needs to be called first and then it adds them to the HID class. However this requires those variables to be non const:

  const uint8_t numEndpoints;
  const uint8_t numInterfaces;
  const uint8_t *endpointType;

Is this correct? Can anyone follow?

Then we can have (all pluggable):

• Interface 1 RawHID
• Interface 2 BootKeyboard
• Interface 3 Multireport with:
  • Mouse
  • Consumer
  • etc
• Interface 4 Gamepad (works better as separate interface)
• (not that CDC Serial limits this. If this would be pluggable too we could remove this and create awesome 4 player gamepads etc)