Refactor to support extra layouts

[AngainorDev]

Working, but I don't think this is ready to merge yet.
Opening a PR for discussion.

Common KeyboardLayout class, US and FR layouts so far.

I opted to keep the main packed table with SHIFT modifier encoded as first bit, for space reasons.

Delaing with accentuated chars means

• need for more than just ascii (sparse list, hard to pack)
• extra modifier (altgr), needing an extra bit if encoded as the SHIFT
  The result is a very hybrid approach that may be space efficient but not that elegant.

I used a function to deal with the rare above ascii chars, and a string to list the very few chars needing the ALTGR modifier.
So, that's kinda 3 different methods at once, for space savings sake.

I'd like to clean that up, but not at the expanse of space.

[tannewt]

This looks good to me! Thanks! Is there more you want to do?

[tannewt]

@kattni This triggers duplicate-code too. :-( I don't think it makes sense to deduplicate it. Do we have a way to disable it locally? I wish pylint-dev/pylint#214 was implemented.

[dhalbert]

Space is very tight on the CPX and other boards where this library is frozen in. We might need to split off other layouts into separate libraries.

[AngainorDev]

@tannewt I'd like to fully test the FR layout and add a few extra accentuated chars before release, then I'll try to move on German, at least a first prototype.

With small flash boards in mind, would it make sense to only have the ancestor layout class included in the base lib, then an extra lib per layout?
Would free at least the 128 bytes of the map for all multimedia pads, that do not need to write text.

[tannewt]

@AngainorDev Ya, many files is the best way to minimize RAM cost.

[nico7885]

Hello,

(sorry for bad english translated)

I am a French user, I have been working on the Fr keyboard for a while now, which is why I allow myself a few comments on this project.

To have the French keyboard, you must have a table of 255 characters, the first 6 bits will identify the key and the last 2 bits for the modifiers (normal 0x00, shift 0x80, altgr 0x40), there is only one problem for the key 0x64 (Europe key () that will have to be renamed to 0x03 for example.

So it is not necessary to treat Altgr separately, you can include it in the table.

This is a start, but it is not enough, you need to double the bytes to process the dead keys, i.e. a total of 512 bytes but that could be reduced by starting the array at the value 32 and doubling the bytes starting from the value 128 (is to be studied) for example, which would give an array of (128 * 2) + 96 = 352 bytes

The good news is that it will be easy to implement an Fr Mac version, all useful bridge modifiers will be in the array (normal, shift, alt and shift + alt)

It will then be easy to add the altcodes to have special features for windows, to print characters independently of the keyboard and unicode characters.

I did this in c ++ for Arduino Leonardo and Adafruit QT PY (tiny usb) boards, you can see here:
https://forum.arduino.cc/index.php?topic=718926.0

If the implementation is not complete for keyboards, it will force us to modify the table and associated functions anyway.

I made a Fr translation and tested on the raspberry pi pico with an array of 512 bytes (no problem on the Pico), the file takes up space because the array, also I added some functions like the altcodes for windows

[AngainorDev]

Thanks a lot for the feedback @nico7885, much appreciated.

There are different angles I think:

• support a full and consistent mapping for FR and PICO only (could be a targeted hid lib)
• design a common base for many mappings that can be used on many CP compatible boards (adafruit lib)

With that in mind, I tried to

• keep most of the things compatible between US and FR mappings, including extra bit encoding fior shift only.
• keep the used flash space as low as possible, to keep compatibility with smaller boards

As for encoding altgr with 7th bit 0x40, is this compatible with US mapping? German one? Other iso charsets?
If it is - or just needs a few exceptions, then this can be factorized in the ancestor class and much cleaner, agreed.
If not, or if there are too many exceptions, I think the small altgr string may still be a generic solutions, with low space overhead.
Sometimes, when you aim for efficiency or code size, you may have to denormalize a few things.
(Not saying this is the case here, need to be dug)

Same I'd say for dead keys or chars requiring 2 scan codes, like ï for instance, needing 2 key strokes on the keyboard..
Either you encode all the keys as tuples and bloat space, or there are not many and it's more efficient to deal with them in a specific way.

Not pretending to have a definitive answer there, I'm experimenting.
But I think this precise lib added value is to have a consistent interface no matter the mapping or board used.

Media keys are something I did not consider at all for the moment.

[nico7885]

I think there is no problem, only two keys that change 0x32(no problem) and 0x64 (need to remap)
see the keyboard image here at the bottom of the page:
https://github.com/qlyoung/armory-keyboard

Am I making an error where the array is stored in the flash memory of the card currently?
"Micropython / CircuitPython will store the resulting bytes constant in flash memory"

In general, the choice to make a keyboard will limit the number of external library because it is a specific use.

I would like to point out that the ability to add the altgr in the array is only valid for Circuitpython and is not true for the Arduino lib, due to their processing procedure.

I am providing you with the library that I use with the Raspberry pi Pico that I tested with the Pimonori RGB Keypad test board, it may be useful to you or give you ideas.

it is not final, this is my first code in python, please be indulgent. In any case, it was easier and faster than I would have thought.

Functions for displaying text:
-> To display text only with the characters available on the French keyboard
layoutFR.write ("Your text")
layoutFR.print ("Your text") -> identical to layoutFR.write

-> To display text with all possible characters in Windows
(will use alt-codes internally if the character is not available on the French keyboard)
layoutFR.printWin ("Your text")

-> To display text with all the possible characters but independently of the keyboard under Windows
therefore will work regardless of the language chosen under Windows
unicode characters may not work in all applications
but those corresponding to the extended ASCII offer good compatibility
(all characters will therefore be emulated with the alt-codes)
layoutFR.altCodeWin ("Your text")
https://nicosoft.weebly.com/uploads/4/3/4/0/4340015/circuitpython_keyboard_fr.zip

[kattni]

@tannewt We could make another exception and increase the min-similarity-lines threshold, though I'm not entirely keen on that. I would prefer we sort out deduplicating it. I reran the job, it's only failing on one section now instead of several.

[AngainorDev]

Checked the dup portion:
In the precise current case, It's normal to have duplicated code there: the byte map has similar portions between the US and FR mappings. I can't see how to really dedup without loosing the size optimization.

As a workaround, would adding a different comment on these "dup" lines help pass the CI?
Like including # FR or # US for every similar bytes. Would bloat the PY source code with no impact on MPY.

[kattni]

@AngainorDev Unfortunately, we have it set to ignore comments when checking for duplicated code, so in this case, that wouldn't work. If it is unreasonable to change the duplicated code in this specific case, then we will increase the min-similarity-lines threshold.

Are you running Pylint locally? Does it fail the same way for you locally?

[AngainorDev]

Not running pylint locally, but I can see why it gives that warning.
There are in fact dup code lines, and I don't see how we can avoid that without dirty hacks that would end up more troublesome than a CI warning.

I can also see the reason for that dup test.
It's really tied to the specific nature of that map, 128 contiguous bytes, with common portions that need to be in different files.

[kattni]

@AngainorDev I was asking if you were running it locally because I was going to have you test to see to what we would need to increase the threshold to avoid the duplication issue, but if you're not running it locally, that would take ages. I'll sort out getting it done. Please be patient while I sort out someone to test it or do it myself in the next couple of days. Thanks!

[kattni]

Ok @FoamyGuy is going to see to what we need to change the threshold to get this passing. Thanks!

[FoamyGuy]

@kattni 34 was the magic number here. I've made a new commit that changes the threshold to that. However there are two remaining issues to handle.

1. My commit caused there to be merge conflicts with master. Likely due to the other pending changes to the duplicate threshold. I can come back a little while later today and merge master into this branch and push again. I think it should resolve the merge conflicts.

2. There is a single remaining item that pylint is flagging:

************* Module adafruit_hid.keyboard_layout
adafruit_hid/keyboard_layout.py:92:4: R0201: Method could be a function (no-self-use)

I am unsure of the proper fix for this one. Perhaps make it into a static method, or move it outside of the class into it's own individual helper function? If there is a preferred way to deal with it let me know and I can take care of it as well when I come back to merge master to this and resolve the conflicts.

[dhalbert]

2. There is a single remaining item that pylint is flagging:

************* Module adafruit_hid.keyboard_layout
adafruit_hid/keyboard_layout.py:92:4: R0201: Method could be a function (no-self-use)

I am unsure of the proper fix for this one. Perhaps make it into a static method, or move it outside of the class into it's own individual helper function? If there is a preferred way to deal with it let me know and I can take care of it as well when I come back to merge master to this and resolve the conflicts.

It could be made a @staticmethod, removing the self arg, but that would need to be done here and in the subclasses. It's possible that a future class might want to use the self arg for some reason (so it should just be ignored), but perhaps we can cross that bridge when we get to it.

[AngainorDev]

Agreed with @dhalbert, @staticmethod can do for the moment.

I'll likely refactor again with @nico7885 hints later on anyway.

[tannewt]

(sorry for bad english translated)

Thanks Nico! Feel free to reply in French (or both) and either us English speakers can use Google Translate or the bilingual folks can help bridge the conversation back to us.

[FoamyGuy]

This is updated now with a large enough duplication threshold, and has had master branch merged in to resolve the conflicts.

I've added the pylint ignore as well to make this passing. I'm not confident that I understand it's usage enough to successfully swap it to a static method after looking into it a bit further. And I'm not sure how to test it afterward if I did try. Feel free to add further commits to change that.

Thanks for all of your work on this @AngainorDev and @nico7885. This is great stuff!

[nico7885]

@tannewt,
I also use Google Translate, but if it's important, I will answer in French and in translated English, thank you, I adopt your remarks.

French:
La création de raccourcis est aussi importante que la conversion de caractères en touche, pour l'édition de vidéos (avec la popularité croissante d'OBS par exemple), on voit de plus en plus de projets sur Kickstarter pour des claviers personnalisés .

Les exemples fournis pour les raccourcis ne fonctionneront pas avec les autres langues, envisagez vous des solutions (mettre à jour les définitions des touches, créer une fonction dédiée ou autres choses?)

Google Translate:
Creating shortcuts is as important as converting characters to keys, for editing videos (with the growing popularity of OBS for example), we see more and more projects on Kickstarter for custom keyboards.

The examples provided for shortcuts will not work with other languages, do you see any solutions (update key definitions, create ** a dedicated function ** or other things?)

[AngainorDev]

@nico7885 Pourrais-tu donner un exemple concret de raccourcis et de comment ça se traduit coté scancode ?
J'ai du mal à comprendre si c'est directement lié au layout ou si c'est une fonctionnalité différente.

[nico7885]

Bonjour AngainorDev,
Mise en situation->par exemple pour copier du texte, je vais faire kbd.send(Keycode.CONTROL, Keycode.A), sauf que pour nous Keycode.A = Keycode.Q

Comment ça se traduit coté scancode:
Vous n'avez pas à vous préoccuper du scancode puisque vous avez la fonction keycodes(self, char) dans la classe KeyboardLayoutUS, donc pour le moment, je fais un press de: Keycode.CONTROL avec layoutFR.keycodes('a'), je joue à la fois avec la définition des Keycodes et la conversion caractère vers keycode. Si vous mettez en place une procédure, cela rendra les choses beaucoup plus simple.

[AngainorDev]

Ok.
En gros si j'ai bien compris, tu voudrais une manière d'encoder les raccourcis comme ctrl-a sous forme de chaine par exemple, pour pouvoir les "rejouer" sans avoir à traduire les scancodes auparavant ?
De ce que j'ai compris, un des objectifs essentiel de cette librairie HID est de rester très light car elle est utilisée sur toutes les board CP, yc des boards avec très très peu de flash et de ram.

Comment tu verrais cet encodage des raccourcis ? Il existe des formats déjà définis ?

[Neradoc]

Le soucis est que pour faire ctrl-A indépendamment du layout on doit faire par exemple:

# ctrl-A:
kbd.press(Keycode.CONTROL)
kbd.press(*layout.keycodes('a'))
kbd.release_all()

Il serait utile de convertir les keycodes et pouvoir faire quelque chose comme layout.press(Keycode.CONTROL,Keycode.A)qui serait converti en kbd.send(Keycode.CONTROL, Keycode.A), mais ça nécessiterait une nouvelle table ou des trucs compliqués.

Comme compromis j'avais pensé dupliquer les fonctions de Keyboard avec quelque chose dans ce genre là (non testé)

class KeyboardLayout:
    def press(self, *keycodes):
        for keycode in keycodes:
            if isinstance(keycode,str):
                self.keyboard.press(*self.keycodes(keycode))
            else:
                self.keyboard.press(keycode)

    def release(self, *keycodes):
        for keycode in keycodes:
            if isinstance(keycode,str):
                self.keyboard.release(*self.keycodes(keycode))
            else:
                self.keyboard.release(keycode)

    def send(self, *keycodes):
        self.press(*keycodes)
        self.keyboard.release_all()

Permettant:

layout.send(Keycode.CONTROL,"a")

[nico7885]

@Neradoc , c'est exactement l'idée, c'est un ajout non négligeable même si la façon de le faire peut en dérouter certains; cela dit même avec une redéfinition des keycodes, cela n'aurait pas été plus simple pour les caractères en dehors de A-Z sans consulter la table.

[AngainorDev]

Ok, je vois mieux, merci pour l'exemple !

Je pensais à un encodage spécifique sous forme de chaine pour aussi pouvoir définir ces raccourcis en dehors du code, par l'utilisateur.
ex bidon "_ctrl_a" et "_ctrl__shift_a" qui seraient parsés pour obtenir la liste des scancodes à envoyer.
Ca peut toujours se faire sans, touche par touche, mais ça complique l'UI.

Pour rebondir sur l'exemple de @Neradoc, je dirais qu'on peut partir sur l'interface qu'il propose, avec un petit tweak.
layout.send - dans la classe ancetre - pourrait donc soit accepter une chaine comme actuellement, soit une liste
layout.send(Keycode.CONTROL,"a")
Chaque élément de la liste étant soit un keycode (int) soit une chaine.

Pour gérer aussi des raccourcis plus tordus en une fois, je suggère qu'on ajoute la chaine vide comme indicateur de "release_all".
ça permettrait de faire donc le classique
layout.send(Keycode.CONTROL,"a")
mais aussi de différentier
layout.send(Keycode.CONTROL,"ad")
(ctrl puis a puis d sans relacher ctrl entre deux)
de
layout.send(Keycode.CONTROL,"a", "", d)
(ctrl-a, relache ctrl et a, puis d)
ou encore
layout.send(Keycode.CONTROL,"a", "", Keycode.CONTROL,"c")
ctrl-a suivi de ctrl-c
et des macros plus complexes comme "fin de ligne / insérer , / shift entrée / flèche bas / espace espace espace"

@tannewt @dhalbert I can translate should Google not be precise enough

[dhalbert]

Je comprends assez bien le français, mai pardonnez svp tous mes petites fautes. Il y a longtemps...

De ce que j'ai compris, un des objectifs essentiel de cette librairie HID est de rester très light car elle est utilisée sur toutes les board CP, yc des boards avec très très peu de flash et de ram.

Pour ça, nous pouvons créer une autre librarie "add-on" for chaque autre langue.

Pour les raccourcis (nouveau mot vocabulaire pour moi 🙂 ), suffit-il de créer une classe KeycodeFR où les noms des keycodes correspondent à les étiquettes sur les touches? Par example,

Keycode.Q == KeycodeFR.A
Keycode.W == KeycodeFR.Z

etc.

layout.send(Keycode.CONTROL,"ad")

Cette idée est intigrante, bien qu'elle ajoute à la taille du code dans la librarie de base.

[nico7885]

AngainorDev,
"Je pensais à un encodage spécifique sous forme de chaine pour aussi pouvoir définir ces raccourcis en dehors du code, par l'utilisateur."
Eh oui, c'est tout l'intérêt de disposer d'un lecteur USB.

Quelque soit la solution adoptée, @Neradoc, @AngainorDev ou de @dhalbert; c'est une bonne nouvelle, si vous décidez de le mettre en place.

Sinon si je peux me permettre une interrogation, quel est la problématique qui empêche d'avoir les notifications leds (caps lock, num lock, scoll lock etc...) avec circuitpython sachant qu'elle est basée sur tinyusb qui le gère parfaitement, est ce que cela pourrait être intégré dans le futur éventuellement?

[dhalbert]

Sinon si je peux me permettre une interrogation, quel est la problématique qui empêche d'avoir les notifications leds (caps lock, num lock, scoll lock etc...) avec circuitpython sachant qu'elle est basée sur tinyusb qui le gère parfaitement, est ce que cela pourrait être intégré dans le futur éventuellement?

Did you see this recent merged PR? #62

[nico7885]

@dhalbert,
Ok, thank you very much, good news.

[tannewt]

Is this ready to merge? I didn't follow the last discussion.

[dhalbert]

Is this ready to merge? I didn't follow the last discussion.

I think this is left to do or discuss:

1. The current PR adds KeyboardLayoutFR to this library. This will increase the size of the library and make it less likely to fit as a frozen module in CPX, etc. Instead, I think we should factor out the French-specifiic part into a new library, such as Adafruit_HID_KeyboardLayoutFR, as part of the community bundle, or it can be an Adafruit library (I am willing to "sponsor" it into our repos). The superclass idea is good and is a starting point for other such libraries.
2. Consider also my idea of KeycodeFR as an addition to the new library, to map French keyboard keycap names to the proper keycodes, in a way that is natural to someone looking at a French keyboard.
3. The idea of layout.send(Keycode.CONTROL,"ad") is interesting but also increases the size of the basic library

The USB spec is chauvinistically US-centric, unfortunately. I would have liked to see them just label the keys in rows in some neutral way, but they didn't do that.

[AngainorDev]

@dhalbert

1. This is an Adafruit decision, you only have the overview to tell I think.
   There could also be a "light" US only hid lib for small boards, and a more complete one with extra features for more recent boards with more flash.
   I don't know how many different possible layouts we may end up with, so a lib per child layout could be required in all cases.

Third option, unsure this makes sense in your philosophy of easing things up for the user:
Have a code generator for a custom lib:

• layouts defined as json
• template code
• user defines the layout he needs (json file, can be customized)
• user tells what features he needs (shortcuts, consumer, numpad leds...)
• the minimal code for the lib is generated, to cope with small flash boards.

2. I don't think we need an extra class or map for that. We already have layout.keycodes('a') that will provide Keycode.Q with a French layout, so this is covered already, unless I missed the point.

3. I think I can add that shortcut/macro handling with very few extra lines in the ancestor code.

As for merging, I'd like to fully explore the altgr encoding as 7th bit by @nico7885, that will remove the need for the "altgr" list, and likely give a try at the shortcut things as well.
If this is short enough to go into the core ancestor lib, as well do it now than refactor in a few weeks.

[jposada202020]

@AngainorDev have you worked in the factor as previously mentioned in your comments?
just trying to keep the discussion moving.. thanks :)

[Neradoc]

So, I am using a modified version of the KeyboardLayout class in my layouts bundle, I'm not sure how to propose the changes on a PR, I think they are a little too spread out to use the review system. It's over there: https://github.com/Neradoc/Circuitpython_Keyboard_Layouts/blob/main/libraries/keyboard_layout.py

• removed the dependency to Keycodes, it's only used for shift and alt.
• fixed the keycodes() method to return both shift and alt (like shift-alt-) for ] in fr).
• replaced _above128charval_to_keycode with _above128char_to_keycode, which takes a char and uses a dictionary that can be indexed with both strings (the char) and ints (compared to ord(char)).

[dhalbert]

Superseded by #84. Thank you for initial work on this!