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

Complete rewrite of Qukeys #640

Merged
merged 1 commit into from Aug 22, 2019

Conversation

@gedankenexperimenter
Copy link
Contributor

commented May 3, 2019

This is a complete rewrite of Qukeys, built on top of #621, in order to implement several improvements and new features:

  • A new EventQueue class has been introduced, in order to store both key press and release events in the queue.
  • The direct dependence on KeyboardioHID is removed by only flushing one event from the queue per cycle.
  • The array of Qukey objects is now stored in PROGMEM instead of SRAM.
  • There is a new algorithm for determining which state a qukey will collapse into in the case of rollover from qukey to another key, which should reduce the rate of errors for "sloppy" typists.
  • A Qukey with a primary key value that is a modifier (including layer shift keys) is treated like a SpaceCadet key, with different semantics. The alternate (non-modifier) key value is only used if the SpaceCadet key is pressed and released on its own, without rolling over to any other key.
  • The code is generally simpler and easier to understand, with better inline comments explaining how it all works.

This pull request supersedes #637 (and functionally includes those changes, though not the actual commits).

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I have added two files in this branch in the src/kaleidoscope directory, one of which I'm fairly certain is generically useful (progmem_helpers.h), and one that is potentially useful to other plugins, but which is not needed at the moment by any of them (EventQueue.h).

For the progmem_helpers.h, I chose the name to be consistent with the pre-existing macro_helpers.h file. I think perhaps both of these should go in a utils subdirectory, along with some other things of a similar nature.

EventQueue.h should probably also be located elsewhere in the tree, unless @obra & @algernon want it right there. The most consistent place for it appears to be plugin/Qukeys/EventQueue.h, since it's function at the moment is to support Qukeys, like the files in plugin/Macros/ and plugin/MouseKeys/, but I didn't put it there at first because of the potential for it to be useful in other plugins. Perhaps a plugin/common/ directory would be a good idea for this kind of thing?

There's also the matter of the namespace for the EventQueue class, which I have tentatively put under kaleidoscope, rather than kaleidoscope::plugin.

I have no particular reason to prefer these organizational choices over other ones, so I would welcome authoritative decisions from the top on the matter.

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Major Structural Changes

  1. Qukeys now queues both key press and release events, where it only queued press events before. This allows it to use more data points (timestamps) to determine qukey state in rollover situations, hopefully reducing or eliminating instances of unintended output for people using them on letter keys.
  2. Qukeys now doesn't send extra HID reports when flushing events from its queue. This is similar to the changes in #637. Instead, it only allows one event to be flushed per scan cycle, so there is a longer delay if several events need to be flushed at a time, but not so much that it should be perceptible by users.
  3. Qukeys now stores its array of Qukey objects in PROGMEM, so it uses significantly less memory (4 bytes per Qukey).

Major User-Visible Changes

  1. The setTimeout() function has been renamed to setHoldTimeout() to clarify its function, and setTimeout() has been given a deprecation warning. I also updated the documentation to downplay this significance of this timeout, as many people seemed to assume this was the primary determining factor in what state a qukey ends up in.
  2. The setReleaseDelay() function has been deprecated in favour of a new setOverlapThreshold() function. The deprecated function now makes a reasonable guess as to the user's intention based on the value passed to setReleaseDelay(), so sketches still using that function should still work similarly.
  3. If a Qukey object is defined for a key whose primary value (i.e. its value in the keymap) is a modifier (including both keyboard modifiers & layer shift keys), it treats that key as if it were a SpaceCadet key. In other words, it is "inverted", and the primary key value is used when the key is held (where on a normal qukey, the alternate value would be used), and the alternate value is used when it is tapped. It is also treated differently in rollover situations; a "SpaceCadet" qukey will only use its alternate value (the non-modifier) if it is pressed and released without any intervening key press events. This should solve the problems that some users were having with qukeys on there modifier keys, and it might make Qukeys a viable replacement for SpaceCadet. This version is already immune to some existing SpaceCadet issues (see #623). It also means that if Qukeys is turned off, the key will become a modifier key, and won't need changes to the keymap when adding parens to shift keys (for example).

Footprint

On my example sketch (Model01-Firmware master, with Qukeys added), which includes 13 Qukey objects, plus several DualUse keys, this version uses +4 bytes of PROGMEM and -59 bytes of RAM. 51 of those bytes in RAM were used for storing the Qukey array, so even without that, it's using 8 fewer bytes of RAM. It should now be possible to define a much larger number of qukeys without risk of running out of memory. The code size has barely increased, despite the addition of a new feature (SpaceCadet emulation).

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from 9d74f08 to 928c6d9 May 3, 2019


// Load any data type stored in PROGMEM into an object of that type in memory.
template <typename _Type>
void loadFromProgmem(_Type const& pgm_object, _Type& object) {

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 3, 2019

Contributor

Very useful helper functions!

You might want to add to the comments that these are only applicable to intrinsic data types and trivial classes.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 3, 2019

Author Contributor

I would be happy to write more detail about what types can and cannot be used…if I understood what the rules were. No inheritance? No virtual functions? No constructors that have a body other than {}?

This has worked properly for all of the classes I've used it with, but I don't understand the seemingly arbitrary rules from the language specification.

// Copy an object from PROGMEM to RAM. This works as long as the type has a
// suitable constructor that does not require arguments.
template <typename _Type>
_Type cloneFromProgmem(_Type const& pgm_object) {

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 3, 2019

Contributor

Same here (only intrinsic types and trivial classes). The latter is in parts already mentioned in the comment (suitable constructor...).

// Remove the first event from the head of the queue, shifting the others.
void shift() {
--length_;
for (uint8_t i{0}; i < length_; ++i) {

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 3, 2019

Contributor

Could shift and remove be possibly made more performant by using a circular buffer approach instead? This would remove the copying operations.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 3, 2019

Author Contributor

I would guess so, but the downside is that EventQueue would then need to store two pointers or indices, and do more computation every time it iterates through the queue, which Qukeys does much more often than it shifts the queue. I did consider trying to use a circular buffer, but this seemed to be more likely the better choice overall. I'm not at all opposed to using a circular buffer if it turns out to be better; I just did it this way because the overall code was more obvious to me.

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 4, 2019

Contributor

A circular buffer variant would need only one additional modulo operation per loop cycle when iterating events and, as you pointed out, a single additional byte for the second index.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 4, 2019

Author Contributor

I would like to see how that is accomplished.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 4, 2019

Author Contributor

Wouldn't the ring buffer also need to translate the index value whenever one of the stored items is requested?

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 5, 2019

Contributor

One more thing. I would prefer the name KeyEventQueue as the term 'event' alone is to unspecific, especially given the fact that algernon started working on supporting rotation-knob-things (forgot the name), so there might be other types of 'events' in the future.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 5, 2019

Author Contributor

Excellent points, all; thank you. I'll rename the class to make it more specific, but I think KeyEventQueue is also not quite right, because this is a queue for before the Key value has been determined. I think that either KeyswitchEventQueue or KeyAddrEventQueue would be more apt.

In Kaleidoglyph, I found it useful to have two separate hooks: onKeyswitchEvent() and onKeyEvent(), with different treatment and guarantees. The KeyswitchEvent variant only gets events from physical keyswitches, and only gets each event once, whereas the KeyEvent hooks run in a loop that gets restarted whenever the Key value changes, and can get simulated events generated by plugins.

For that matter, I also think handleKeyswitchEvent() is a misnomer in Kaleidoscope. Its parameters don't necessarily represent physical key switches, and also represent states, not events — a very important distinction when trying to debug Kaleidoscope code.

At one point, I recall you commenting on the overuse of the term "key" for too many different things, and you suggested something more specific for the Key class, but I can't recall what it was. I do remember being in favor of it, though.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 5, 2019

Author Contributor

Regarding pushBack() & popFront(): I do not find those names particularly intuitive; though their meaning is clear, it takes me a few seconds each time I look at them to deduce their function. More important, they are not quite analogous to append() & (especially) shift(). For (arguably misguided) performance reasons, I haven't made a QueueEntry class, so there isn't a single object that is passed to append(), and there's no return type at all for shift(). The need for random access to the queue entries, and the fact that Qukeys frequently looks only at one property of each queue entry are what led me to implement it in this way. I feel like adding a popFront() method would require either an additional type to be defined as the return type. That's not too hard to do, but I still wouldn't use it for Qukeys, because by the time it knows that shift() should be called, it already has a copy of the event type and address, so there's no need to call popFront().

This all suggests to me that EventQueue is more particular to Qukeys than I had first imagined it to be. I'll try adding an inner class to give popFront() & pushBack() an appropriate type, and see what it looks like, but I'm leaning towards leaving them out and putting this class header under plugin/Qukeys/ to make it clear what it was intended for.

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 6, 2019

Contributor

For that matter, I also think handleKeyswitchEvent() is a misnomer in Kaleidoscope. Its parameters don't necessarily represent physical key switches, and also represent states, not events — a very important distinction when trying to debug Kaleidoscope code.

Good point. Now as we have a flexible hook system, that due to recent changes supports deprecation of hooks, it would be possible to fix this without causing too much overhead.

At one point, I recall you commenting on the overuse of the term "key" for too many different things, and you suggested something more specific for the Key class, but I can't recall what it was. I do remember being in favor of it, though.

Right, I remember that, too. IMHO, KeyswitchEventQueue would be a better choice than KeyAddrEventQueue, as we also use KeyAddr to reference per-key LED's, so the prefix KeyAddr does tell less about what the queue actually handles/stores.

Regarding pushBack() & popFront(): ...

Letting the event queue store some sort of struct that represents the keyswitch event would make it more like a traditional container and pushBack() would make sense. But it is obvious that this would make using an internal bitfield more complex and possibly more costly.

FWIW, popFront() in some implementations/libraries does not return a value (cf. STL's deque or list). i have seen suggestions to rename it dropFront(), which would make more sense, though.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 6, 2019

Author Contributor

Letting the event queue store some sort of struct that represents the keyswitch event would make it more like a traditional container and pushBack() would make sense. But it is obvious that this would make using an internal bitfield more complex and possibly more costly.

Yeah, I'm not thrilled about the complexity of the bitfield implementation, but the savings of almost one byte per entry was enough to tempt me. On the other hand, that's the biggest thing that keeps the class from being more generally useful; on anything that wasn't so space constrained (e.g. ARM embedded processors), I think it would be better to just use a bool inside a structure used to store the event info. I'd still be inclined to keep the timestamp management separate, especially if I could figure out a way to have a template class specialization that doesn't use timestamps at all. And now that I've got that in my head, I'll give it a try.

In a similar vein (on account of the bitfield's specificity), I'm sold on preferring KeyswitchEventQueue to KeyAddrEventQueue. I had thought of the latter as being more illustrative of the data it stores (in light of the functions (mis)named *KeyswitchEvent), but it was the weakest of preferences. I'll change it; thanks.

typename _Timestamp = uint16_t>
class EventQueue {

static_assert(_max_length <= 255,

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 3, 2019

Contributor

Restricting _max_length by 255 does not make sense as it is restricted by 64 anyway, given that uint64_t is the largest possible unsigned integer datatype to be used for _Bitfield.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 3, 2019

Author Contributor

Good point. I'll fix that. It would be crazy to have a queue even that long, anyway.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 3, 2019

Author Contributor

On further consideration, it is possible for _max_length to be greater than 64 — if _Bitfield is not merely an integer type. That's an academic point here, though, unless we can think of an actual use case for a queue that long.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 3, 2019

Author Contributor

Oh — but if _Bitfield isn't just an integer, bitRead() won't work as expected…


static_assert(_max_length <= 255,
"EventQueue error: _max_length must be less than 256!");
static_assert(_max_length <= (sizeof(_Bitfield) * 8),

This comment has been minimized.

Copy link
@noseglasses

noseglasses May 3, 2019

Contributor

This second static_assert is IMHO already sufficient.

This comment has been minimized.

Copy link
@gedankenexperimenter

gedankenexperimenter May 3, 2019

Author Contributor

Right. I'm not sure why I added the first one. I started out with just this one, and now that I look at it, the first one would always pass because _max_length is a uint8_t.

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch 2 times, most recently from f88caa9 to 2a5466f May 4, 2019

@noseglasses

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@gedankenexperimenter, appologies for concentrating on rather peripheric parts of your PR. I am aware that the far more important part are your changes/improvements to Qukeys.

I am currently lacking capacity to dig into the latter. That's why I concentrated on those parts that you intent to become part of Kaleidoscope's internal service infrastructure.

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@gedankenexperimenter, appologies for concentrating on rather peripheric parts of your PR. I am aware that the far more important part are your changes/improvements to Qukeys.

No problem! Those were areas in which I was hoping to get some input from others, anyway, so your comments have all been very helpful.

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

I want to pop the discussion of EventQueue up to the top level. I'm thinking that it makes sense to make it more generic. Instead of having a hardcoded KeyAddr type as part of the queue entry, that could be a template parameter. Then it would still amount to the same class for Qukeys, but would have more potential utility to other plugins, as well.

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

I just added a bunch of small commits; I'll squash them before submitting this PR for real.

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from ee53a98 to 142456d May 6, 2019

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I added a change to set the count of configured Qukey objects correctly automatically, without requiring the use of sizeof in a macro (though the macro is still there). It uses an array reference parameter in a template function, which is a very useful technique that I think is worth drawing attention to. See here.

@kejadlen

This comment has been minimized.

Copy link

commented May 14, 2019

It looks like I get å every time I type å with this change? (Åutocorrect is turning some of them back into normal å characters.)

My layout is here: https://github.com/kejadlen/Model01-Firmware/blob/master/Model01-Firmware.ino

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Can you tell me what operating system you're using? Also, if you know how you would type a å character on a "normal" keyboard, what sequence or combination of keys would you use?

Thanks for giving it a try; I'll look into it today.

@kejadlen

This comment has been minimized.

Copy link

commented May 14, 2019

macOS - my bad on not including it initially. 🤦‍♂️

It looks like you can get it by holding option (alt) and typing a:

https://fsymbols.com/keyboard/mac/https://fsymbols.com/keyboard/mac/

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I added a commit that fixes that bug with layer-shift keys. Thanks for finding it, @kejadlen!

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from bb27b58 to 54aff6f May 16, 2019

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from 54aff6f to dd02ae5 Jul 8, 2019

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

I have rebased this on master, but I haven't tested it yet, so I'm leaving the PR as a draft until I get access to a Model01 again.

@kejadlen

This comment has been minimized.

Copy link

commented Jul 11, 2019

FYI, seems to be working fine for me, at least for the past few hours since I flashed this after seeing your update.

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I've got a couple of very minor changes to make still. Thanks for trying it out, @kejadlen!

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from 35b39ca to 17f9b54 Jul 27, 2019

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from 17f9b54 to 9281402 Jul 27, 2019

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

I rebased this on top of the master branch again, and used the standard timer helper functions where appropriate. I've tested it on a Model01, and everything appears to be working properly.

@gedankenexperimenter gedankenexperimenter marked this pull request as ready for review Jul 27, 2019

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from 2789eca to bfa135f Jul 27, 2019

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

I'm having a devil of a time satisfying astyle on this one, and the code is looking less and less readable to me with every change it demands in order to pass the build tests.

Rewrite Qukeys plugin from scratch
This is a complete rewrite of Qukeys, in order to implement several improvements
and new features:

- A new KeyAddrEventQueue class has been introduced, in order to store both key
  press and release events in the queue.
- The direct dependence on KeyboardioHID is removed by only flushing one event
  from the queue per cycle.
- The array of Qukey objects is now stored in PROGMEM instead of SRAM, and is
  configured via an array reference template function in order to automatically
  ensure the count will be correct.
- There is a new algorithm for determining which state a qukey will collapse
  into in the case of rollover from qukey to another key, which should reduce
  the rate of errors for "sloppy" typists.
- A Qukey with a primary key value that is a modifier (including layer shift
  keys) is treated like a SpaceCadet key, with different semantics. The
  alternate (non-modifier) key value is only used if the SpaceCadet key is
  pressed and released on its own, without rolling over to any other key.
- The code is generally simpler and easier to understand, with better inline
  comments explaining how it all works.

Fixes #626.

Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>

@gedankenexperimenter gedankenexperimenter force-pushed the gedankenexperimenter:qukeys.rewrite branch from f7b6625 to 1e58fcf Jul 27, 2019

@gedankenexperimenter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

I've tested this out, and I believe it's ready to merge.

@algernon

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Ooof. This has been lingering so very long, and I apologize for that. I had a hard time doing a proper review, because I never quite understood the old Qukeys code, but wanted to take the opportunity to fix that now, and understand the new one.

While my understanding is still not as good as I will eventually want it to be (because Qukeys is one of the most versatile plugins we have), but I think I know it much better now. The new code is also considerably easier to follow. The deprecations and the docs are OK too, so a step up in pretty much every way.

Thank you @gedankenexperimenter!

@algernon algernon merged commit e50b393 into keyboardio:master Aug 22, 2019

2 checks passed

DCO DCO
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.