Skip to content

Keyboard layout improvement #60

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Conversation

bitboy85
Copy link

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
Copy link
Member

tannewt commented Mar 11, 2021

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
Copy link
Author

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
Copy link
Member

tannewt commented Mar 17, 2021

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
Copy link
Author

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
Copy link

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
Copy link
Contributor

Neradoc commented Mar 18, 2021

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
Copy link
Member

tannewt commented Mar 18, 2021

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
Copy link
Author

bitboy85 commented Mar 28, 2021

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
Copy link
Member

tannewt commented Mar 29, 2021

@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
Copy link
Author

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
Copy link
Member

tannewt commented Apr 1, 2021

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
Copy link
Author

bitboy85 commented Apr 1, 2021

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
Copy link
Collaborator

dhalbert commented Apr 1, 2021

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
Copy link

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
Copy link
Member

tannewt commented Jun 4, 2021

@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.

@evaherrada evaherrada changed the base branch from master to main June 7, 2021 16:41
@bitboy85
Copy link
Author

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

@dhalbert
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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

@Neradoc
Copy link
Contributor

Neradoc commented Aug 4, 2021

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
Copy link
Collaborator

dhalbert commented Oct 26, 2021

@dhalbert dhalbert closed this Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants