Skip to content
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

M5Stack Cardputer keyboard handling improvements. #9529

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

konstantint
Copy link

  • Exposed the DemuxKeyMatrix object as board.KEYBOARD.
  • Implemented the ability to not have the keyboard assigned as serial input (sys.stdin) by specifying M5STACK_CARDPUTER_KEYBOARD_SERIAL=0 in settings.toml. This enables custom handling of individual key up/key down events.

For context, see discussion in 090f330

@konstantint konstantint force-pushed the m5stack-cardputer-keyboard-fix branch 5 times, most recently from f75dd60 to 34ed9a3 Compare August 16, 2024 11:52
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. Would you prefer to do this dynamically instead? We may be able to add functions to board to do it.

@tannewt
Copy link
Member

tannewt commented Aug 16, 2024

@konstantint
Copy link
Author

konstantint commented Aug 16, 2024

Would you prefer to do this dynamically instead?

This is a reasonable idea. I didn't go for it immediately as I was not sure what would be the best interface for it and simply tried the simplest thing that came to mind first (and was easy enough to try even without deep knowledge of the codebase).

Intuitively it currently seems to me that a reasonably clean solution would be to have some kind of a M5StackCardputerKeyboard class, exposed as board.KEYBOARD, which could a DemuxKeypad with the extra functions:
- attach_serial/detach_serial that would enable/disable routing of the keyboard events to stdin.
- translate_key (because the mapping between key numbers and characters is useful and would be worth exposing to the Python codebase).
The class could also expose the character ringbuffer for the user to consume (say char_events), which might be a convenient alternative to events due to the built-in key id -> character mapping (once it is done anyway).

I'm not super-sure how this class would be best exposed to the python codebase and documented. Putting it into shared-bindings does not seem very useful, due to the (currently) rather limited scope. Should it be kept in the board-specific build files but exposed as a non-instantiatiable M5StackCardputerKeyboard class in Python? Where should the docs go to make users aware of this existence? Am I overengineering this? :)

@bill88t
Copy link

bill88t commented Aug 19, 2024

I think the ideal api would be booting with it as stdin, then whenever the user calls board.KEYBOARD() the keyboard is detached from stdin.
Then when that object is deinitialized (or expires from scope, finalizers?), it's attached back to stdin.

I don't think it's wise to try to get it under shared-bindings since every keyboard board will need different hacks.
It'd be more function arguments than actual code. (Just look at a cardputer, the t-deck and your keyboard, you'll get what I mean.)
I think it's best to leave it as is, under the board folder.

@konstantint
Copy link
Author

konstantint commented Aug 19, 2024

Given that for a few of the other boards that I've seen board.XXXXXX seems to always be either a pin or a display object, making board.KEYBOARD a function would feel rather inconsistent. I'd also find the behavior of detaching from stdin on object access pretty unintuitive (also, what if I want to use the object without detaching) and even more unintutiive if anything would happen to it automagically on scope expiration.

Something like an explicit attach_keyboard_serial / detach_keyboard_serial would be a way more straighforward interface for this specific functionality IMO.

My suggestion about making a dedicated class is related to the fact that there is more to the keyboard than just being able to attach/detach it to stdin. Namely, the key ID to character mapping is a valuable thing and if not exposed from the built-in solution, most users would probably have to copy-paste-roll-their-own. To avoid this, we should expose this mapping somewhere. That "somewhere" could be the M5StackKeyboard class / module / variable, for example.

I agree that putting this into shared-bindings feels unnecessary. However, if we have a class implementation hidden in the board/ directory, how would any user be able to find any documentation about it (without having to browse the code on github, which, IMO, is not the right way of advertising functionality :)?

Is there a good place for documenting such a board-specific module / class / variable?

@dhalbert
Copy link
Collaborator

I agree that putting this into shared-bindings feels unnecessary.

You can put it in ports/espressif/bindings, which is port-specific, and turn it on only for that board. There is already a mechanism for finding doc in ports/*/bindings.

@tannewt
Copy link
Member

tannewt commented Aug 19, 2024

Is there a good place for documenting such a board-specific module / class / variable?

There isn't a great place. This is kinda the first board-specific functionality. I think bindings is fine. Other docs can live on Adafruit Playground: https://adafruit-playground.com/

@konstantint konstantint force-pushed the m5stack-cardputer-keyboard-fix branch from 34ed9a3 to 34c605e Compare August 19, 2024 21:30
@konstantint
Copy link
Author

konstantint commented Aug 19, 2024

... so I looked a bit into the directory structure and it feels to me that creating something like a espressif/bindings/m5stack_cardputer_keyboard, along with the need to introduce the pretty niche makefile variable and effectively require every build to indirectly be aware of this, is not nice at all. Things would then become especially ugly if the approach starts proliferating for any other boards that might want to implement a similar board-specific module.

I'm therefore pretty convinced now that the code should go into boards/<board>/, and the documentation generation problem needs to be solved as a separate follow-up. I presume it should be possible to include board-specific docs somewhere somehow eventually, right?

I'll try to draft a new PR with a custom keyboard class implementation soon.

@tannewt
Copy link
Member

tannewt commented Aug 20, 2024

I'm therefore pretty convinced now that the code should go into boards/<board>/, and the documentation generation problem needs to be solved as a separate follow-up. I presume it should be possible to include board-specific docs somewhere somehow eventually, right?

Ya, that'd be a good idea. I think it'd help folks.

* Exposed the DemuxKeyMatrix object as board.KEYBOARD.
* Implemented the ability to not have the keyboard assigned as
  serial input (sys.stdin) by specifying
  M5STACK_CARDPUTER_KEYBOARD_SERIAL=0 in settings.toml.
  This enables custom handling of individual key up/key down events.
@konstantint konstantint force-pushed the m5stack-cardputer-keyboard-fix branch 2 times, most recently from e0518b9 to 10beacf Compare August 24, 2024 23:53
@konstantint
Copy link
Author

... so I figured that making a subclass of DemuxKeyMatrix is a bit of a pain and instead just made a cardputer_keyboard module.
The module exposes the KEYBOARD object as well as the functions detach_serial, attach_serial and key_to_char.

As previously, the board starts with the serial "attached". If the user does not need it, they can do something like:

import cardputer_keyboard
cardputer_keyboard.detach_serial()

...
event = cardputer_keyboard.KEYBOARD.events.get()
if event:
   char = cardputer_keyboard.key_to_char(event.key_number, shift_down)
   ...

which seems reasonable, at least for my personal purposes. As discussed above, the docs for this probably won't propagate anywhere so it will be a bit of a secret functionality for the time being (perhaps Google will do a good job pointing to this discussion when queried).

WDYT?

@konstantint konstantint force-pushed the m5stack-cardputer-keyboard-fix branch from 10beacf to 7a99a70 Compare August 25, 2024 00:00
 - Move the keyboard-related code from board.c to
   cardputer_keyboard.c, exposed to python as cardputer_keyboard.
 - Remove board.KEYBOARD, provide cardputer_keyboard.KEYBOARD instead.
 - Provide additional methods in cardputer_keyboard:
    - detach_serial()
    - attach_serial()
    - key_to_char(key, shift)
@konstantint konstantint force-pushed the m5stack-cardputer-keyboard-fix branch from 7a99a70 to 4fb31c4 Compare August 25, 2024 00:03
@konstantint
Copy link
Author

@tannewt ping

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine with me!

Want to try tweaking the make stubs command to include this? https://github.com/adafruit/circuitpython/blob/main/Makefile#L266 Once it is in the stubs, then it should be made available in the API docs.

@tannewt
Copy link
Member

tannewt commented Sep 3, 2024

@konstantint Waiting for you to reply before merging.

@konstantint
Copy link
Author

I took a quick peek at the make stubs you mentioned and it seems I'd need more than 20 minutes to figure out what exactly needs to be done. So far I haven't found that extra time :)

I'd probably prefer merging this now as-is and looking into how to generate the documentation a bit later, to avoid this hanging for uncomfortably long (e.g. I'm probably too busy this weekend to take a look).

@tannewt tannewt merged commit eb2054b into adafruit:main Sep 4, 2024
15 checks passed
@tannewt
Copy link
Member

tannewt commented Sep 4, 2024

Works for me! Thank you!

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.

4 participants