Add focus command for retreiving a list of installed LED effects

[EvyBongers]

No description provided.

[EvyBongers]

On a side note, I noticed that there are several plugins that don't implement onNameQuery. Is that something to be fixed sometime, or is there a specific use case where that's needed for certain plugins?

[obra]

On a side note, I noticed that there are several plugins that don't implement onNameQuery. Is that something to be fixed sometime, or is there a specific use case where that's needed for certain plugins?

I wanted the answer to be "check event-handler-hooks.md". So I made that true: https://github.com/keyboardio/Kaleidoscope/blob/master/docs/api-reference/event-handler-hooks.md#onnamequery

[EvyBongers]

On a side note, I noticed that there are several plugins that don't implement onNameQuery. Is that something to be fixed sometime, or is there a specific use case where that's needed for certain plugins?

I wanted the answer to be "check event-handler-hooks.md". So I made that true: https://github.com/keyboardio/Kaleidoscope/blob/master/docs/api-reference/event-handler-hooks.md#onnamequery

I had that change ready to be committed 😅 Doesn't change that I can't tell in what scenarios it is or isn't important for the know to know about it. I'll leave that up to you

[EvyBongers]

The basics are implemented in all LED mode plugins.

Example output using the M100 example sketch:

$ ../_firmware/kaleidoscope/bin/focus-send led_effects
LEDRainbowEffect
LEDChaseEffect
SolidColor
SolidColor
SolidColor
SolidColor
SolidColor
SolidColor
SolidColor
LEDBreatheEffect
AlphaSquareEffect
StalkerEffect
ColormapEffect

For the plugin to be useful for displaying LED effect names in Chrysalis, some configuration options might need to be included, for the SolidColor plugin for example.

[obra]

Nice! It does seem like it makes sense to be able to (optionally) specify an additional string for at least the SolidColor LED effect. I wonder if it's worth implementing something in the base class to let the user pass in a descriptive string in the constructor or with another method call and then having the base class able to respond with that in preference to the hardcoded name.

Given that, it might make sense to refactor onNameQuery into the base class and to pull the name from a string baked into the plugin so we can reuse it for this

[EvyBongers]

Nice! It does seem like it makes sense to be able to (optionally) specify an additional string for at least the SolidColor LED effect. I wonder if it's worth implementing something in the base class to let the user pass in a descriptive string in the constructor or with another method call and then having the base class able to respond with that in preference to the hardcoded name.

Given that, it might make sense to refactor onNameQuery into the base class and to pull the name from a string baked into the plugin so we can reuse it for this

I like the idea of implementing it in the base class. Right now every plugin has a few nearly identical lines, just to make this work. Moving the LED effect name to a variable allows for deduplicating lines as well as adds flexibility for including parameters, like for SolidColor, so it's win/win.

Will look into that soon.

[EvyBongers]

There's something I have yet to figure out about the constuctor arguments, but this new design seems to be working great.

$ ../_firmware/kaleidoscope/bin/focus-send led_effects
LEDOff
LEDRainbowEffect
LEDRainbowWaveEffect
LEDActiveLayerKeysEffect
LEDChaseEffect
SolidColorEffect: /* TODO */
SolidColorEffect: /* TODO */
SolidColorEffect: /* TODO */
SolidColorEffect: /* TODO */
SolidColorEffect: /* TODO */
SolidColorEffect: /* TODO */
SolidColorEffect: /* TODO */
LEDBreatheEffect
AlphaSquareEffect
StalkerEffect
ColormapEffect
HeatmapEffect
LEDActiveLayerColorEffect
WavepoolEffect
DigitalRainEffect

One question: should led modes that haven't got a name specified return a default (empty?) string? With this new design, that can be easily achieved in LEDModeInterface.h

[obra]

Since the goal is to match up these names to the LED effect list in a desktop app like Chrysalis, I think we want a straight one to one mapping between effects and names ordered the same as they are on the device

[EvyBongers]

I implemented and tested the led mode name in just about all the relevant plugins now. The only exceptions are Jukebox and Miami plugins, because with my limited understanding of C++, I failed to figure out how to implement it in those.
On a side note: when I tried getting Tricolor to work, I got errors that I failed to resolve and at this point I'm not even sure if Tricolor is even meant to be used as a plugin.

[EvyBongers]

I'm not sure what I need to change in order to get those failing checks to pass. I would appreciate some help :)

[obra]

I'm not sure what I need to change in order to get those failing checks to pass. I would appreciate some help :)

Oh. I think the issue here is that you're essentially introducing a focus dependency in the core that has not previously existed. I think you probably want to have onLedEffectQuery return a pointer
and have the code inside focus do the output.

[EvyBongers]

Oh. I think the issue here is that you're essentially introducing a focus dependency in the core that has not previously existed. I think you probably want to have onLedEffectQuery return a pointer and have the code inside focus do the output.

The api expects hooks to return an EventHandlerResult, so it can't return a pointer. I tried to solve it by passing a pointer as reference and read it after the LED mode plugin set the value:

if (inputMatchesCommand(input, cmd_led_modes)) {
  const char *ledModeName;
  kaleidoscope::Hooks::onLedEffectQuery(ledModeName);
  ::Focus.send(ledModeName, '\n');
  return EventHandlerResult::EVENT_CONSUMED;
}

This resulted in only a single LED mode being sent back. After hours of debugging, the simplicity of the problem struck me: kaleidoscope::Hooks::onLedEffectQuery(ledModeName); only returns once the hook has been called on all plugins.

I could try to make it a list of strings that gets updated, but that seems far from efficient. It also doesn't solve the other problem that is reported in CI, that the sketch becomes too large for the Model01.

An alternative solution might be to update the hooks API to support callback functions and supply a callback that calls ::Focus.send() every time the hook is called on a plugin.

[obra]

I think this patch gets us what we want in terms of flow, using the callback. Before it can land, I need to make sure we cut down the code size for AVR a bit. I've got a potential refactor I just need to test out.
for-evy.patch

[EvyBongers]

Shouldn't this return EventHandlerResult::OK, similar to ::Focus.sendName?
Even if it doesn't, with the current signature of onLedEffectQuery we might as well rename ::Focus.sendName to ::Focus.sendLine and use that, rather than having a handler method that does almost exactly the same :)

[obra]

EventHandlerResult::OK is something that gets returned by the actual EventHandler, which, in this case, is onLedEffectQuery

This is made more confusing than it should be by me misnaming the callback that prints out the LED mode ledModeHandler_ rather than sendLedModeCallback_ or something like that.

And I think I agree more with your comment down-conversation about moving the 'unnamed mode' guard into the callback than about reusing sendName as the callback.

I didn't realize that with the signature const __FlashStringHelper *name we could also pass in regular in-memory strings and that everything would work.)

(I do think sendName should get refactored, but let's tackle that outside this PR)

[EvyBongers]

This is made more confusing than it should be by me misnaming the callback that prints out the LED mode ledModeHandler_ rather than sendLedModeCallback_ or something like that.

I agree. I'll rename the method

[EvyBongers]

All of them are implemented now. Only thing is that the Tricolor effects (Jukebox and Miami) don't yet support setting a custom name.

$ ../_firmware/kaleidoscope/bin/focus-send led.modes
Off
Rainbow
RainbowWave
Wavepool
ActiveLayerKeys
Chase
solidRed
solidOrange
solidYellow
solidGreen
solidBlue
solidIndigo
solidViolet
Breathe
AlphaSquare
Stalker
Colormap
Heatmap
ActiveLayerColor
DigitalRain
Jukebox
JukeboxAlternate
Miami

[obra]

Reading through the code, it's weird to me that we're ending up with an underscore prefixed lowerCamelCase _ledModeName pointer as an argument to the constructor, when all the other constructor params are snake_case and that we're ending up with a lowerCamelCase protected const __FlashStringHelper *ledModeName; in our LEDModeInterface class.

The style guide isn't super helpful for the constructor args, but for members, it says we do want 'em snake cased and that if they're not public, they should have trailing _s. AccessTransientLEDMode.his the first example in the core I can find with an actual member variable that's protected:

protected: uint8_t led_mode_id_ = 255; /* 255 means uninitialized */ };

I think I'd have expected the constructors to look more like:

TriColor::TriColor(const __FlashStringHelper *led_mode_name, cRGB base_color, cRGB mod_color, cRGB esc_color) { led_mode_name_ = led_mode_name;

and that we should end up with the member looking like this:
protected: const __FlashStringHelper *led_mode_name_;

[EvyBongers]

Reading through the code, it's weird to me that we're ending up with an underscore prefixed lowerCamelCase _ledModeName pointer as an argument to the constructor, when all the other constructor params are snake_case and that we're ending up with a lowerCamelCase protected const __FlashStringHelper *ledModeName; in our LEDModeInterface class.

I'll update that. I mostly focussed on getting code to work and for the parts of the style guide I was already aware of, I've tried to stick to it, but I certainly haven't read the full style guide. Thank you for pointing that out.

The style guide isn't super helpful for the constructor args, but for members, it says we do want 'em snake cased and that if they're not public, they should have trailing _s. AccessTransientLEDMode.his the first example in the core I can find with an actual member variable that's protected:

protected: uint8_t led_mode_id_ = 255; /* 255 means uninitialized */ };

I think I'd have expected the constructors to look more like:

TriColor::TriColor(const __FlashStringHelper *led_mode_name, cRGB base_color, cRGB mod_color, cRGB esc_color) { led_mode_name_ = led_mode_name;

and that we should end up with the member looking like this: protected: const __FlashStringHelper *led_mode_name_;

👍🏻

[obra]

@EvyBongers - just that one question from this last review. Other than that, I just want to run it on hardware, then I'm happy to merge. Thanks so much for your work on this!