Keyboard report - clear in every cycle vs. report on key event

(Noseglasses) #21

You’re right again. I will put Qukeys first.

(Michael Richters) #22

Getting back to this – yes, that’s obviously better than what I had in mind, assuming a loop through an array of plugin pointers. I was thinking of using a pre-build script to define the hook master functions, enabling automatic sorting of plugins so I don’t have to count on the user to get it right (and allowing users to override it, if they really want to), and allowing a different plugin order based on which hook point is being called. I’m on the fence about doing it that way and just having the array of Plugin pointers. Thanks for the idea, though; if I go with the Plugin* array system, I’ll definitely use it.

That’s a matter of semantics, really. I haven’t used the term “injected” anywhere, and it’s possible to have an event generated by a plugin that nevertheless sets caller to nullptr. I can’t think of why one would do this, mind you…

(Jesse) #23

Is there some abstractable feature or service that could be offered as a standalone plug-in that both Qukeys and Papageno could use?

(Michael Richters) #24

I have been toying with the idea of having more than one keyswitch event hook, based on the guarantees the different plugins make. That would help keep them ordered properly, but I’m not certain that it would be a big improvement. Qukeys & Papageno could both have physicalKeyswitchEventHook() functions, run before the general keyswitchEventHook() functions of other plugins…

I don’t think this really describes what you had in mind, but it’s vaguely in the same area.

(Michael Richters) #25

Earlier today, I worked out what I think would be an improved system for dealing with “injected” keyswitch events. This should work well for Kaleidoglyph, but maybe not so well for Kaleidoscope.

For most plugins (i.e. not Qukeys or Papageno or perhaps Leader), it’s actually not important to ignore injected events that come from plugins later in the processing order, as long as there’s some mechanism to prevent infinite loops.

Suppose we have two plugins: A and B. Each plugin checks the event.key value, and has its own Key variant that it recognizes (like Kaleidoscope-Ranges). Each plugin also (sometimes) replaces the event.key value with some other Key value, and lets processing continue. And that replacement Key value, in general, doesn’t have any restrictions — A can replace an AKey value with a BKey value, and vice versa.

Unless the plugin event handler loop is restarted somehow (e.g. by calling handleKeyswitchEvent()), only one of those will work properly, depending on the order of plugins in the loop. But if we restart, there’s a possibility of an infinite loop. There’s also a possible problem of recursion causing the MCU to run out of memory.

So, instead of having the plugins handle the problem, I decided to try iterating through the handlers, and checking to see if event.key has changed after each one. If it changes, I restart the loop, but mask that plugin by loop index, so that any plugin that has already changed the Key value for a given event doesn’t get its handler called again. This way, to a certain extent, it doesn’t matter if A comes first or B comes first; each can inject Key values that the other will interpret, and there’s less for them to do. All a “normal” plugin needs to do is update the event.key value and return; there’s no need to call handleKeyswitchEvent() to make sure the other plugins get a chance to process the new value. This way, infinite loops are prevented, there’s no recursion, and there’s even less for the plugins to do.

(Michael Richters) #26

Qukeys needs to know the key position and the time (uint16_t(millis())) of each keyswitch event. I haven’t thought of a way that it would be useful to share that data with another such plugin, because it would introduce more complexity than the expected savings. The one thing that I think might be worthwhile would be to pass the timestamp of the event as a function parameter. This would preserve the actual time of the physical keypress, which subsequent plugins might find useful (but could still complicate timeouts). I do think this would be useful for Qukeys, as long as the prior plugin guarantees that it will not drop any events, and flush them in order.

Both of the above guarantees are somewhat loose, however, and can be defined in human terms. For example, Qukeys can be configured to change the order of certain events that occur nearly simultaneously. If using this system, it would probably alter the timestamps slightly when flushing them out of order, effectively rewriting the events to better match the user’s intentions. This could even apply to the first requirement, too — if Papageno was before Qukeys in the order, it might still work well enough, because the user could be thinking of a Papageno sequence as a single logical key press. There is a potential problem, though, depending on how long events are delayed by each plugin.

I don’t think I see a way to work for Kaleidoscope, but it could be done easily in Kaleidoglyph. I’m still not sure that it would be an improvement, but I’ll probably give it a try, anyway.

(Noseglasses) #27

It seems that the most common feature of Qukeys and Papageno is that both plugins store events in a queue. Although a common feature, in practice, there are some differences that would make it unfeasible to have a common event queueing “service”-plugin.

One difference is that, Qukeys stores a time-stamp with the event, probably in the queue data structure. (please correct me, if I’m wrong here, @merlin). Papageno, on the contrary does only preserve the event order, without any time stamps required. Also, as the event queue is a key feature of the Papageno’s pattern matching, it’s an integral part of the Papageno library, rather than the plugin. Kaleidoscope-Papageno is just an interface library to the actual Papageno library. That’s why at least Kaleidoscope-Papageno wouldn’t benefit from an event queueing service-plugin.

However, that doesn’t mean that other/future plugins that require event queueing wouldn’t benefit from a dedicated service plugin.

Apart from that, according to @merlin, there are a bunch of possible changes to Kaleidoscopes principal design that would make both plugins’ implementations simpler and probably more robust. But that is another story, much different from what you were asking for.

(Noseglasses) #28

The mechanism you describe - masking handlers to prevent infinite recursion - sounds promising. It solves at least two problems, hook recursions and memory exhaustion as a result of stack stack overflow (which can happen even without infinite recursion).

Under the assumption that all event handler hooks are stateless, this solution appears quite optimal.

With current Kaleidoscope, however, handlers are not guaranteed to be stateless. It is therefore possible that an existing plugin’s handler intents to be called recursively to do different things each time it is called, based on some internal state. A hook also might return different Key values each time it is called. Both is worth being taken into account when rethinking the hooking/event handling system. If we try to disable recursive hooks, all code that currently uses recursion as a simple replacement for iteration or for whatever other reason will have to be drastically changed.

Anyway, I still like the masking idea. You already proposed other more or less radical changes that may require some plugins to be redesigned. But as long as their implementations would benefit, e.g. become significantly shorter and more transparent, it would be worth it.

In any case, there is no proper way to prevent direct or indirect recursive calls to handleKeyswitchEvent(). There is no non-template C++ feature that I am aware of, that would enable to check for recursion at compile time. The only runtime solution I can think of, is to add a guard variable that causes an early exit from handleKeyswitchEvent() once a recursive call is detected. Plugins that attempt recursion would silently malfunction, which does not seem a desired behavior.

Papageno, in its current state, is a good example for such recursive calls to handleKeyswitchEvent() as it is designed to flush events via handleKeyswitchEvent() from its event handler hook. It is probably possible to change this and trigger postponed flushing from some other hook function, but that implementation seems less straight forward to me.

I think that we should accept recursive calls to handleKeyswitchEvent(). If we do so, there is one critical aspect of the proposed masking approach. handleKeyswitchEvent() must be stateless in order to be reentrant. This could e.g. be achieved by stack allocating all data structures (local variables instead of global variables) dedicated to hook masking within handleKeyswitchEvent(). I am pretty sure you have done so already.

(Michael Richters) #29

Good ponits.

I never had the goal of entirely preventing recursion (Qukeys would still call handleKeyswitchEvent() from its own event handler), but I did want a way for plugins to interoperate well without requiring recursion.

I have noticed a flaw in my scheme, however. The way I had written it would still allow event-queueing plugins to use the caller mechanism as before when making calls to handleKeyswitchEvent() (necessary because a queued event isn’t generally flushed in the same cycle that it was queued, of course). But when restarting the event handler hook loop, I was only thinking about masking handlers that had triggered a restart. It’s also necessary to mask any handlers that are done processing the event (like the aforementioned event-queueing plugins), and want to make sure that they don’t see it again (where “event” is defined somewhat loosely). A queueing plugin could call handleKeyswitchEvent(event, this), resulting in its own handler setting caller to nullptr, but then when some other plugin changes the event.key value, and the loop restarts, it needs to be masked, or it will perceive a new event, instead of recognizing the one it just processed.

A few solutions have occurred to me, including adding at least one additional hook point, but the one I like best at the moment is to have plugins use the caller parameter to signal that they’re done with that event by setting it to this. In turn, the controller checks the caller value when the handler returns, and if it’s not nullptr, it masks that plugin, and if it’s a pointer to the plugin that just returned, it sets it to nullptr, allowing subsequent plugins to normally process the event.

That gives plugins a way to prevent repeated calls to their event handlers for the same event, and that should help make the system more robust. In particular, I think it would at least help address your concern about stateful plugins.

(Noseglasses) #30

Your proposal appears to be a good approach, but it’s growing more and more complex.

I am at my personal limits when it comes to imagining what exactly happens in the different scenarios (with and without event-queueing plugins and with and without returning caller == this).

Some examples that demonstrate the resulting call order of event handler hooks in the different possible scenarios would help my understanding and possibly that of others following this discussion. I could imagine to use some kind of pseudo C++ code like in the following (pseudo) examples. But maybe you know of some better way to demonstrate what happens.

// Plugins a,b
// Event handler call order a_eventHandlerHook, b_eventHandlerHook

// In matrix scan
caller = nullptr;
handleKeyswitchEvent(event, caller);

// In handleKeyswitchEvent
a_eventHandlerHook(event, caller); // caller unchanged
b_eventHandlerHook(event, caller); // caller unchanged
// Plugins a,b
// Event handler call order a_eventHandlerHook, b_eventHandlerHook

// In matrix scan
caller = nullptr;
handleKeyswitchEvent(event, caller);

// In handleKeyswitchEvent
a_eventHandlerHook(event, caller); // caller = this
// eventHandlerHook masked
// handler loop restarted
// caller =  What to assign here?
b_eventHandlerHook(event, caller); // caller unchanged

You propose to use the caller argument as signal value (like an additional return value) to the controller. In your original proposal caller was used by handlers to keep track of possible recursion. There it was only input to the event handler hooks. I am slightly confused now. To me it seems that caller is now a multi-purpose argument for input and output.

To make it easier for plugin developers and also to enable future changes to the interface, it might be worth considering to handle all access to caller by auxiliary (wrapper) functions or (function) macros. Taking this idea even further, it would help to call the variable something neutral like context instead of caller, just to keep the plugin developer from assuming anything about it. In the optimal case, the user wouldn’t have to deal with caller explicitly.

Possibly invoked at the end of a hook:


Used by event-queueing plugins:

void handleAsynchronousKeyswitchEvent(EventType /* replace with the actual typename */ event) {
   handleKeyswitchEvent(event, nullptr);