Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow 9 LED pins with audioreactive #3380

Closed
wants to merge 3 commits into from
Closed

Allow 9 LED pins with audioreactive #3380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32c4045
Allow 9 LED pins with audioreadctive
softhack007 Sep 21, 2023
ad863dd
change order of I2S drivers
softhack007 Sep 21, 2023
11a6678
fix typo in comment
softhack007 Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion wled00/bus_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,8 @@ class PolyBus {
#else
// standard ESP32 has 8 RMT and 2 I2S channels
if (num > 9) return I_NONE;
if (num > 7) offset = num -7;
if (num == 8) offset = 2; // first use I2S#1 (so #0 stays available for audio)
if (num == 9) offset = 1; // use I2S#0 as the last driver
#endif
switch (busType) {
case TYPE_WS2812_1CH_X3:
Expand Down
4 changes: 2 additions & 2 deletions wled00/const.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@
#define WLED_MIN_VIRTUAL_BUSSES 4
#else
#if defined(USERMOD_AUDIOREACTIVE) // requested by @softhack007 https://github.com/blazoncek/WLED/issues/33
#define WLED_MAX_BUSSES 8
#define WLED_MIN_VIRTUAL_BUSSES 2
#define WLED_MAX_BUSSES 9
Copy link
Collaborator

@blazoncek blazoncek Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@softhack007 softhack007 Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#define WLED_MIN_VIRTUAL_BUSSES 1
#else
#define WLED_MAX_BUSSES 10
#define WLED_MIN_VIRTUAL_BUSSES 0
Expand Down
Loading