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

CharShift rework to support named key lookup without having to deal with remembering indexes #1299

Open
Lilith-Daemon opened this issue Oct 26, 2022 · 0 comments
Labels
design-change Proposals that incur potentially breaking design changes

Comments

@Lilith-Daemon
Copy link

Premise & Idealogy

I have been working with @algernon @gedankenexperimenter to implement a better firmware interface to the wonderful CharShift plugin. In particular I find working with integer indexes to reference the CharShift::KeyPair to be difficult and confusing.

In particular, I want to be able to assign a label to the key pair when I declare it, and then use this label to reference the pair in my keymap. In pseudo code, an ideal interface may appear as:

DEFINE_KEY_PAIRS {
    LABEL_A => (NO_SHIFT_KEY, YES_SHIFT_KEY),
    LABEL_B => (NO_SHIFT_KEY, YES_SHIFT_KEY),
    LABEL_C => (NO_SHIFT_KEY, YES_SHIFT_KEY),
    ...
    LABEL_N => (NO_SHIFT_KEY, YES_SHIFT_KEY)
}

And we can reference this in our keymap as: LABEL_N. Due to the syntactical constraints of the C++14 language, I settled on a visually and structurally similar method of defining and referencing our key pairs.

NCS_KEYS(
    (LABEL_A, NO_SHIFT_KEY, YES_SHIFT_KEY),
    (LABEL_B, NO_SHIFT_KEY, YES_SHIFT_KEY),
    (LABEL_C, NO_SHIFT_KEY, YES_SHIFT_KEY),
    ...
    (LABEL_N, NO_SHIFT_KEY, YES_SHIFT_KEY)
)

And we can reference this in our keymap as: NCS(LABEL_N).

Implementation

To keep existing sketches functional, the current CS macros remain in place, and users may update to the new version without any issues. Additionally, I have created an ICS_KEYS series of macros, to allow declaring an indexed key shift setup outside of the setup() function, to be more consistent with the new NCS_KEYS. (It uses the onSetup event, rather than monkeypatching the initialization). As such, I would like to depreciate the CS macros, in favor of ICS/NCS, but they can remain in place without issue.

The current plugin proof of concept can be seen https://github.com/Lilith-Daemon/CharShift

And you are welcome to view my personal firmware for a demonstration of its usage.
https://github.com/Lilith-Daemon/Model100

Developer Concerns

As one of the concerns raised about the plugin, is that due to the usage of macros it can be difficult to debug user errors, and present long and meaningless error messages. This is solved on two fronts: First, adding the compiler directive -ftrack-macro-expansion=0 we can dramatically decrease the meaningless bulk output that comes from macro expansions. Second, through a combination of variadic #defines and clever naming, we are able to trick the preprocessor into providing meaningful error messages.

Some examples of these include:

  • NCS_KEYS___INVALID_FORMAT_EXTRA_COMMA
  • NCS_KEYS___INVALID_FORMAT_EXTRA_COMMA
  • NCS_KEYS___INVALID_TUPLE_FORMAT___NEEDS_3_VALUES___NOT_ENOUGH
  • NCS_KEYS___INVALID_TUPLE_FORMAT___NEEDS_3_VALUES___TOO_MANY
  • etc

If you are interested in seeing the full error messages generated, I've attached error_messages.md which contains 5 test cases showing a call to NCS_KEYS from my personal firmware, followed by all STDERR messages generated.

Future Work

The CharShift plugin as I propose it requires at least two additional tasks before it could be merged into kaleidoscope, first updating the documentation to explain the new functionality; and second writing new test cases to validate said functionality.

@Lilith-Daemon Lilith-Daemon added the design-change Proposals that incur potentially breaking design changes label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-change Proposals that incur potentially breaking design changes
Projects
None yet
Development

No branches or pull requests

1 participant