adafruit / Adafruit_CircuitPython_HID

USB Human Interface Device drivers.
MIT License
373 stars 105 forks source link

Keyboard layout improvement #60

Closed bitboy85 closed 2 years ago

bitboy85 commented 3 years ago

This PR moves the write method to the keyboard class (instead of having a layout class). So its script breaking.

It supports latin-1 charset (255 chars) which is required for layouts in europe. An example for a german keyboard layout is included.

New layouts can easily added.

Known issue: € sign is not part of the latin-1 charset, so i cannot be used inside the write method.

tannewt commented 3 years ago

Thanks @bitboy85! What do you think about moving the functions to a Layout base class instead of into Keyboard? That way both en_us and de_de can subclass it and the API will stay the same for en_us.

bitboy85 commented 3 years ago

I fully understand that we should try to avoid script breaking changes. But i did it this way because i think its the most easy and obvious way for someone who wants to use this feature. Using the keyboard object for single key presses or functional keys like F1 and using the layout object for writing strings is logical from a technical point of view, but somehow confusing from a users point of view.

tannewt commented 3 years ago

I fully understand that we should try to avoid script breaking changes. But i did it this way because i think its the most easy and obvious way for someone who wants to use this feature. Using the keyboard object for single key presses or functional keys like F1 and using the layout object for writing strings is logical from a technical point of view, but somehow confusing from a users point of view.

I disagree with you. Keyboard is a lower-level class that sends keycodes (not characters) and is agnostic to layout. Most folks will want to use the layout class and so I think having a super-class for the Layout API is still my preference.

bitboy85 commented 3 years ago

I think this way makes usage more complicated than necessary. If you have a look at other implementations, all use the same object for single keypresses as for strings. Everyone who has used those before (like me) will be confused by using two different things. https://www.arduino.cc/reference/en/language/functions/usb/keyboard/ https://www.pjrc.com/teensy/td_keyboard.html

AngainorDev commented 3 years ago

Gave a try at refactoring with ancestor and including a french layout. Working but likely to need further work https://github.com/AngainorDev/Adafruit_CircuitPython_HID/tree/layouts/adafruit_hid

Neradoc commented 3 years ago

Hi, I'm glad there are people out there making layouts !

I'm not sure if it is confusing the way you think though. The keyboard is used to send keycodes, which are not translated. So on an AZERTY keyboard we would have this confusion:

kbd.send(Keycode.A) # q
kbd.write("aA")     # aA

Also note that Arduino does not support alternate layouts that I know of (I had to copy and modify the keyboard.h and .cpp for that), and Teensy seems to only support it as a static change at compile time. So there's a difference in usage, but because there's a difference in features.

Not loading the layout by default also allows to reduce the memory footprint if it is not needed, if you are just gonna use F keys for example. It also makes intentional the choice of the layout, associated with loading the appropriate module, which makes more sense to me than having a favored one loaded by default.

Memory reduction is useful here since in my opinion a macro-button-pad is a perfect use for M0 boards like the trinket or QT PY. KeyboardLayoutUS even goes to the effort of packing key codes into 1 byte each by using the higher bit as a flag for shift.

I made a KeyboardLayoutFR a loooong time ago (mac-fr actually), though for lack of time and need I left it incomplete. It used 2 bytes to fit shift and alt, so methods are different from KeyboardLayoutUS, but I could see a base class supporting the common parts, and even adding Keyboard's functions to it, with send, press and release that take a string as input and extract the matching key codes from the layout.

tannewt commented 3 years ago

I think this way makes usage more complicated than necessary. If you have a look at other implementations, all use the same object for single keypresses as for strings. Everyone who has used those before (like me) will be confused by using two different things. https://www.arduino.cc/reference/en/language/functions/usb/keyboard/ https://www.pjrc.com/teensy/td_keyboard.html

Thanks for these links! It's always good to look at other implementations.

These implementations aren't enough to override the way we've had this library for four years though. Changing it would invalidate all existing examples and guides.

I think @Neradoc makes good points too.

So, how about we integrate #61 into this?

bitboy85 commented 3 years ago

Hi, I'm glad there are people out there making layouts !

I'm not sure if it is confusing the way you think though. The keyboard is used to send keycodes, which are not translated. So on an AZERTY keyboard we would have this confusion:

kbd.send(Keycode.A) # q
kbd.write("aA")     # aA

I agree thats confusing using the keycodes library. But this is a result of the USB specifications which rely on a us layout. Thats the reason why i have used raw numbers instead of thoses constants in my provided layouts. Those keycodes does not really represent a character but more of a position on a physical keyboard. So instead of Keycode.A it would be more clear to name it like keycode.row2column2

Also note that Arduino does not support alternate layouts that I know of (I had to copy and modify the keyboard.h and .cpp for that), and Teensy seems to only support it as a static change at compile time. So there's a difference in usage, but because there's a difference in features.

True. It does not support them by default which is sad i think. The method of using 7-bit ascii and one shift bit (which is also used in the current circuit python) does simply not work for layouts which uses latin-1 or 8-bit ascii. So requiring another layout will lead to a rewrite of this code part. To avoid this i have choosen this way. Creating new layouts is quite easy. I guess the reason why teensy does this at compile time is memory usage. Otherwise all keycode -> char tables would be compiled too making it quite large. But using circuit python we have other options. 1. Just copy the layouts you really need to the device. 2. It would be the only implementation where layouts can be swapped at runtime.

Not loading the layout by default also allows to reduce the memory footprint if it is not needed, if you are just gonna use F keys for example. It also makes intentional the choice of the layout, associated with loading the appropriate module, which makes more sense to me than having a favored one loaded by default.

Memory footprint is a point. While doing this i had in mind that bioses and maybe other low level devices refer to an us layout. Not sure how important this is on boards running circuit python.

@tannewt From my point of view #61 cannot be integrated as AngainorDev stated that his implementation is done to be as much compatible ans small as possible while mine is designed to be as easy as possible with the possibility of full character set (at the cost of memory).

tannewt commented 3 years ago

@tannewt From my point of view #61 cannot be integrated as AngainorDev stated that his implementation is done to be as much compatible ans small as possible while mine is designed to be as easy as possible with the possibility of full character set (at the cost of memory).

Sounds like we have different goals. You are welcome to fork and maintain a bitboy85_hid library in the community bundle. My preference for adafruit_hid is to not include layout related functions in Keyboard. Hopefully we can add de_de into the structure @AngainorDev has done.

bitboy85 commented 3 years ago

My goal is to have a lib which is similar to use as on other devices (just because i have known them before) and additionally give users the ability to easy create their own layouts (just because i'm lazy and have no idea what requirement an ukrainian keyboard has). So the design decision was to not have any logic within the layout files. It should be a simple translation table. This is at the cost of memory. As my only device is a Pi Pico, i have checked and the keyboard implementation with layout eats about 12kb or 5% of the available space. So no need to be stingy here.

Meanwhile i created a "standalone" lib which can be copied in its own folder within the lib directory and also included a way to add dead keys. Feel free to copy anything which is helpful. https://github.com/bitboy85/circuitpython_keyboardext

tannewt commented 3 years ago

So the design decision was to not have any logic within the layout files. It should be a simple translation table. This is at the cost of memory. As my only device is a Pi Pico, i have checked and the keyboard implementation with layout eats about 12kb or 5% of the available space. So no need to be stingy here.

Yup, we're agreed on that. That's why we are switching to a shared super class with the logic. The smallest boards we run on are 32kb of RAM so it makes a big difference. The RP2040's 256k RAM is very nice.

bitboy85 commented 3 years ago

I used .py in my test for both, the adafruit_hid as for my keyboardext. Not sure how much memory could be saved if those are converted to mpy.

dhalbert commented 3 years ago

We will continue to freeze the basic library into the Circuit Playground Express and the Trinkey-style devices. For frozen libraries, the library lives in flash, and takes very little RAM. However, it is a tight squeeze these days, so anything that grows the current library significantly will be a problem.

jposada202020 commented 3 years ago

Hello folks, if I am understand this correctly, and correct me if I am wrong, as space is tight we would not like to integrate this PR. However, this is more than welcome in the Community Bundle. If this is indeed the case I proposed to close this PR and move it this PR to the community bundle. Thanks :) @bitboy85 @tannewt feel free to comment.

PD: if help is needed to include this in the community library I could provide it.

tannewt commented 3 years ago

@jposada202020 Ya, you are correct. I think we want bitboy's version in the community bundle and future layouts in their own separate libraries that depend on this one.

bitboy85 commented 3 years ago

Is it moved to the community bundle or do i need to do something?

dhalbert commented 3 years ago

Is it moved to the community bundle or do i need to do something?

No, a new library needs to be created that just contains this layout, and then that library would be added to the community bundle. And #61 needs to be polished off and merged.

bitboy85 commented 3 years ago

Sry, i don't really understand what needs to be done now. From my point of view #61 and my library cannot be merged because of different design principles (but of course its possible to create a french layout)

@jposada202020 Feel free to create the PR for the community bundle

dhalbert commented 3 years ago

Yes, sorry, they need to be reconciled. I have not proceeded due to the incompatibilities.

Neradoc commented 3 years ago

FYI I'm in the process of building up a keyboard layouts repository/bundle, to make it easier to find and download keyboard layouts independently, while not requiring to create a new repository for each layout. It uses a modified KeyboardLayout class from PR 61.

I converted and added your German keyboard layout to it. I haven't tested it with a German keyboard.

https://github.com/Neradoc/Circuitpython_Keyboard_Layouts

dhalbert commented 2 years ago