$ rgrep '#define CRGB' -C3
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-42-#define ROWS 14
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-43-#define LED_COUNT 0
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-44-
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h:45:#define CRGB(r,g,b) (cRGB){b, g, r}
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-46-
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-47-namespace kaleidoscope {
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-48-namespace hardware {
--
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h-11-#define COLS 16
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h-12-#define ROWS 4
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h-13-
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h:14:#define CRGB(r,g,b) (cRGB){b, g, r}
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h-15-
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h-16-class Model01 {
Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h-17- public:
$ (master *=) rgrep 'struct cRGB' -C3
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h-48- * colors where r, g, and b do not have the same value. Each Hardware library
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h-49- * defines a CRGB(r,g,b) macro which returns a literal cRGB with the given values.
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h-50- */
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h:51:typedef struct cRGB cRGB;
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h-52-
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h-53-namespace kaleidoscope {
Kaleidoscope-Hardware/src/Kaleidoscope-Hardware.h-54-/** Kaleidoscope Hardware base class.
--
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-34-
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-35-#include "macro_helpers.h"
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-36-
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h:37:struct cRGB {
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-38- uint8_t r, g, b;
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-39-};
Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/hardware/ErgoDox.h-40-
--
KeyboardioScanner/KeyboardioScanner.h-3-#include <Arduino.h>
KeyboardioScanner/KeyboardioScanner.h-4-#include "wire-protocol-constants.h"
KeyboardioScanner/KeyboardioScanner.h-5-
KeyboardioScanner/KeyboardioScanner.h:6:struct cRGB {
KeyboardioScanner/KeyboardioScanner.h-7- uint8_t b;
KeyboardioScanner/KeyboardioScanner.h-8- uint8_t g;
KeyboardioScanner/KeyboardioScanner.h-9- uint8_t r;
I see that the ErgoDox port defines the CRGB macro to initialize the input values in the BGR order, but the struct cRGB declaration is still in the RGB order. Is this a bug? (Probably not discovered in practice because the ErgoDox doesn’t have LEDs.)
Why is the struct cRGB defined for the Model01 in the KeyboardioScanner library and not in the Kaleidoscope-Hardware-Model01 like the ErgoDox struct defined under Kaleidoscope-Hardware-EZ-ErgoDox?
Kinda, because indeed, the EZ doesn’t have per-key LEDs. If/when that changes, we’ll fix the bug.
Because on the Model01, it is the ATTiny on the other half that implements both key scanning and controlling the LEDs; while on the ErgoDox, there’s only one MCU, so the cRGB definition can go into the -Hardware module.
To further clarify the Model01 case, since the ATTiny does all the LED business, it makes sense for it to dictate what cRGB looks like. -Hardware-Model01 just sends data over to the other half, it would be wrong for it to declare cRGB - not to mention that KeyboardioScanner would need to depend on the hardware library, not the other way around, which would be another mistake.
Those points are debatable. It is reasonable to structure things this way, but it’s not the only way that makes sense (a matter of opinion). The Kaleidoscope-Hardware-Model01 module doesn’t need to have the same color object structure as used in the scanner firmware that runs on the ATTiny chips in the Model01, as long as the translation from user/plugin code to ATTiny hardware takes place somewhere. I would argue that the way it has been done is unnecessarily confusing.
Perhaps, but it’s efficient. Rearranging bytes before sending them over the wire is very expensive (been there, done that, the resulting latency wasn’t pretty; it’s ok for cases where the updates are infrequent, but this kills any non-trivial animations). Doing it on the ATTiny even more so. So we either keep the layout the same, or incur a hefty processing cost.
If the color structure was a class with a proper constructor, it wouldn’t need to re-order any bytes, and the confusing byte order required by the particular wiring of ATTiny scanners on the Model01 would be completely hidden from client code.
Are you sure about this? Before sending values to the ATTiny, KeyboardioScanner reads the value of each cRGB byte individually, and looks up the value to send in the gamma8 array; it doesn’t directly copy the bytes in order to the ATTiny.
I just tested out flipping the byte order in a version of KeyboardioScanner, and I don’t see any human-detectable difference in any of the LED modes in the standard firmware from doing so (except that the colors are now different). If I recall correctly from my earlier experimentation with LEDs, the big performance hit came when sending the data over I2C from the MCU to the scanners; any byte-shuffling before that would be trivial by comparison.
The LED modes in the standard firmware are pretty light and may not show up any difference in their own display, except for something else you can test to see whether an LED mode is using up much CPU:
turning the breathing LED mode on, try moving the mouse pointer by keeping a mouse button held, and
switching to the solid mode, try it again, and
with the byte rearrangement, try it again.
Even the simple breathing effect creates a drag on the mouse point as you can see here, and you can use this to test whether the byte reordering does the same or more.
I don’t see any obvious “drag” on the mouse pointer movement in any LED mode, either with or without byte-reordering. I could try cycle-time measurements for a more definitive assessment of the impact, but I don’t have time for that today.
Hrm. It has been a while I last tested the byte-swap impact. We are doing a few things differently since then, so @merlin might be right, and the impact might be much smaller than what I remember.
To be clear, the byte-reordering that I’m trying is unquestionably less efficient, and the firmware is larger by 62 bytes; I’m just questioning the assertion that it’s a significant difference in performance. I’ve tested out other changes to how color values could be stored that should be even slower, and never noticed user-visible effects from doing so. That said, I was never looking for that type of problem, so I may not have been stressing it enough to notice.