Need a way to default to boot protocol

[algernon]

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.

[algernon]

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.

[algernon]

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?

[algernon]

For summary, as far as I see, we have the following options to have a device that defaults to boot protocol rather than report:

1. 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.
2. 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.
3. 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.
4. 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.

[obra]

I am fine with doing #4 ᐧ

…

On Mon, Nov 25, 2019 at 8:55 AM Gergely Nagy ***@***.***> 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: 1. 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. 2. 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. 3. 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. 4. 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 <#59?email_source=notifications&email_token=AAALC2HFGNVVDPJUUG5EKETQVP7PZA5CNFSM4JRID7L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFDCF6Q#issuecomment-558244602>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALC2HKI2R4X3M26MLIQMTQVP7PZANCNFSM4JRID7LQ> .

[algernon]

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.

[algernon]

Sadly, just moving ::driver::hid::Keyboardio to KeyboardioHID did not help.

[noseglasses]

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.

[algernon]

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!

[algernon]

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.

[algernon]

And with a few more tricks: -42 PROGMEM, -15 RAM, with functional equivalence, and 0 changes to KeyboardioHID, and with Kaleidoscope-HIDAdaptor-KeyboardioHID dropped.

[algernon]

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.

[algernon]

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.

[obra]

make size-map will likely be instructive

…

On Nov 28, 2019, at 9:59 AM, Gergely Nagy ***@***.***> 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.

[algernon]

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:

1. HID_::setup(): +136
2. HID_::getDescriptor(): +134
3. HID_::getInterface(): +130
4. MouseKeys::onKeyswitchEvent(): +104
5. HID_::sendReport(): +36
6. MouseWrapper::begin(): +24
7. HID_::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.