drivers: led_strip: apa102: brightness & end frame #74073
Conversation
fouge
commented
Jun 11, 2024
fouge
commented
- use scratch byte to set brightness
- fix end frame in case more than 64 LEDs
use scratch byte in struct led_rgb to set brightness Signed-off-by: Cyril Fougeray <cyril.fougeray@toolsforhumanity.com>
end frame used to supply clock pulses so that data goes to next led Signed-off-by: Cyril Fougeray <cyril.fougeray@toolsforhumanity.com>
There was a problem hiding this comment.
Hi @fouge,
Thanks for this PR.
It is not correct to use the scratch byte to pass the brightness. It is a hack :) As specified in the API, this is simply a placeholder for some drivers, to help serialize 3 RGB bytes into 4 bytes (wire format). Drivers are not free to assign the usage they want to this parameter.
Please, can you explain what is the gain in supporting the global brightness register for this chip ? You already have 255 levels of brightness for each color channel...
Note that the ->update_channels() API method could be implemented to offer control of more registers to users.
| BUILD_ASSERT(sizeof(struct led_rgb) == 4); | ||
| uint8_t ones[((size / sizeof(struct led_rgb)) / sizeof(uint8_t) / 2) + 1]; | ||
|
|
||
| memset(ones, 0xFF, sizeof(ones)); |
There was a problem hiding this comment.
Is there a bug with the current code ?
There was a problem hiding this comment.
yes, not enough 1 are sent in case there are more than 64 LEDs in the strip. The following LED after the 64th are not actuated.
It's not correctly documented in the APA102 datasheet
There was a problem hiding this comment.
Understood. Please fill a bug report and submit a separate PR to fix it.
There was a problem hiding this comment.
Hi @fouge. Are you moving forward on this ?
| * number of clock pulses required is exactly half the total number | ||
| * of LEDs in the string | ||
| */ | ||
| BUILD_ASSERT(sizeof(struct led_rgb) == 4); |
There was a problem hiding this comment.
This is not a problem, but it is customary not to write BUILD_ASSERT inside a function.
I think it would be better to put it inside APA102_DEVICE macro.
There was a problem hiding this comment.
#76533 split into another PR to remove how brightness is being used
|
superseded by #76533 |
