apa102: fix end frame #76533
apa102: fix end frame #76533
Conversation
fouge
force-pushed
the
worldcoin/apa102-end-frame
branch
2 times, most recently
from
6954d01 to
3527c83
Compare
There was a problem hiding this comment.
Hi @fouge,
Thanks for this patch. Please see my comments below.
Additionally, can you please improve the commit description message ? The PR description message you wrote is more detailed and understandable. You could use it.
|
ping @fouge :) |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
fouge
force-pushed
the
worldcoin/apa102-end-frame
branch
from
3527c83 to
fe97124
Compare
zephyrbot
requested review from
fabiobaltieri,
kartben,
nashif and
stephanosio
|
@simonguinot sorry for the very late reply, & thank you for the review. |
fouge
force-pushed
the
worldcoin/apa102-end-frame
branch
3 times, most recently
from
6d68ba3 to
9c628d8
Compare
No problem @fouge. Thanks for the update. It looks good. Please see above for one last question. |
drivers/led_strip/apa102.c
Outdated
| * number of clock pulses required is exactly half the total number | ||
| * of LEDs in the string | ||
| */ | ||
| uint8_t ones[((size / sizeof(struct led_rgb)) / 2) + 1]; |
There was a problem hiding this comment.
Wondering if it is really a good idea to allocate a dynamically sized array on the stack, especially since it's just filled with 0xff, this could waste an awful lot of RAM on a long strip.
How about allocating an 0xff array statically up to a certain configurable size? The drawback is then that the maximum strip length would have to be configured at build time but imo that's way better than changing this as potentially blowing up the stack
There was a problem hiding this comment.
Hi @fabiobaltieri. Thanks for spotting this.
@fouge Please can you statically initialize the ones array (with the half the strip length) and pass it via the dev->data pointer ?
There was a problem hiding this comment.
pointers can and should go in the config structure btw, even pointers of non constant data, the pointer itself is going to be constant
SRAM is expensive :-)
There was a problem hiding this comment.
@fabiobaltieri Thanks again. It is confusing and counterintuitive to not put a data buffer in the data structure but in the config structure because the pointer is constant. I think we have made this mistake in the lp50xx and tlc5971 LED and strip drivers.
There was a problem hiding this comment.
Can't say I haven't done it myself. :-)
There was a problem hiding this comment.
@simonguinot @fabiobaltieri I like that approach a lot
I could make the array const with some macro that are available with GNU extension
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Initializers
uint8_t apa102_end_frame_##idx[] = { [0 ... LENGTH] = 0xFF };
not sure we want a specific implementation for GNU though, what do you think?
There was a problem hiding this comment.
wouldn't risk it, the project as a whole aims to be compatible with other compilers that we don't have access to in CI, not a fan of that myself but that's unfortunately something we have to deal with
fouge
force-pushed
the
worldcoin/apa102-end-frame
branch
from
fe60f34 to
2e3f7a0
Compare
fouge
force-pushed
the
worldcoin/apa102-end-frame
branch
2 times, most recently
from
8faaa30 to
d4e8f07
Compare
end frame is used to supply clock pulses so that data goes to last LED in the chain. Thus, it depends on the number of LEDs in the chain. Previously, the number of ones sent into the end frame was hard-coded and limited the usage of the driver to 64 LEDs in the strip. Signed-off-by: Cyril Fougeray <cyril.fougeray@toolsforhumanity.com>
fouge
force-pushed
the
worldcoin/apa102-end-frame
branch
from
d4e8f07 to
d690b3e
Compare
end frame is used to supply clock pulses so that data goes to next led.
It depends on the number of LEDs in the chain.
Previously, the number of ones sent into the end frame was hard-coded and limited the usage of the driver to 64 LEDs in the strip.
This PR fixes that issues by computing the number of bits to send in the end frame