New Library: DycoLed for Arduino

Hello robtillaart, thank you for the review of my code. Here are the answers you are (hopefully) looking for :wink:

so if I fill in 0 I get MAXLEDS? Why???

The answer is easy. What sense would it make to control zero leds? I agree with you that MAXLEDS is a bit oversized, could change this to 1 or leave it like so.

[improve]
The type of _ledcount can be (byte) -- idem for setLED()

Agreed.

[missing]
It is not possible to set the pins, now you are using 1 and 2 where pin 1 is allready part of the serial port....

This is the price of an early release ... I've tested the code only on one ATtiny85 right now. Last week I changed from software bit-banging to hardware output (USI). In a later (the next?) version of the library I will add support for real SPI and software mode.

[missing]
no method to request the #leds or the interval set in the constructor.

"get...()" methods will be added. The thing with the interval is not fully featured at the moment (delay() does it's job, but not very efficient).

The usage of capital like LEDCOUNT is in C++ primary used for #defines like you did MEXLEDS
for variables lowerCase is normally used [/quote]
Tried to get the quantity of leds by template, but it didn't work out. Therefore I set this as a #define some days ago an didn't change it back after the usage as a variable. Is fixed.

[bug 2x]
void DycoLed::setLed(unsigned int led, byte value[3]) {
if (led <= LEDCOUNT) {

if LEDCOUNT == 1 the only valid number is 0

Not right, 1 would be true, too. (smaller or equal to LEDCOUNT)

[shorter]
void DycoLed::update() { .........

The shorter version does it's job very well :slight_smile: Was so long during testing the basic commands for the leds, temp vars are not needed anymore.

begin() ...
WHY 256 ??? LEDCOUNT should do

Had a problem whlie testing the leds. I changed the quantitiy to a smaller one than connected to my ATtiny85 ... the DycoLeds have the "feature" that they go to maximum brightness at start ... without initialization, all leds without this first command would drain 60 mA (for each led). Therefore this huge initialization in begin()

[idea]
I would add a boolean called changed; in the class that is set to true if setLed() was called and reset if values were written. If changed == false you dont need to send all the data as it was not changed.

Same as for the interval ... will be added when usage without delay() is fully featured.

I would merge clearAll() and resetAll() to

Not sure why I left both in the code ... maybe it is a remain from testing the code. clearAll() could be useful for a fluid color changing. resetAll() may be used for shutting down everything. Will search for the reason in my notes.

You have a double administration one for easy setting and one for sending.
You can win 60+bytes footprint by removing the ledMessage Array's. You could then also remove the complete update() function.

Yes and no :wink:
I had the just-in-time calculation in a very early stage of writing the code. But I thougt that pre-calculating all values and just sending them out at the desired time would be better. I will think about this again later.

missing the function getLed(&r &g & b);

What do you mean exactly? To get the values of a specific led ( getLed(byte led) with an array return)? Or to search for one led with the matching color values (returning index of led)?