Allow 9 LED pins with audioreactive

[softhack007]

In response to user feedback - people seem to miss their last two led pins in the audioreactive version.

NPB cannot use I2S#0 because audioreactive needs it. But I2S#1 is still availeable on esp32. So this PR addsI2S#1 to the digital led driver, allowing for 9 instead of 8 pins.

@blazoncek please review, and if OK for you then I propose to merge it into main. I've (intensively) tested that running audio and in parallel using I2S#1 for NPB does work.

[blazoncek]

Don't get me wrong but if it is possible I'd much rather see we do not add #ifdef USERMOD_AUDIOREACTIVE into lowest level file (wrapper).

In fact why would you want to restrain LED buses if audio is compiled in? It may be used as a network receiving device only without any I2S device attached.

Instead I would prefer that usermod would inform user (in settings) that he/she is using I2S device for LEDs which is prohibiting audio input functionality (if he/she is using too many outputs).
IMO that would be much cleaner approach which would allow same binary to be used with 10 LED outputs and no audio input or 9 LED outputs and audio input.

[softhack007]

The reason for #ifdef USERMOD_AUDIOREACTIVE in bus_wrapper is that I need to modify the rules, so bus nr 9 will go to I2S#1 instead of I2S#0 in normal WLED without AR.

In fact why would you want to restrain LED buses if audio is compiled in? It may be used as a network receiving device only without any I2S device attached.

The problem is that audioreactive setup does not check if I2S#0 is in use for LEDs, so we decided last year to restrict the user from 10 to 8 led pins, to avoid conflicts. In the mean time I found out that people could still have 9, just not 10.

I don't know how AR setup could detect that NPB NeoEsp32I2s0800KbpsMethod is in use - maybe there is a way. I can't say exactly what will happen when AR initializes I2S#0 while in use for LEDs - maybe we will get a crash, or maybe leds on pin 9 will just switch off.

IMO that would be much cleaner approach which would allow same binary to be used with 10 LED outputs and no audio input or 9 LED outputs and audio input.

Agreed it will be a cleaner solution, but with a lot more changes needed compared to this easy improvement. Maybe the clean solution could be implemented in the next major release? What do you think?

Edit: maybe pinManager could be extended to manage allocation of I2S (unit 0 and unit 1), so that both AR and bus_wrapper could check if it's available?

Edit2: just confirmed that AR needs I2S#0, we cannot use I2S#1 as some features are not possible with I2S#1

[softhack007]

Another idea: if we change the default order of drivers, so pin9=I2S#1 and pin10=I2S#0, then there would be no need for #ifdef in bus_wrapper.

if (num == 8) offset = 2;
if (num == 9) offset = 1;

Would always work, no matter if 9 or 10 led pins are possible.

[blazoncek]

This #ifdef may be unnecessary as well. Just allow 10 digital buses.

A check in Audioreactive's setup() could be added as follows:

int digitalBuses = 0;
for (int i=0; i<busses.getNumBuses(); i++) if (busses[i]->getType() >= 16 && busses[i]->getType() <= 31) digitalBuses++;
if (digitalBuses>9) return;

[softhack007]

Thanks, that check is actually an easy way to verify. I would still need a new #ifdef in audioreactive, because a similar check for bus 5 is needed for -S2.

I'm still not sure it's a good solution from user experience point of view. Because AR would simply fail to initialize, without information what is wrong. Maybe I'll need to add a flag and put something into info page, but space there is limited so we needed a short but absolutely clear message text.

Need to think about it ...

[blazoncek]

There is no problem if usermod requires core dependency, but core should not be dependant on usermods. That's how I see it.
Info dialog can be used with a simple note: "AR Inactive: Too many LED outputs used."

And perhaps instead of returning in setup() set enabled to false.

[softhack007]

if (busses[i]->getType() >= 16 && busses[i]->getType() <= 31) digitalBuses++;

As I'm not really in love with "magic numbers" that will change over the years - are there any constants I could use instead? This macro is in const.h, do you think its the correct one?

#define IS_DIGITAL(t) ((t) & 0x10) //digital are 16-31 and 48-63

[blazoncek]

No, as you do not want SPI variants included.
const.h defines bus types and anything 16>=x<=31 is single pin digital.
This may change of course in the future, but such magic numbers are also used in JS for UI so there is no way around them.

[netmindz]

Even if a magic number needs to be defined in both C and JavaScript, they should still be constants not magic numbers

[blazoncek]

The magic numbers are defined in const.h. Look at LED types TYPE_xxxxx.
JS does not have such definitions. Yet.

[softhack007]

@blazoncek I'm still wondering if its good to change UI and core behaviour like this, literally 1 week before a release is scheduled. Maybe this PR is better suited for 0.15 🤔 ??

Just for clarity, allowing I2S#1 for LEDs itself is a safe change IMHO, because i've already tested a lot to ensure that using I2S#1 for LEDs while audio is running will work.

[blazoncek]

@blazoncek I'm still wondering if its good to change UI and core behaviour like this, literally 1 week before a release is scheduled. Maybe this PR is better suited for 0.15 🤔 ??

0.14.1 may be better suited if you want to push it faster. Otherwise 0.15 may be good candidate as I already pushed AR palettes there. Still waiting for @netmindz response, but once @Aircoookie approves new collaborators (if he does) he may be able to work on it directly.

[softhack007]

0.14.1 may be better suited if you want to push it faster.

OK, then let's postpone this one, and add it to 0.14.1. The current situation with 8 digital LED pins is not optimal, however starting to revise user interactions now - including new error messages that need explanation - is also not a good idea IMHO.

[netmindz]

Still waiting for @netmindz response, but once @Aircoookie approves new collaborators (if he does) he may be able to work on it directly.

What are you waiting on me for?

[softhack007]

What are you waiting on me for?

I think @blazoncek was referring to his new proposal for audioreactive palettes.

[softhack007]

superseded by commit 20ed81c (0_15 branch).

[blazoncek]

@softhack007 I did not ask if you are ok with the change (removal of #ifdef) but I was considering using audioreactive as a standard build in a near future and remove non-audio binary. What do you think?