keyboardio
Need a way to default to boot protocol
Currently, KeyboardioHID follows the HID spec, and defaults to report protocol, with an option to go back to boot. We should have a way to default to boot, without having to detach/attach first.
One simplistic way to do this would be to use KEYBOARDIOHID_DEFAULT_KEYBOARD_REPORT_PROTOCOL for the default, and it would be set to HID_REPORT_PROTOCOL if unset. This would allow vendors to set a default in boards.txt, for example.
Another option, which I personally like better, would be to not instantiate BootKeyboard from within KeyboardioHID, but leave it up to the device plugins. This would allow us to have a constructor that takes a default_protocol argument, and each device could use whichever they want.
Even better, we could turn KeyboardioHID into kaleidoscope::driver::hid::keyboardio::, in which case we'd be able to make the whole HID facade nicer, move it all to the Kaleidoscope repo, and allow an even easier choice for devices: in Props! The default ::hid::keyboardio::BaseProps would default to report, but boards could easily override that.
@obra, what's your take on an attempt to try and turn this into a HID driver?
For summary, as far as I see, we have the following options to have a device that defaults to boot protocol rather than report:
- Switch protocols in
setup(). This requires no changes to KeyboardioHID, and can be achieved within the firmware sketch alone. The downside is that this requires a detach-attach dance on every connect. It's also an ugly workaround. - Use an optional
KEYBOARDIOHID_DEFAULT_KEYBOARD_REPORT_PROTOCOLmacro that boards can define inboards.txt, and would default to report if unspecified. This is simple to implement, doesn't require existing device plugins to change, but it's kind of a hack. It also makes it harder for the end-user change the default if need be. - We could stop instantiating
BootKeyboardfromKeyboardioHID, and leave it up to the device plugins. This would delegate it to the right place (device props), would allow changing the default in a reasonably easy way. But still feels like a hack, especially because we instantiate the rest of the stuff. - We could merge
KeyboardioHIDintokaleidoscope::driver::hid::keyboardio::*. Like the previous option, this would allow one to select the default via Props, but everything would be instantiated there, not justBootKeyboard.
For the very short term, I can go with option 1 for the Raise firmware (because I'd like to not fork KeyboardioHID for option 2 or 3). In the long run, I think option 4 is the best.
I am fine with doing #4 ᐧ
On Mon, Nov 25, 2019 at 8:55 AM Gergely Nagy [email protected] wrote:
For summary, as far as I see, we have the following options to have a device that defaults to boot protocol rather than report:
- Switch protocols in setup(). This requires no changes to KeyboardioHID, and can be achieved within the firmware sketch alone. The downside is that this requires a detach-attach dance on every connect. It's also an ugly workaround.
- Use an optional KEYBOARDIOHID_DEFAULT_KEYBOARD_REPORT_PROTOCOL macro that boards can define in boards.txt, and would default to report if unspecified. This is simple to implement, doesn't require existing device plugins to change, but it's kind of a hack. It also makes it harder for the end-user change the default if need be.
- We could stop instantiating BootKeyboard from KeyboardioHID, and leave it up to the device plugins. This would delegate it to the right place (device props), would allow changing the default in a reasonably easy way. But still feels like a hack, especially because we instantiate the rest of the stuff.
- We could merge KeyboardioHID into kaleidoscope::driver::hid::keyboardio::*. Like the previous option, this would allow one to select the default via Props, but everything would be instantiated there, not just BootKeyboard.
For the very short term, I can go with option 1 for the Raise firmware (because I'd like to not fork KeyboardioHID for option 2 or 3). In the long run, I think option 4 is the best.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/KeyboardioHID/issues/59?email_source=notifications&email_token=AAALC2HFGNVVDPJUUG5EKETQVP7PZA5CNFSM4JRID7L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFDCF6Q#issuecomment-558244602, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2HKI2R4X3M26MLIQMTQVP7PZANCNFSM4JRID7LQ .
I made some progress on this: Kaleidoscope@driver/hid & KeyboardioHID@hid-driver. It works, and a lot of things are simpler to do now.
The downside is that Mouse and AbsoluteMouse never get optimized out by the linker. They don't get dropped, because the linker sees more now, and sees possible references to them, while it did not before, when they were in the adaptor library. If I use the no-op mouse/absolutemouse drivers, then we're a few bytes PROGMEM and RAM better than the status quo.
The solution here is to have the ::driver::hid::Base base class in Kaleidoscope, but ::driver::hid::Keyboardio in KeyboardioHID, on top of the stuff we already have there. This should allow the linker to do the right thing. At least, I think that is the solution. Will need to actually test it.
In any case, with the new system, a board can easily set their default protocol in their setup(), so all is well! I can probably make it so that we set the protocol in Props. I'm kinda liking what we have so far, though it does need considerable amounts of cleanup.
If the idea works, we can get rid of Kaleidoscope-HIDAdaptor-KeyboardioHID, and will only need a HID library like KeyboardioHID along with Kaleidoscope for a minimal bundle.
Sadly, just moving ::driver::hid::Keyboardio to KeyboardioHID did not help.
They don't get dropped, because the linker sees more now, and sees possible references to them, while it did not before, when they were in the adaptor library.
That must be due to the fact that the symbols were moved to another compilation unit. Mind you, what linkers do is highly dependent on the order of object files being passed at the linker command line.
Maybe you could get back to the old behavior by somehow forcing the linker command line. Not sure, though, what rules the order depend upon, maybe alphabetical, maybe something else. But it might be worth experimenting with by intercepting the linker command line with VERBOSE=1 and then experimenting a bit.
That must be due to the fact that the symbols were moved to another compilation unit.
...and this is probably it. With the new setup, Mouse, Keyboard, BootKeyboard & friends do not get instantiated by KeyboardioHID, but by Kaleidoscope. Where the classes live is then irrelevant, unless I can move instantiation back to KeyboardioHID.
Thanks, I have a new route to explore now!
Having an xWrapper class that just wraps the global object provided by KeyboardioHID helps, and allows the linker to optimize out the Mouse/AbsoluteMouse objects as needed. With that, we're at +56 PROGMEM and +87 RAM, which is much better than before, but still more than I'm happy with.
And with a few more tricks: -42 PROGMEM, -15 RAM, with functional equivalence, and 0 changes to KeyboardioHID, and with Kaleidoscope-HIDAdaptor-KeyboardioHID dropped.
Hmmhmm. The above numbers were for the Basic sketch. For Devices/Keyboardio/Model01, we're at +386/+11, which is a tad too much. Looking into it.
Most examples saw -500/-29 PROGMEM/DATA, with only a few seeing an increase in size:
Model01: +386/+11Planck: +256/-34Atreus: +254/-34Atreus2: +708/+6
The Atreus2 is most interesting.
make size-map will likely be instructive
On Nov 28, 2019, at 9:59 AM, Gergely Nagy [email protected] wrote:
Most examples saw -500/-29 PROGMEM/DATA, with only a few seeing an increase in size:
Model01: +386/+11 Planck: +256/-34 Atreus: +254/-34 Atreus2: +708/+6 The Atreus2 is most interesting.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
I'm using elf_diff, even more instructive. I now see which functions increased in size considerably, but couldn't figure out why yet.
FWIW, the Atreus2 case looks something like this:
HID_::setup(): +136HID_::getDescriptor(): +134HID_::getInterface(): +130MouseKeys::onKeyswitchEvent(): +104HID_::sendReport(): +36MouseWrapper::begin(): +24HID_::HID_(): +16
The rest are smaller, and make reasonable sense. These... these feel like we might be initializing something more than once. Or perhaps AbsoluteMouse not getting optimized out... At least in some of the cases. The Planck example, for example, doesn't use MouseKeys.
Looking at the disassembly, we have two copies of many keyboard stuff. Now to find out where we do that!
struct KeyboardioProps: public BaseProps {
typedef keyboardio::KeyboardProps KeyboardProps;
typedef keyboardio::Keyboard<KeyboardProps> Keyboard;
typedef keyboardio::MouseProps MouseProps;
typedef keyboardio::Mouse<MouseProps> Mouse;
typedef keyboardio::AbsoluteMouseProps AbsoluteMouseProps;
typedef keyboardio::AbsoluteMouse<AbsoluteMouseProps> AbsoluteMouse;
};
If I drop the Keyboard typedef, then we do not have duplication. But then we don't have a single instance of Keyboard_ either. So it is something around that place that's fishy.
I give up. I have no clue why we have two constructor calls, or why the other functions grow so much.
I'll keep the branches around, and may revisit it some other time, but for now, I surrender. Thankfully, we can set the default protocol on a per-board basis, we just have to set BootKeyboard.default_protocol and call BootKeyboard.setProtocol() before Kaleidoscope.setup().
Could you possibly make the elf_diff stuff available? I could take a glimpse and maybe I have an idea.
@algernon, I think the reasons for your troubles is that the recently I introduced stuff in Kaleidoscope/src/kaleidoscope/device/virtual/ in the course of working on the virtual builds. @obra cherry picked that stuff recently to master.
Unfortunately not all files in Kaleidoscope/src/kaleidoscope/device/virtual/ are properly protected by #ifdef KALEIDOSCOPE_VIRTUAL_BUILD, including Kaleidoscope/src/kaleidoscope/device/virtual/HID.cpp. That file contains a no-op HID implementation that is accidentlally also compiled in the non-virtual builds. I guess that due to link order dependencies its symbols are sometimes caught up and used instead of the actual HID implementation. I totally forgot that Arduino is greedy and builds whatever it can find. Also, when testing this before submitting the virtual build PR, that problem did not show up :(
That's why elf_diff shows the strange increase in symbol sizes. It compares the virtual HID symbols with the non-virtual ones.
I think that issue is somewhat urgent as it potentially can make the entire firmware dysfunctional. Apologies for not being able to fix this within the next two days. Hope you can.
The fix is to add the proper #ifdef KALEIDOSCOPE_VIRTUAL_BUILD clauses to everything that lives in Kaleidoscope/src/kaleidoscope/device/virtual.
I already added an ifdef guard to that HID.cpp, and still saw double Keyboard_ constructors. Though, I have not added guards to the rest of stuff in device/virtual. I'll see if guarding the rest has any effect.
Just checked, only Logging.cpp was missing the ifdef guard, and adding it there made no difference :|
But the elf_diff document you made available definitely showed a comparison with a build that linked the virtual hid implementation.
Damn, I just concentratet on the HID stuff in the elf_diff doc. Cause that's where the big size increase came from. I missed the Hardware thing. Can't chech now on the phone. What do you mean by duplicate. Identical signatures are not accepted by the linker so I suppose there must be a difference.
@algernon, I'm still curious, could you post an elf_diff output of a comparison after you added the ifdef guards in virtual/hid.cpp?
Having rebased my branch on top of master, the Model01 example is substantially better: +64 PROGMEM & +11 RAM compared to master, which is much better than +386/+11, but I'd still like to understand why. There's no virtual hid involved (size-map | grep -i virtual only shows non-virtual thunk to SingleAbsoluteMouse_::sendReport(void*, int)).
Main takeaways from Model01-master-vs-hid-driver.html.txt:
MouseKeys_::onKeyswitchEventgrew 122 bytes for no good reason. Might need somenoinlinelove there?MouseKeys_::onSetup()andMouseKeys_::warp_jumpare also suspiciously bigger for no good reason.sendKeyboardReport()grew from 126 bytes to 178, that's 52 bytes in and of itself!- Looking at the disappeared & new symbol list, there's a suspicious lack of
kaleidoscope::driver::hid::base::Mouseand::AbsoluteMouse.
So it is increasingly looking like things are getting inlined, which is probably good for performance, but bad for size.
Hrm, no. Adding __attribute__((noinline)) anywhere near the mouse stuff increases code size.
I think we can stomach a +32/+11, but will check the rest of the examples to see if there any other outliers.