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

[WIP] New plugin: UnderglowControl #551

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@algernon
Copy link
Member

algernon commented Jan 24, 2019

The bulk of this is the UnderglowControl plugin, modeled after LEDControl. The KBD4x keyboard is the first one to support this, and I ported a few of the existing LED effects over.

Documentation is missing, but creating a PR makes it easier to review naming, the API, etc. I'll write the docs once we're happy with how things look.

algernon added some commits Jan 24, 2019

hardware/KBD4x: Add support for the WS2812 underglow
This configures up an instance of the WS2812 driver, to drive the underglow on
the keyboard. This adds the raw API only.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
New plugin: UnderglowControl
Inspired by the `LEDControl` plugin, this plugin serves a similar purpose, but
instead of driving per-key RGB LEDs, it is for RGB underglow, as the name
implies.

The API is slightly different than LEDControl's, and is intentionally kept
simple. There is no support for going back and forth between underglow effects
yet, for example.

Hardware that implements underglow, need to set a new define in their main
header: `KALEIDOSCOPE_HARDWARE_HAS_UNDERGLOW`. Without this set, the plugin
becomes a no-op.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
hardware/KBD4x: Enable UnderglowControl support
Signed-off-by: Gergely Nagy <algernon@keyboard.io>
LEDEffect-Rainbow: Add an Underglow variant
This lifts out the core of the rainbow effects into a common ancestor class,
rebuilds the existing LEDEffects on top of it, and adds two new plugins:
`UnderglowRainbowEffect` and `UnderglowRainbowWaveEffect`, that implement the
same effect, but for underglow LEDs.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
LEDEffect-Chase: Add an Underglow variant
Signed-off-by: Gergely Nagy <algernon@keyboard.io>
LEDEffect-SolidColor: Add an Underglow variant
Signed-off-by: Gergely Nagy <algernon@keyboard.io>
LEDEffect-Breathe: Add an Underglow variant
Signed-off-by: Gergely Nagy <algernon@keyboard.io>
IdleLEDs: Add support for Underglow too
If we have Underglow LEDs, turn them off when idle, too.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
LED-ActiveLayerColor: Add an Underglow variant
Signed-off-by: Gergely Nagy <algernon@keyboard.io>

@algernon algernon requested a review from obra Jan 24, 2019

@algernon algernon referenced this pull request Jan 25, 2019

Open

Syncing up with Dygma #552

0 of 4 tasks complete
@mattvenn

This comment has been minimized.

Copy link
Contributor

mattvenn commented Jan 25, 2019

looks like a great start, thanks for your work on this.

uint8_t b;
};

#define CRGB(r, g, b) (cRGB){g, r, b}

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

I feel like this should be a separate PR -- it's adding color support to the 4x, not really adding underglow to kaleidoscope

This comment has been minimized.

@algernon

algernon Jan 25, 2019

Author Member

The reason I opted to add color (well, underglow) support to kbd4x in the same PR is because UnderglowControl relies on the hardware parts to work, and it felt weird having a dependency without anything to satisfy it.

I can lift the KBD4x part out with little effort though, so I'll do that.


#include "kaleidoscope/hardware/ATMegaKeyboard.h"
#include "kaleidoscope/driver/led/WS2812.h"

using Color = kaleidoscope::driver::led::color::GRB;

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

...same as above. But also, now I wonder if we could add helpers for the standard color orders, to reduce the chance of implementation code drift. So that you could just declare the color order and get your CRGB, cRGB, etc. It might also make it easier when we have to do cRGBW...

This comment has been minimized.

@algernon

algernon Jan 25, 2019

Author Member

Yup! I have a half-baked branch that does just that. I'd keep that separate, because that's a fairly invasive change.

if (Kaleidoscope.millisAtCycleStart() < end_time_)
return EventHandlerResult::OK;

if (::Kaleidoscope.has_leds && !::LEDControl.paused) {

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

I think this is a small change that can land without the underglow PR

This comment has been minimized.

@algernon

algernon Feb 1, 2019

Author Member

I started to extract this into a separate PR, but I decided not to for the time being. Reading back the discussion here, I have the feeling it will not be needed. The change is here to support UnderglowControl, but if we bake underglow control into LEDControl, then this is a needless exercise, at least in its current form.

::LEDControl.set_all_leds_to(CRGB(0, 0, 0));
::LEDControl.syncLeds();

::LEDControl.paused = true;
}
if (::UnderglowControl.has_leds() && !::UnderglowControl.isPaused()) {
::UnderglowControl.setColor(0, 0, 0);

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

I feel weird that this is "setColor(0,0,0)" and the LEDControl version is set_all_leds_to(CRGB(0,0,0)).

(LEDControl should at least grow setAllLedsTo() and this API and that one should use the same name and calling convention)

This comment has been minimized.

@algernon

algernon Jan 25, 2019

Author Member

Agreed. My thinking here was to play with names, to come up with something better than what we have in LEDControl (and then transition LEDControl over to the new name too). I'm not really fond of setCrgbAt for example, because it has the typename in the name too.

Btw, setColor(CRGB(0, 0, 0)) also works, and so does set_all_leds_to(r, g, b).

::LEDControl.set_all_leds_to(CRGB(0, 0, 0));
::LEDControl.syncLeds();

::LEDControl.paused = true;
}
if (::UnderglowControl.has_leds() && !::UnderglowControl.isPaused()) {
::UnderglowControl.setColor(0, 0, 0);
::UnderglowControl.sync();

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

If this is called sync, then the version in LEDControl should also be called sync

if (::UnderglowControl.has_leds() && !::UnderglowControl.isPaused()) {
::UnderglowControl.setColor(0, 0, 0);
::UnderglowControl.sync();
::UnderglowControl.pause();

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

LEDControl and this should have parity. If this is going to be a method here (which I like), it should get added to LEDControl at the same time.

This comment has been minimized.

@algernon

algernon Jan 25, 2019

Author Member

Ok. I didn't want to touch LEDControl yet, before we agree on naming & API, happy to add the new methods there!

::LEDControl.paused = false;
::LEDControl.refreshAll();
}
if (::UnderglowControl.has_leds() && ::UnderglowControl.isPaused()) {

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

has_leds() and isPaused() look different. Feels weird.

This comment has been minimized.

@algernon

algernon Jan 25, 2019

Author Member

Good point. Should be hasLeds() I think, right?

::LEDControl.paused = false;
::LEDControl.refreshAll();
}
if (::UnderglowControl.has_leds() && ::UnderglowControl.isPaused()) {
::UnderglowControl.resume();

This comment has been minimized.

@obra

obra Jan 25, 2019

Member

LEDControl should grow resume()

@obra

This comment has been minimized.

Copy link
Member

obra commented Jan 25, 2019

I've left a bunch of small notes inline, but as I've read through the code, I've started to wonder if it would make more sense to have a universal concept of a 'bank' of LEDs, with LEDControl and UnderglowControl both inheriting from, implementing or using it.

A bank of LEDs may or may not have its own driver. (In the underglow keyboard I was designing, underglow and key LEDS were driven by the same IC)

Effects would 'run on' a given bank of LEDs

It'd be kind of crazy to have duplicate effects and implementations for, say, under-key, underglow, and side-glow.

Happy to talk more about this

@algernon

This comment has been minimized.

Copy link
Member Author

algernon commented Jan 25, 2019

My original thinking (#376, previously keyboardio/Kaleidoscope-LEDControl#15) was similar, but the other way around: a common API for both, instead of a common ancestor. A common ancestor makes more sense. I'll give that idea a try next time I'm working on Underglow.

@obra

This comment has been minimized.

Copy link
Member

obra commented Jan 26, 2019

@algernon

This comment has been minimized.

Copy link
Member Author

algernon commented Jan 26, 2019

That's pretty much what I ended up with before I drifted off to sleep last night. Something along these lines:

class LEDBank : public kaleidoscope::Plugin {
 public:
  LEDBank();

  const int8_t ledCount();
  const boolt isPerKey();
  void sync();

  void setColorAt(uint8_t index, cRGB color);
  void setColorAt(uint8_t index, uint8_t r, uint8_t g, uint8_t b);
  void setColorAt(byte row, byte col, cRGB color);
  void setColorAt(byte row, byte col, uint8_t r, uint8_t g, uint8_t b);
  void setColor(cRGB color);
  void setColor(uint8_t r, uint8_t g, uint8_t b);
  cRGB getColorAt(uint8_t index);
  cRGB getColorAt(byte row, byte col);

  EventHandlerResult beforeReportingState(); 
};

Methods that take row,col would be no-ops for banks that aren't per-key (could use a better name for .isPerKey() though).

On top of this, we'd have something like:

class LEDControl : public kaleidoscope::Plugin {
 public:
  LEDControl();

  const int8_t bankCount();
  LEDBank &getBank(int8_t bank);
  void useBanks(LEDBank banks[]);
  void selectBank(int8_t bank);

  // setColorAt, getColorAt, setColor, isPerKey(), ledCount() and sync(), as above
};

The setColorAt(), etc methods would use the bank selected with selectBank() (defaulting to bank 0). Effects would grow a .selectBank(int8_t index) method, which would store the index within the effect, and when it needs to do something with the leds, it'd do ::LEDControl.getBank(bank_).setColorAt(...).

To make things easier, we could also have an enum of banks, defined by the hardware libs:

enum class LEDBankName {
  PERKEY,
  UNDERGLOW,
  SIDEGLOW,
  NOT_SUPPORTED
};

If a board doesn't support a particular group, it can define the group to have the same value as NOT_SUPPORTED. This means that we could do something like ::LEDControl.getBank(UNDERGLOW).setColorAt(...) and be it almost a no-op if underglow is not supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment