SpenceKonde / ATTinyCore

Arduino core for ATtiny 1634, 828, x313, x4, x41, x5, x61, x7 and x8
Other
1.53k stars 302 forks source link

Why is the ATTinyCore version of tinyNeoPixel missing ColorHSV()? #733

Closed H3wastooshort closed 1 year ago

H3wastooshort commented 1 year ago

It would be nice to have it included in some cases. Its only in the megaTinyCore version. This confused me quite a lot, especially because its in both header files.

https://github.com/SpenceKonde/megaTinyCore/blob/master/megaavr/libraries/tinyNeoPixel_Static/tinyNeoPixel_Static.cpp

SpenceKonde commented 1 year ago

It isn't missing in github version - but it was added to all versions of tinyNeoPixel after the last board manager release (when I became aware that Adafruit had added some functions). The version that has the updated tinyNeoPixel is in 2.0.0, but that isn't released yet. This is also the version that gets rid of the need for tinyNeoPixel Port tools submenu (the original author claimed it wasn't possible to meet the timing constraints with ST instead of OUT. But that's only because he was not all that good at AVR asm, and hadn't seen the way to make it work), and fixes constraints (the only reason the old version and any of the adafruit ones work is that C++ classes kneecap the optimizer and prevent it from inlining calls to methods, even when only one member of a class is ever instantiated). If it was able to inline those methods, all of them would break horribly, because they got the read/write-ness of the constraints backwards: Using indirect load with postincrement or predecrement requires the pointer register to be specified as a read-write operand, not a read-only one. Incrementing a pointer register absolutely counts as changing it. Meanwhile, it specified that the pointer to the PORT registers was read-write. But we don't change that! We change the thing it's pointing too, but that's a volatile special function register, so the compiler is required to load it from the dataspace every time it needs it, but not where the pointer points. Hence, it's read only! And they specified "+r" (read/write, any register) for the register used for counting bits - but they populate it with ldi (load immediate), and the immediate instructions only work on upper registers*, so the constraint should have been "+d".

However since it's a C++ class method those problems never actually manifest, because it can't inline them, hence has to use the normal function call API. Thus it uses call-used registers first, which are all upper registers. And the arguments to the function are passed as copies, so nothing bad happens as a result of writing to something we promised to only read from.

In any event, this issue has already been corrected and the fix is in 2.0.0.