[PluggableHID] 3rd round of API improvement (WIP)

[NicoHood]

cc @cmaglie @facchinm @matthijskooijman

• Fixed uint8_t endpoint and interface var
• Pass the setup struct to the nodes. This fix was required to add multiple interfaces with the same type.
• Added an HID function (that is not used but available in general)

Also see (and below)
#3896 (comment)

[cmaglie]

Everything looks fine thanks!

The only thing that I'll add is a more strict check for interface as explained in my comment NicoHood@0f9f63f#commitcomment-13661401

[facchinm]

Hi @NicoHood , first of all thanks for all these useful patches.
I was testing your patchset and I adapted a small library I was writing to implement a multiinterface-multiendpoint HID to handle n Mice at once.

Test it if you wish, I used this sample sketch

#include "Mouse_multi.h"

Mouse_ Mouse_0(0);
Mouse_ Mouse_1(1);

void setup() {
  // put your setup code here, to run once:
}

void loop() {
  // put your main code here, to run repeatedly:
  Mouse_0.move(100,100);
  delay(100);
  Mouse_1.move(-100,-100);
  delay(100);
}

With evtest you can see that the movements are performed by different input devices (you could to the same with a multireport adding a quirk to the usbhid module)

[NicoHood]

I will try this out now!

Thinking about this I'd say that the pluggable HID in general should use multiple endpoint/interfaces. This gives us some very important features:

• More reliable, works on any PC
• Boot Protocol compatibility can be achieved
• RawHID and such things can be implemented
• Gamepads would work better
• Multireport is still possible (on a 16u2 useful with only 1 free endpoint if Serial is used)

Since Arduino by default only supports Keyboard + Mouse with a 32u4 we have enough endpoints to do this. Even 1 more endpoint is free. Boot support is also a very desired feature around the web. The pros above should make it clear enough.

This way we are most flexible in any way. We (or better me) could then create another layer of pluggable Multireport which can be used for u2 devices with less endpoints.

Let me see what @facchinm s example already gives us ;)
Oh and working on a fork is better to compare the changes...

[NicoHood]

Thinking about this twice results in:
It makes more sense to treat every HIDDevice as USB plug. Meaning Keyboard and Mouse are just independent. And the current HID library should be renamed to MultiHID and the current Keyboard + Mouse to MultiKeyboard/Mouse.

Otherwise we end in using tons of linked lists with a lot of overhead. This way its also easier to implement HID Out reports (leds, rawHID data to MCU). Adding single interface HIDDevices via linked list makes no sense to me, also because they differ too much which gives a lot of overhead. For this purpose we have the awesome pluggable USB API.

The APIs however should be kept that you can inherit them and dont need to dupe the code (if you want a keyboard as multiHID or normal). Not sure if thats possible without overhead/limitations.

This also means the current API is fine how it is. Maybe it should be renamed and a proper Keyboard/Mouse API should be added (with boot support and all that). Or you leave it like that and I just add those features to my HID Project.

[NicoHood]

https://gist.github.com/NicoHood/c5e77a3f9d73b998a49f

Leds currently dont work, 2 additional headers needs to be copied from the HID Project. Just to get the idea. I tested this code and it works. Havent tested the Boot function but it should (and if not there is a fix for sure because it works in the current HID project with old PUSB).

I am now wondering if your PUSB core is full avr/sam independent. If yes you could just use the same API. If not, its not solved this easy.

After applying a few changes I think the best solution is to keep it like that. If someone really want to add a special HID device real quick, he now can. I also do really like the pluggable HID and I will use it (how it is right now). However a real working HID device with more than just simple functions requires a real interface anyways. And for those features I will just provide a special 2nd API/Singleton. So people can use the (pretty good) pluggable HID with a single endpoint or extended singe EP HID Devices.

Depending on your opinion I will start working on my HID Project and say those changes are final now. The sam implementation requires some time to patch anyways. However I dont have one to test, so I am out of that.

So any opinions?

Edit: Oh and we have to make the Serial pluggable. I am really looking forward to see this. ISerial may then be implemented as well @facchinm
Please also have a look at this once again:
#3679

[NicoHood]

And another thing I want to mention: It should be possible to create just two instances of this class and you have just two keyboards. This is especially useful with gamepads of course. Or with multiple RAW HID channels, but then a different descriptor is needed (another story). This might also be useful to create a dual Serial interface. Lufa has an example for this, not sure if its so easy to do, or If you need to add more tweaks. But two debug channels, how cool is that? (however then no more HID device is supported since it uses all 6 endpoints)

And we should also make the endpoint size flexible. This way we can increase/decrease the buffer size of the serial or of the HID devices. Especially for the u2 Series we are currently limited to 16bytes single bank which slows down the serial a bit. However if this change requires too much work or overhead I'd just leave it like that.

[cmaglie]

Hi @NicoHood,

I've just read all your emails, at some point I felt a bit lost, so let me recap your conclusions:

• The current PluggableUSB is good enough as it is now. If you need a "basic" HID device with multiple-reports inside a single usb iface/ep the current HID library can be used. If you need an advanced or particular HID devices the PluggableUSB class may be used instead.
• I see your reason for asking a rename of HID to MultiHID and Keyboard/Mouse to MultiKeyboard/MultiMouse, but I don't want to change this to keep compatibility with the old API (where Mouse and Keyboard were predefined objects). We are happy to leave a better implementation of Keyb and Mouse to you :-)

So, if I've interpreted your emails correclty, before declaring closed the refactoring, I would like to rename the following classes:

• rename PUSBListNode to PluggableUSBModule
• rename HIDDescriptorListNode to HIDSubDescriptor

the word "List" in this case is really an implementation detail and the new names have a much better meaning IMHO.

[NicoHood]

Yes you are right. I think for the simple stuff the API is good how it is right now. And the rest can be achieved via PUSB.

• The names you suggested look very good to me
• Also rename getDescriptor and sendDescriptor. They are mixed up between USBCore and PUSB.
• Serial should be made pluggable. If we could instantiate the Serial class a 2nd time this would be even greater!
• Flexible EP sizes?
• Please also have a look at my USB Core deactivation patch, which currently doesnt destroy anything in the IDE, but gives a simple addtion to deactivate the core for advanced users.
• If you are fine with all the changes I will start working on the HID over the weekend. The Arduino HID core itself looks good to me like that.
• If there is something I'd still like to change, I will let you know for sure. The PUSB gives me enough free room to solve the things I want to do I think.
• In general i suggest not to read the emails since i edit my posts almost every time.

I was thinking of making the Keyboard API etc even more abstract, so one can use that as single device or as multidevice. However I also think that this is not 100% needed for the core IDE and I can better do this myself (with HID Project) anyways. It s now implemented in HID Project 2.4 dev branch.

[NicoHood]

Could you please add the following code (with an improved for() loop for the nodes) to the code (commented):
https://github.com/NicoHood/HID/blob/b28d83b1601a0566844bc884131ceff6b886b30e/src/PluggableHID/HID.cpp#L153-L193

Oh and int length = ((setup.wValueH << 8) | setup.wLength); is wrong, it has to be int length = setup.wLength;

I had a reason to add this:
NicoHood@c886746

Because implementing this feature is not that hard - if you know how to do.
We dont need to implement this for arduino, but the information should not get lost.
In my HID Project I will only implement this via a single Interface HID Device which works a bit different. It would be nice to have the information somewhere, so people can extend the functionality if desired, I will not.
Added here: #3948

Current dev states can be found here:
https://github.com/NicoHood/HID/tree/dev_2_4

[NicoHood]

Another thing that you really should do is to move the static inline functions of USBCore.cpp inside a header file. I wanted to use the Send8() function from my HID implementation but I cannot do this, because no header has this function. Its okay to make it inline, but please put it inside a header.

Also The USB Core (Not the pluggable USB) looks very incomplete to me. We should maybe rework this as well and keep similar names as the LUFA project does. This way the code is easier to read, understand and features can be added easier.