Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Pluggable USB core for AVR #3304

Merged
merged 39 commits into from Jul 16, 2015
Merged

Conversation

facchinm
Copy link
Member

@facchinm facchinm commented Jun 8, 2015

This PR introduces the concept of Pluggable USB modules to help getting the best from our tiny ATmega32u4 without overhauling the core library with functions that could be useful for no more than 10% of the users.

Using this method we can plug a library using the USB core by simply including its header in the sketch, and if not used it will not occupy additional resources.

It is of course a work in progress (thus tagged as RFC), still not ported to DUE core, and VERY probably bug-prone. Also, the memory footprint is not minimized.

Useful infos

  • the CDC module is now non-removable (commit 43701fd)
  • the Pluggable USB module inherits most of the code from old HID and replaces its functions with hooks
  • max additional endpoints is arbitrarily set to 3
  • HID, Mouse, Keyboard and MouseAndKeyboard libraries are "generated" from master code (so all code from Overhaul USB HID for Leonardo and Micro聽#1803 can be merged in a second time)
  • a MIDIUSB library based on PluggableUSB has been added (it is a very raw porting of USB MIDI for Arduino聽#2943) only to demonstrate how to plug multiple modules
  • setupUSB() weak function has been added to main.c , so it will execute before setup() if USBCON is defined.

Known issues

  • I'll add them here as soon as they get discovered (no doubt this list will grow soon 馃槃 )

How to try it
I decided not to change any provided example . If you wish to test the core, download the @ArduinoBot generated packages and modify the examples in this way:

On the top of the file:

  • Mouse only examples: add
#include "Mouse.h"
#include "HID.h"
  • Keyboard only examples: add
#include "Keyboard.h"
#include "HID.h"
  • Mouse and Keyboard examples: add
#include "MouseAndKeyboard.h"
#include "HID.h"
  • HID and MIDIUSB example:
#include "MIDIUSB.h"
#include "HID.h"
#include "Mouse.h"

@NicoHood
Copy link
Contributor

This really sounds good to me. I havent looked at the code yet, but I will test this for sure.
I also made the CDC etc pluggable. Not via include, just because I moved the code in a seperate .cpp file which is not linked if not used. This saves a lot of space.

Also I would provide an include "USB.h" to enable USB stuff. Then users can remove the whole USB core and save even more space if not needed and also avoid problems with the PC.

You may want to have a look at my core and at the dev branches to get new Ideas possibly:
https://github.com/NicoHood/HID

@facchinm
Copy link
Member Author

Hi Nico,
thanks for taking the time to watch this PR!
I obviously took a look at your core before starting this effort and I think that most of your patches (and other peoples' patches as well) could be integrated if we are able to modularize the USB 馃槒
The biggest problem in using your approach is that you need to:

  • provide a lot of board variants (too noisy) or
  • let the user modify the #defines in the core itself (too advanced)

Also, your core handles a lot of HID-based devices but it's not ready for other kind of devices (like MIDI #2943 or something else that I can't imagine at the moment 馃槃 )

However, if you could test the patchset it would be great, I'm only waiting for the feedback!

@NicoHood
Copy link
Contributor

Well I have a midi option that I could integrate. Its just under a dev state so I did not create a proper menu yet. Also it has a 2nd Audio device with errors under windows. I just used the arcore code for this.

I know that the boards file is not perfect, but it was the only way for me to simply add a menu to the IDE.

I really like your IDE to make the USB stuff more flexible. You know that this will change a lot of code. Is the Arduino Team willing to change it? Because so many people provided new features but mostly none of them were integrated yet. You are collaborator, does this mean that we can count on a PR merge if its good?

You should also consider to fix this bug when you update the USB core:
#2474 (comment)

And you should also consider to update the bootloader fuses and watchdog behaviour in order to fully deactivate the USB-Core. Right now the bootloader starts the sketch directly. You could better to a watchdog reset and start the code independent. The difference is that all settings like the USB clock is reset. Without this fix you have to add the USB clock ISR into the core as workaround. I could explain it better if someone is willing to update the bootloader in general. This would make life much easier.

I am not likely able to test your code this/next week, I am too busy, sorry. But I will have a look for sure.

Edit:
How do you want to solve different HID devices? Do you want to add a specific Endpoint for each device? This could be tricky if users want to add more devices. The 32u4 has only 6 endpoints by the way. Also you have to keep in mind that changing the endpoints makes windows go crazy. you better add more PIDs for each device. You have 2^16 as Vendor, so it shouldnt be that big Problem.

@matthijskooijman
Copy link
Collaborator

Overall, this looks like a nice implementation, using a linked list of modules and separate libraries is an elegant way to solve this.

setupUSB() weak function has been added to main.c , so it will execute before setup() if USBCON is defined.

Isn't a weak setup function completely counter to the idea of pluggable modules? Only one module can implement this setup function, otherwise you'll get linker errors (or if the modules make their implementation weak too, they'll get silently ignored). Wouldn't an init function (since the setup name seems to be used already for processing control packets) in the callbacks make sense instead?

Would it make sense to write some documentation about how to use this now? Having documentation would help to understand and review the PR, and writing documentation after merging usually means it'll take months before that happens...

@NicoHood
Copy link
Contributor

What do you mean by cb variable?

Can we also see u2 Series support with this update? HL2 is getting more and more popular this could be a great time to merge everything together.

@NicoHood
Copy link
Contributor

I will add comments/thoughts to the PR here (this post will be edited):

The different Mouse/Keyboard/MouseAndKeyboard HID reports wont work properly under Windows I want to note. You have to use different PIDs to make the driver recognize it properly or reinstall the drivers and provide a correct tutorial somewhere.

May we get some diagrams on how this works together? So others can understand better. I have to fiddle through this right now.

And I have to mention that I just looked at the code in small parts. I do not understand it as a whole thing since I never intended to understand it.

#ifdef HID_ENABLED
total += HID_GetInterface(&interfaces);
#ifdef PLUGGABLE_USB_ENABLED
PUSB_GetInterface(&interfaces);
#endif

return interfaces;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns always 0. Whats the sense of that function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does't return 0 because interfaces is incremented by the callee (instead, the original total variable was completely useless)

@matthijskooijman
Copy link
Collaborator

What do you mean by cb variable?

I presume this was in reply to this comment

I mean that instead of:

cb.setup = &HID_Setup;
cb.getInterface = &HID_GetInterface;
cb.getDescriptor = &HID_GetDescriptor;
cb.numEndpoints = 1;
cb.endpointType[0] = EP_TYPE_INTERRUPT_IN;

you could do:

static const PUSBCallbacks cb = {
    .setup = &HID_Setup,
    .getInterface = &HID_GetInterface,
    etc.
}

@facchinm
Copy link
Member Author

@NicoHood @matthijskooijman Thank you so much for taking time to analyse the (huge) PR. I'll try to update it following your advices as soon as I can get some spare time.

About OS-specific issues, I think that Nico is our man, so any help is really appreciated!

@facchinm
Copy link
Member Author

@matthijskooijman about the weak function, I could not imagine anything better.

From my tests the (also weak) setupUSB functions contained in the library do overwrite the one in main.c (I think it's only a linking order side effect but it works).

However, anyone can freely overwrite it with its own non-weak implementation (like in the MIDI+HID example). Any alternative however is very welcome 馃槃

@matthijskooijman
Copy link
Collaborator

However, anyone can freely overwrite it with its own non-weak implementation (like in the MIDI+HID example). Any alternative however is very welcome
The problem is that when you override the setupUSB function, all previous overrides will be discarded. This means that a sketch that includes multiple USB libraries, must provide its own setupUSB function that calls all the appropriate begin methods.

@matthijskooijman about the weak function, I could not imagine anything better.

Looking more closely, I see you are using setupUSB to call e.g. HID.begin(), which eventually calls PUSB_AddFunction() to register the usb module and its callbacks. My previous suggestion, to use a callback instead of a weak setupUSB function of course doesn't work, since it is needed to register the callbacks in the first place.

There is a good alternative, though: Use a global object, whose constructor calls PUSB_AddFunction(). Constructors of global objects are called on startup automatically.

As an example of how this works, see ModuleHandler and Module in https://github.com/Pinoccio/library-pinoccio/tree/master/src/modules Those register a Module* in a linked list inside ModuleHandler, which is the equivalent of calling PUSB_AddFunction

One caveat is that this all runs before main() is even started and the initialization order is undefined, so you should check that PUSB_AddFunction does not need any constructors or other initalization code to run (global variables initialized to 0 or some other constant are ok, zeroing of the .bss section and loading of the .data section happens before calling global constructor). A quick look around the code shows that this is already ok.

I even think that this approach doesn't require separate libraries for every module, since the compiler can completely remove a module, including its global constructor, if it's not referenced anywhere at all. I'm not 100% sure about this, though. And in most cases, using separate libraries is sane approach regardless of this.

I had a closer look at the HID libraries, and I'm not so happy with how those look. AFAICS, the Mouse and Keyboard libraries both define a few specially-named symbols, which are then used by the HID library. This of course doesn't allow more than one HID "submodule", which is why there is a KeyboardAndMouse library, which is of course fairly ugly.

Ideally, an approach with callbacks similar to PUSB_AddFunction is also used for HID submodules, allowing them to be completely separate from each other. However, I'm not sure if the HID descriptors are actually easy to compose (e.g. can you just concatenate or otherwise combine the Mouse and Keyboard descriptors into something valid)?

@facchinm
Copy link
Member Author

  • You are right about the ugliness of HID submodules (their need of special symbols especially) but the descriptor must be unique so only one #include is allowed "by design" (also enforced by 18a2f2a)
    Of course a fully modular solution is the way to go but from a library creator's point of view it would probably be more complicated than defining a specially named function (I'm thinking about descriptors overriding)
  • Thanks a lot for the global constructor tip, I missed that completely (I'm a C guy 馃槃 )

Now the produced binary should be comparable (in size) with the non-modular one so we can start thinking about integrating the various PR as libraries and test the results

@facchinm
Copy link
Member Author

@ArduinoBot build this please

@facchinm
Copy link
Member Author

Size comparison with the current code:

Sketch Modular Old
KeyboardLogout 5574-218 5030-153
KeyboardSerial 4868-216 4324-151
KeyboardAndMouseControl 5704-217 5102-151
ButtonMouseControl 4962-195 5110-155
empty sketch 3476-133 4254-151
ASCIITable 4364-211 5144-231

@facchinm
Copy link
Member Author

https://github.com/facchinm/Arduino/tree/testPlugUSB_PR+1803 contains a complete porting of PR #1803 adapted to the Pluggable framework

@facchinm
Copy link
Member Author

facchinm commented Jul 2, 2015

Almost end of week update:

  • HID is now pluggable itself! You can add
#include "Mouse.h"
#include "Keyboard.h"
#include "HID.h"

and the descriptor will be the combination of the submodules' descriptors. The method is not very memory efficient; however you can still write a library which plugs on PluggableUSB directly, embedding in it an HID implementation and only the functions you need.

@NicoHood @matthijskooijman @obra @follower @cmaglie @madrang @damellis @PaulStoffregen @weizenspreu and all people interested in USB on AVR, please test the latest @ArduinoBot image to make sure that nothing breaks spectacularly before merging 馃榿

Thank you very much!

@obra
Copy link
Contributor

obra commented Jul 2, 2015

Oh wow. That's amazing. Thank you so, so, so much. I'm on the road this week but will test when I can.

On Jul 2, 2015, at 10:26 AM, Martino Facchin notifications@github.com wrote:

Almost end of week update:

HID is now pluggable itself! You can add
#include "Mouse.h"
#include "Keyboard.h"
#include "HID.h"
and the descriptor will be the combination of the submodules' descriptors. The method is not very memory efficient; however you can still write a library which plugs on PluggableUSB directly, embedding in it an HID implementation and only the functions you need.

the PR now contains @matthijskooijman C++11 patches to get rid of a nasty warning

next step: enable lto (reduces flash occupation by 400 bytes) https://gist.github.com/facchinm/432fbb8ca4d9705644bd

@NicoHood @matthijskooijman @obra @follower @cmaglie @madrang @damellis @PaulStoffregen @weizenspreu and all people interested in USB on AVR, please test the latest @ArduinoBot image to make sure that nothing breaks spectacularly before merging

Thank you very much!


Reply to this email directly or view it on GitHub.

@NicoHood
Copy link
Contributor

NicoHood commented Jul 4, 2015

@yyyc514 of course it is interrupt driven. Not sure if your idea then still makes sense.

@ facchinm Also see this:
NicoHood/HID#42

Going to test this sooner or later... Sounds good at least!

revert this commit when it's time to integrate this library
expect way more interesting user-generated libraries
@cmaglie cmaglie merged commit af290fc into arduino:master Jul 16, 2015
@NicoHood
Copy link
Contributor

huh? that was quick. I didnt even had a chance to test this yet.
Did you got feedback from somewhere else? I am a little surprised that nothing happened related to USB for so long, and now this fix is merged so fast.

Not that I feel bad about it, I should really go ahead and try this now!

@cmaglie
Copy link
Member

cmaglie commented Jul 16, 2015

I think that this is one of the most-desired patch ever (just look the number of PR/Issues), so we decided to merge it upstream to accelerate tests and adoption.

On our side we tested it on everything, BTW this patch is not yet released, so we are still in time to fix bugs or (in the worst case but I hope not :)) completely revert it.

@NicoHood
Copy link
Contributor

Good idea, I think its a good idea to improve the usb stack.

@yahesh
Copy link

yahesh commented Jul 16, 2015

What do you mean by "quick"? This topic took now 1.5 years to come any further.

Am 16.07.2015 um 15:36 schrieb Nico notifications@github.com:

huh? that was quick. I didnt even had a chance to test this yet.
Did you got feedback from somewhere else? I am a little surprised that nothing happened related to USB for so long, and now this fix is merged so fast.

Not that I feel bad about it, I should really go ahead and try this now!


Reply to this email directly or view it on GitHub.

@cmaglie cmaglie added this to the Release 1.6.6 milestone Jul 16, 2015
@NicoHood
Copy link
Contributor

Laugh out loud :D

Yes I know. And this PR started about a month ago and is now merged. Nevermind, its GOOD that there IS progress.

@facchinm
Copy link
Member Author

Initial howto on writing a PluggableUSB or PluggableHID based library

https://github.com/arduino/Arduino/wiki/PluggableUSB-and-PluggableHID-howto

@NicoHood
Copy link
Contributor

May you eliminate the u8 which only causes problems?

Edit: Will have a loom of course ;) Lookin good.

@NicoHood
Copy link
Contributor

Any chance to see a pluggable keyboard layout?
NicoHood/HID#33

@facchinm
Copy link
Member Author

Well Nico, a library providing multiple layouts is now possible, you can write it by yourself and we'll be glad to include it in the library manager 馃槈

@NicoHood
Copy link
Contributor

Looking at the code now, adding notes here via edit:

  • Overall it looks very nice to me and a simple example just works fine.
  • Found a typo in the instruction: Then you can then perform USB writes calling, for example,
  • begin() should return a bool and if possible it should wait till the HID is initialized. Maybe this is impossible, but I noticed problems like this: HID needs to wait until initialized聽NicoHood/HID#29 What about while(!HID); like the Serial? Or remove it completely.
  • u8 should be replaced with uint8_t if you ask me.
  • Keyboard/Mouse.begin() should reset the report to zero. It should be inlined into the header to save flash.
  • It would be very helpful to set the EP Size flexible, not only 64

@facchinm
Copy link
Member Author

Adding the bool() operator is a good idea!
About u8, they will be replaced in the libs when PR #3542 will be merged.
Typo fixed, thanks 馃槈

Endpoint flexible size is a bit of a mess, but if you come up with a solution I'll be very happy to review it. Also, feel free to open PRs for Keyboard and Mouse libraries modifications.

@NicoHood
Copy link
Contributor

I am currently working on a PR to backport all my HID Project fixes. Already done with the major core fixes, HID libraries will follow.

The flexible EP_SIZE needs a bit more core changes, however I applied my simple fix for now in the PR (soon).

What is more important is this:
NicoHood/HID@daa80ec...dev#diff-5478f073c4245fb29e2a0975e99c647eR101

At the moment you set those values wrong. USB cannot be pluggable in every situation. So I dont know how to solve this the best way, but we should at least try to add a correct CDC descriptor.

Let me give my fixes a last check and I'll open a new PR for this.

@NicoHood
Copy link
Contributor

Edit:

  • Also think about making the whole USB core a library if possible. Then one can deactivate it if not needed since it uses a lot of flash and ram. You need a fix for the 32u4 since the Arduino Bootloader doesnt clear the USB Interrupts correct. I have this in my HID project, the empty ISR of the usb clock just needs to be added (maybe weak) to the core. Maybe it would make sense here to update the bootloader once more with the RAMEND fix. So people who really want to deactivate USB have to update the bootloader but we then won't need those workarounds.

@NicoHood
Copy link
Contributor

There we go:
#3640

Edit: very nice work overall. Took some time to understand the whole thing. Still looking through the code. Just tell me how we should continue.

@NicoHood
Copy link
Contributor

NicoHood commented Aug 4, 2015

@facchinm Hey, you did a lot of great work. Would you be so kind to have a look at the PR to apply my small fixes?

If you disagree with the CDC Events and control line stuff I can remove it. There is another PR my mathijs I think who did something different, maybe better than that. But the rest of my PR is not much to look at but would make the core more flexible and fixes a few problems.

Removed, now this PR can be used:
#3640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: USB Device Opposed to USB Host. Related to the USB subsystem (SerialUSB, HID, ...) feature request A request to make an enhancement (not a bug fix)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants