Overhaul USB HID for Leonardo and Micro #1803
Conversation
| @@ -0,0 +1,34 @@ | |||
| /* AbsoluteMouse.ino | |||
|
|
|||
| For ATmega32U4 based boards (like the Leonardo and Micro | |||
|
Is it possible to allow users to customise the USB descriptors so libraries can add support for other types of devices ?? Also this would allow to move the Mouse and Keyboard code in a different library that would only be included when needed. This way users would only import what they need to save space. |
|
I would love to see patches for that. Not sure what the right extension mechanism for that would look like Marc-Andre Ferland notifications@github.com wrote:
Sent from Kaiten Mail. Please excuse my brevity. |
|
It's not the keyboard that handles the locking states (NumLock, CapsLock, ScrollLock) but the operating system. Unfortunately the Arduino Leonardo does not support this. There are solutions to this in the forum ( e.g. http://forum.arduino.cc/index.php?topic=166638.0 or http://forum.arduino.cc/index.php?topic=173583.0 ) and it would be great to see this feature in the overhauled USB HID library. |
|
On Sun, Feb 02, 2014 at 07:07:33PM -0800, Kenneth Newwood wrote:
I'd love to see a version of that patch with slightly cleaner code, maybe some comments and an example. (Also, clarification from the author that it's ok to include in the Arduino core) |
|
Hi @obra Another important aspect to consider is how much this patch impact on the sketch size, I went through some of the base examples:
It seems that there is a fixed increase of 850 bytes, that is a lot. It may be due to the modified USB descriptors? can we figure out a way to reduce the space when some of the features are not used? |
|
On Thu, Feb 06, 2014 at 09:26:21AM -0800, Cristian Maglie wrote:
No worries!
Thanks! The devil, as they say, is in the details and I very much don't want to be the guy who ruined Arduino 1.5.
I've only tested it with the Micro, as I don't have a Leonardo (or any other 32u4 board). If it'd help things along, I'm happy to buy additional Arduinos for testing. Just tell me what you'd like me to try. On the host side, I've tested on Windows 7 and 8, OS X 10.7 and OS X 10.9, and Android 4.4. Is there a documented matrix of configurations that need testing or a testing methodology?
I believe you're correct on the cause of the bloat. I suspect the USB-wakeup code is also part of it. I would be thrilled to work to cut down on the memory footprint. My C++ is pretty rusty, but there are two obvious-seeming ways to cut down
The downside of this approach is that it makes using the new features I expect that the answer is 'no' as it is in the broader C/C preprocessor world, but is there a way in Arduino C to set a compile-time define from within a run-time method. If it is possible, then this all gets a bit simpler and we could shrink the default runtime to smaller than it is today by conditionally compiling the USB keyboard and mouse HID descriptors.
I have no idea what that would look like, but it would let us move most of the HID code into libraries, which would be a lot more flexible and maintainable. I'm 100% happy to do the work on this, but I'll need some coaching so I don't screw it up. Thanks! Jesse |
|
Should be possible to make this work with virtual functions. |
|
I'm having a hard time imagining an inheritance model (rather than plugin model) working well here. That might just be my lack of familiarity with the code base, though. Is there anywhere else in the arduino core that does this so that I can see that it's not scary and wrong? Best,
|
|
Jesse, your test cases seems already quite wide, I didn't expected that. On the board side I would like to add the Leonardo and on the Host side Linux 32/64 and WinXP, but I can manage to make the missing tests here so don't bother trying nor buying other boards :-) About reducing the memory footprint: I don't have a precise idea, it's a hard problem indeed, I asked hoping that someone would come up with a solution. The most obvious approach is to use Composing the USB descriptor at compile-time seems hard too, I can't think of any C/C++ trick to make it happen. An alternative may be to split USB descriptor into pieces and send them all at once at runtime with some chaining but, again, this is just another hypothesis and requires an amount of refactoring on the actual code, I don't know if it's feasible. mmhhh |
|
On Fri, Feb 07, 2014 at 09:00:07AM -0800, Cristian Maglie wrote:
Thanks :)
nod I've actually now got a tester on WinXP. And I've done some limited testing on Linux (x86 64) but I wouldn't want to claim that I've run the current code.
And I worry that that could actually have its own bloat associated with it. Looking at main.cpp, there's just no hook provided before the USB device gets It feels like Arduino could give developers a bunch more rope if there were another pre-setup function called before that USB connection were made. I havent' tried to prototype anything since that seems like a pretty fundamental sort of change. #include <Arduino.h> int main(void) #if defined(USBCON) |
|
What if we made a virtual usb hub and each library register it's own device on it ? |
|
IMHO its not a problem to add an additional function call to the main.cpp if needed (maybe USBinit() instead of init(), but you got the point). |
|
On Mon, Feb 10, 2014 at 03:25:37AM -0800, Marc-Andre Ferland wrote:
That seems like it would end up with a lot of overhead, but I'd love to see a prototype. |
|
On Mon, Feb 10, 2014 at 03:30:23AM -0800, Cristian Maglie wrote:
nod I'd probably give it a name that's not USB specific, just to improve reusability. |
| @@ -75,6 +76,7 @@ class Mouse_ | |||
| void end(void); | |||
| void click(uint8_t b = MOUSE_LEFT); | |||
| void move(signed char x, signed char y, signed char wheel = 0); | |||
| void moveAbs(uint16_t x, uint16_t y); | |||
There was a problem hiding this comment.
I realize this is a very small point about a large proposed change but for consistency with other parts of the Arduino API and the API style guide (http://arduino.cc/en/Reference/APIStyleGuide), this should be called moveAbsolute(), not moveAbs(). The letters "abs" can mean a lot of different things and, in general, we use complete words in the API.
There was a problem hiding this comment.
On Mon, Feb 10, 2014 at 07:30:03AM -0800, David A. Mellis wrote:
@@ -75,6 +76,7 @@ class Mouse_
void end(void);
void click(uint8_t b = MOUSE_LEFT);
void move(signed char x, signed char y, signed char wheel = 0);
- void moveAbs(uint16_t x, uint16_t y);
I realize this is a very small point about a large proposed change but for consistency with other parts of the Arduino API and the API style guide (http://arduino.cc/en/Reference/APIStyleGuide), this should be called moveAbsolute(), not moveAbs(). The letters "abs" can mean a lot of different things and, in general, we use complete words in the API.
I like consistency. Thank you. I'll update this in just a moment.
|
@cmaglie, I started down the road of adding new functions to main.cpp to try and work around the problem of being unable to pass preprocessor macros to the Arduino core. It seemed like an ugly work around, so I tried to fix the 'actual' problem. The patch below looks in a sketch's directory for a directory called "core_include" - If it exists, it's added as the first entry in the include search path. Additionally, Arduino.h now has hooks to load two new include files <before_arduino_h.h> and <after_arduino_h.h>. Dummy files for each of those in the core make sure that nobody will get a "file not found" error. With this infrastructure, users can now #define things from userspace programs and have the results actually do something useful in the core. If this idea/implementation is workable, I believe that I need to do at least the following before submitting a pull request
Does this seem like a valid/valuable addition to the Arduino core? Is my implementation sane? What else would you like to see in a pull request? Thanks! diff --git a/app/src/processing/app/debug/Compiler.java b/app/src/processing/app/debug/Compiler.java
index 5bdb7d2..577d3bd 100644
--- a/app/src/processing/app/debug/Compiler.java
+++ b/app/src/processing/app/debug/Compiler.java
@@ -89,6 +89,11 @@ public class Compiler implements MessageConsumer {
// 0. include paths for core + all libraries
sketch.setCompilingProgress(20);
List<String> includePaths = new ArrayList<String>();
+
+ File coreIncludeOverrides = new File(sketch.getFolder(), "core_include");
+ if (coreIncludeOverrides.isDirectory()) {
+ includePaths.add(coreIncludeOverrides.getAbsolutePath());
+ }
includePaths.add(prefs.get("build.core.path"));
if (prefs.get("build.variant.path").length() != 0)
includePaths.add(prefs.get("build.variant.path"));
@@ -666,8 +671,13 @@ public class Compiler implements MessageConsumer {
String corePath = prefs.get("build.core.path");
String variantPath = prefs.get("build.variant.path");
String buildPath = prefs.get("build.path");
-
+
List<String> includePaths = new ArrayList<String>();
+
+ File coreIncludeOverrides = new File(sketch.getFolder(), "core_include");
+ if (coreIncludeOverrides.isDirectory()) {
+ includePaths.add(coreIncludeOverrides.getAbsolutePath());
+ }
includePaths.add(corePath); // include core path only
if (variantPath.length() != 0)
includePaths.add(variantPath);
diff --git a/hardware/arduino/avr/cores/arduino/Arduino.h b/hardware/arduino/avr/cores/arduino/Arduino.h
index 7bf5119..f51f560 100644
--- a/hardware/arduino/avr/cores/arduino/Arduino.h
+++ b/hardware/arduino/avr/cores/arduino/Arduino.h
@@ -1,6 +1,7 @@
#ifndef Arduino_h
#define Arduino_h
+#include <before_arduino_h.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>
@@ -217,4 +218,5 @@ long map(long, long, long, long, long);
#include "pins_arduino.h"
+#include <after_arduino_h.h>
#endif |
|
@obra, an approach like this seems ok to me. I don't think it's the most reliable or elegant solution, but at least it is simple and powerful. Also see #1734, which has code to do pretty much the same thing for libraries. Ideally, we'd have a single solution (or at least a single convention) that works for both. Regarding your actual patch, if you indent it by a few spaces, github Markdown will show it as a code block, it's pretty unreadable as it is now... |
|
The idea of having a specially-named .h file in the sketch be included during compilation of the core and libraries is a long-standing one: https://code.google.com/p/arduino/issues/detail?id=27 It may or may not be a good idea (I've never quite been convinced it's worth it) but it is, I think, a big change and one that should be considered carefully on the developers list. This would open up lots of potential tweaks to the core and libraries, greatly increasing the effective API of those files (i.e. previously-internal details of the files that people will now be able to rely on and therefore will become more difficult to change). It will also add a lot of potential complexity / features that people may see in sketches they get from others. In addition, by not having a way to provide pre-processor macros / compile-time constants to the core and libraries, I think we've forced people to find easier-to-use alternatives in the form of run-time configuration (which is more consistent with other parts of the Arduino API). With this option, I worry that people will instead take the path-of-least-resistant and use compile-time configuration, complicating things for users. In any case, I'd encourage a thoughtful discussion of this feature before adding it and it has far-reaching implications. |
|
David, Thanks very much for the feedback. I'll bring it up on the list. Do you have thoughts about other ways to solve the problem I'm specifically trying to address for the code in this pull request? USB HID descriptors can take up a relatively large amount of space in a compiled sketch and it's pretty clear that the new features in this pull request can't go into the core until and unless there's a way to conditionally compile them. |
|
On 19 February 2014 05:55, Jesse Vincent notifications@github.com wrote:
Am I correct in thinking that the issue is that this extra space is If so, then we need the code/descriptors written in such a way that One thought I had was in relation to something that I saw when working --Philip; |
Yes.
Pointers to documentation? I'm a clueless newbie on this stuff.
The problem is that we're taking the space hit just by compiling the descriptors in and storing them in PROGMEM . We need to do something at compile time to not compile them in. And, at least as of now, the USB connection happens before any user code is run. |
|
On 19 February 2014 10:16, Jesse Vincent notifications@github.com wrote:
http://embeddedfreak.wordpress.com/2009/02/10/removing-unused-functionsdead-codes-with-gccgnu-ld/ Also, apparently the GCC term is "weak symbols" rather than weakrefs: http://www.embedded-bits.co.uk/2008/gcc-weak-symbols/ Using this approach would presumably require having a "default"
I'm kinda vague and hand-wavy on all of this because I've only ever Good luck! --Philip; |
Note that there's really two types of weak symbols (not sure about exact I suspect that a similar approach to what I added for HardwareSerial #1711 (comment) I'm not entirely sure what the problem is you're trying to solve, but A solution for this could look like the following. USBCore.cpp contains a main init function, that weakly calls Then, there is an USBKeyboard.cpp, which contains the descriptor, Now, when a sketch does not use any USB keyboard functions, the only This approach needs an Note that this assumes that all descriptors have the same type and can Also, note that the above assumes that stuff will be loaded from PROGMEM In this case, it would be better to let the USBKeyboard.cpp statically Finally, I'm wondering if you can do something like this in USBCore.cpp: And have the compiler sort everything out automatically, either putting I'm not entirely sure if this will do what I hope it does, but it looks http://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Variable-Attributes.html I hope this helps you figure out something cool and efficient! :-) |
|
On 20 February 2014 01:19, Matthijs Kooijman notifications@github.com wrote:
--Philip; |
|
Wow. Thank you so much for that explanation, Matthijs. I suspect that's enough |
I think that'll only work if you put the Keyboard and Mouse support in separate libraries. If you keep it in the core, it'll certainly compile them, but just not include them in the link. However, if you move stuff into external libraries, the weak linking stuff I proposed above might become tricky again. You can still use the same trick with hardcoding symbol names in USBCore that are defined in the library, but that doesn't seem like good form. Moreover, if we want to support third-party libraries also adding extra descriptors etc., then we can't know in advance how all these symbols are named, so we can't use that approach. So, perhaps using the dynamic memory approach with an |
cmaglie
added
Component: USB Device
all code from arduino#1803 included
all code from arduino#1803 included
|
This may be closed now as well. |
|
I'd close this issue as, from 1.6.6, USB-related enhancements can be implemented by libraries. |
This pull request includes the features from pull request 1488 and pull request 1391. I've somewhat overhauled the code to better match the surrounding code and a few features.
I've also added code examples for the less discoverable features.
If this pull request isn't "right" yet, I'd love feedback so I can improve it.