WWA strip support & parallel I2S for S2/S3 (bumping outputs from 5/4 to 12) #4484

blazoncek · 2025-01-12T14:31:34Z

Bus wrapper modifications

NeoPixelBus update 2.8.3
automatic selection of appropriate single I2S bus (X1xxxxxxMethod)
removed I2S0 on ESP32 (used by AudioReactive)
renumbered internal bus numbers (iType)
added buffer size reporting (prototype, requested @Makuna to add appropriate methods to NPB)

Bus modifications

WWA strip support
bus initialisation rewrite (moved to WS2812FX::finalizeInit())
optional parallel I2S (ESP32, S2 & S3)
ABL speed improvements (no float)

This modification will allow unrestricted use of AudioReactive usermod on ESP32 and ESP32-S3. S2 is still prone to conflict but since it is not best suited for large LED counts in combination with AudioReactive the compromise may be acceptable.

WWA strip support may need WW/CW channel swap as I do not have one for test. A(mber) channel is not supported.

Fixes #552

- NeoPixelBus update 2.8.3 - automatic selection of appropriate I2S bus (`X1xxxxxxMethod`) - removed I2S0 on ESP32 (used by AudioReactive) - renumbered internal bus numbers (iType) - added buffer size reporting Bus modifications - WWA strip support - bus initialisation rewrite - optional parallel I2S (ESP32, S2 & S3)

DedeHai · 2025-01-13T06:21:00Z

I tried this PR on an S2 lolin: cannot get it to save correctly LED settings if more than 5 outputs are set. It saves but config gets screwed up a little, next time in LED settings, all outputs are gone except the first and it is reset (to a backup I assume)

blazoncek · 2025-01-13T09:08:59Z

Hmm, that may be due to change in initialisation procedure. Will investigate.
In the meantime, if you can manually update cfg.json that should work.

- speed improvements in ABL - verbose debugging

@Makuna

- Implement vector in bus manager - Memory calculation according to explanation from @Makuna - Prefer 8 RMT before 8 I2S on ESP32 (fixes Aircoookie#4380) - speed improvements in ABL - verbose debugging - get bus size from NPB (prototype) - Parallel I2S output bugfix - automatic selection of appropriate I2S bus (`X1xxxxxxMethod`) - removed I2S0 on ESP32 (used by AudioReactive) - renumbered internal bus numbers (iType) - added buffer size reporting

- Segment::setName() - S2 limits - bus debug macros - remove cctBlending from strip

Remove logs form Git tracking

blazoncek · 2025-01-19T11:46:53Z

Latest update will also fix #4380 until resolved in NeoPixelBus.

TripleWhy · 2025-01-19T13:21:48Z

wled00/cfg.cpp

-        doInitBusses = true;  // finalization done in beginStrip()
-      }
+
+      busConfigs.push_back(std::move(BusConfig(ledType, pins, start, length, colorOrder, reversed, skipFirst, AWmode, freqkHz, useGlobalLedBuffer, maPerLed, maMax)));


emplace_back is generally preferable to push_back(move(construct())))

I (personally) would also have tried to simply emplace a default BusConfig, and then modify its values instead of creating a bunch of values, then copying them all over to a BusConfig

Good idea.
BusConfig is fixed in size and has simple constructor though.

Well in that world I would have removed the explicit constructor. Better for performance anyway, and I imagine better for code size too?
I don't see how BusConfig's size would have any impact here.

ijspear · 2025-01-19T18:22:37Z

Latest update will also fix #4380 until resolved in NeoPixelBus.

Amazing! Are your artifacts using this latest code or should I compile myself?

blazoncek · 2025-01-19T19:18:14Z

You can use artifacts from this PR. Click on ✅ of last commit.

- experiment with std::unique_ptr

willmmiles

Overall thoughts: definitely a worthwhile change set. I'm happy to see fewer and fewer C-style pointers.

Longer term, I'd like to squash down all the parallel I2S logic to be inside BusDigital; it bugs me a little bit that this needs to be exposed and handled elsewhere, as in some sense it's an "execution detail" of the digital output subsystem. That said, it's also clear that there's value in having a staged config->plan->construction process which permits more "global" analysis and optimization over the plan, and it's not obvious to me how to devolve that to local knowledge (like how to manage digital output resource allocation).

willmmiles · 2025-01-20T00:30:58Z

wled00/FX_fcn.cpp

@@ -613,6 +613,19 @@ Segment &Segment::setPalette(uint8_t pal) {
  return *this;
 }

+Segment &Segment::setName(const char *newName) {


Could we use a String for the segment name? It already handles all the dynamic allocation and whatnot.

Internally?
I added a from String conversion function.

Yes? eg. String name, instead of char* name.

wled00/FX_fcn.cpp

wled00/wled.h

willmmiles · 2025-01-20T19:29:24Z

wled00/bus_manager.cpp

  if (Bus::isVirtual(bc.type)) {
-    busses[numBusses] = new BusNetwork(bc);
+    //busses.push_back(std::make_unique<BusNetwork>(bc)); // when C++ >11


No need to wait - make_unique requires no new compiler features, it's simple enough we can bring a shim implementation:

#if __cplusplus >= 201402L using std::make_unique; #else // Really simple C++11 shim for non-array case; implementation from cppreference.com template<class T, class... Args> std::unique_ptr<T> make_unique(Args&&... args) { return std::unique_ptr<T>(new T(std::forward<Args>(args)...)); } #endif

Experimented by using -std=gnu++14 as suggested by @TripleWhy and it worked.
Unfortunately code bloated by 1.4kB.

Wonder if this would make code any smaller.

Using this shim and removing delete increases code by 452B.
Compared to gnu++14 a huge win if it yields same safety.

I dug around online for flags to enable newer C++ standards, and turned up a bunch of forum posts from people recommending against it - something something ESP32 platform not tested, doesn't support it, strange crashes, etc. I was surprised - I wouldn't've anticipated trouble from a whole-system build - but it didn't seem to be worth chasing down how applicable those old posts still were for this one case.

I did a quick test with the shim above, and the results were within few hundred bytes for most targets (though the older ESP32 platform really suffers - we need to drop that ASAP)

Target a98685d 244e0c9 Cost

nodemcuv2 879847 879879 32

esp8266_2m 878183 878215 32

esp01_1m_full 886715 886763 48

nodemcuv2_160 879851 879883 32

esp8266_2m_160 878187 878219 32

esp01_1m_full_160 886719 886767 48

nodemcuv2_compat 866399 866447 48

esp8266_2m_compat 864719 864767 48

esp01_1m_full_compat 873347 873379 32

esp32dev 1347273 1347709 436

esp32dev_V4 1250905 1251001 96

esp32_eth 1357633 1358029 396

lolin_s2_mini 1280009 1280137 128

esp32c3dev 1152868 1152938 70

esp32s3dev_16MB_opi 1150241 1150337 96

esp32s3dev_8MB_opi 1150241 1150337 96

esp32s3_4M_qspi 1148601 1148697 96

esp32_wrover 1171514 1171506 -8

What happens when you inline it?

If it's not inlined, the compiler would have to generate one function per template argument it's called with, so might might be the number of bytes of your table times the number of classes that use this (potentially times number compilation units using it)

There are only 4 calls in BusManager::add() function. Each for one of the classes: BusDigital, BusPwm, BusOnOff and BusNetwork.
There may be additional classes in the future (BusUsermod or BusVirtual).

I was thinking more long term: What happens if make_unique starts to get used in more places

blazoncek · 2025-01-20T21:22:15Z

What I also wanted to do is convert BusManager into namespace. But got linker errors that _milliAmpsMax was defined multiple times.

willmmiles · 2025-01-20T21:54:53Z

What I also wanted to do is convert BusManager into namespace. But got linker errors that _milliAmpsMax was defined multiple times.

A missing extern in the header maybe? I think making BusManager in to a namespace is a good idea, too, but it could be a second PR.

blazoncek · 2025-01-21T10:14:20Z

Got it working as a namespace. No real benefits that I can see, though.

blazoncek · 2025-01-21T16:56:27Z

@willmmiles @TripleWhy would such approach be ok?

I am not sure about BusManager::getBus() as it does no checking if vector is empty and assumes that busses[0] always exist (it should).

willmmiles · 2025-01-21T18:09:16Z

@willmmiles @TripleWhy would such approach be ok?

I am not sure about BusManager::getBus() as it does no checking if vector is empty and assumes that busses[0] always exist (it should).

Re conversion to namespace: Alas, exposing a couple internal variables is the price we pay for inlining. Best we can do without link-time optimization.

Re getBus(): Hm, it's probably not ideal to return something strange given an invalid input. It's not unreasonable to expect the callers of getBus() to have to explicitly handle any invalid inputs. I think there are two real options: either (a) permit getBus() to indicate failure (for example, return a Bus* which can be nullptr if the index is bad -- this is one of those rare cases for bare pointers); or (b) decide that getBus(index) where index>=getNumBusses() is UB and the caller is responsible for making sure that doesn't happen.

TripleWhy · 2025-01-21T18:22:01Z

Your question is just about line 473 in bus_manager.h in that commit, right? (I could say more about the changes of that commit if you want, but I'm not sure you'd like it :)

If it is ok depends on how sure you can be that busses is never empty when getBus is called. The safe approach would be to check for emptiness, and have an empty dummy bus somewhere that you can return (typically a static variable in the cpp file, but you'd need to move the implementation to the cpp file as well). That costs a bit of performance and space.
This is better suited for functions that return const references though. Another way would be to return an optional. (Or a pointer. You'd have to make it clear somehow that the caller doesn't own the pointer; and that it might nullptr.)

You could also make it the caller's responsibility to never call getBus when busses is empty. In that case, you should make that clear with at least a comment.
What I would usually do in such a case is add an assert as the first line in the function. That serves a way to formalize the precondition contract, and could be enforced by some contract framework if you want. But more importantly, the assert always runs on developer and tester machines, but never in production. I'm not sure how feasible such an approach is for something like WLED however.

Also note that typically you'd define two getBus functions, one that returns a mutable reference and one that returns a const reference. That works much better when you have non-static class methods, but can also be done with two separate function names. I guess that would also increase code size, however.

blazoncek · 2025-01-21T18:55:40Z

how sure you can be that busses is never empty

It is supposed to have 1 bus at least unless compilation overrides made impossible bus configuration (GPIO conflict).

make it the caller's responsibility to never call getBus when busses is empty

That is already in place as all loops go: for (i=0; i < BusManager::getNumBusses(); i++) which is equal to busses.size().

All in all it may be best to stick with returning a pointer and do explicit nullptr check everywhere.
Unfortunately it cannot be const as some calls to modifier functions are necessary.

but I'm not sure you'd like it :)

Go ahead unless it is personal. 😉 Best done inline in the commit.

blazoncek · 2025-01-23T06:35:17Z

After this is merged (if it is) I have another set of updates ready to allow unlimited virtual buses.

blazoncek added the enhancement label Jan 12, 2025

blazoncek added 5 commits January 13, 2025 17:27

Bus creation bugfix

cc011e3

- speed improvements in ABL - verbose debugging

Merge fixes & updates

7daea18

- Segment::setName() - S2 limits - bus debug macros - remove cctBlending from strip

Update NPB to 2.8.3

d3954b9

Remove logs form Git tracking

Merge branch 'bus-improvements' into parallel-I2S

1c4ba20

blazoncek requested a review from willmmiles January 19, 2025 11:47

DedeHai mentioned this pull request Jan 19, 2025

Implement WS2811 WW/CW #4457

Open

TripleWhy reviewed Jan 19, 2025

View reviewed changes

Remove numBusses, RMT idle bugfix

a98685d

- experiment with std::unique_ptr

willmmiles reviewed Jan 20, 2025

View reviewed changes

Merge branch 'main' into parallel-I2S

24082d1

netmindz mentioned this pull request Jan 23, 2025

Update Platform, Toolchain, Compiler/GCC, C++ Versions #4509

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WWA strip support & parallel I2S for S2/S3 (bumping outputs from 5/4 to 12) #4484

WWA strip support & parallel I2S for S2/S3 (bumping outputs from 5/4 to 12) #4484

blazoncek commented Jan 12, 2025 •

edited

Loading

DedeHai commented Jan 13, 2025

blazoncek commented Jan 13, 2025

blazoncek commented Jan 19, 2025

TripleWhy Jan 19, 2025

TripleWhy Jan 19, 2025

blazoncek Jan 19, 2025

TripleWhy Jan 19, 2025

ijspear commented Jan 19, 2025

blazoncek commented Jan 19, 2025

willmmiles left a comment

willmmiles Jan 20, 2025

blazoncek Jan 20, 2025

willmmiles Jan 20, 2025

willmmiles Jan 20, 2025

blazoncek Jan 20, 2025

blazoncek Jan 20, 2025

willmmiles Jan 20, 2025

TripleWhy Jan 21, 2025 •

edited

Loading

blazoncek Jan 21, 2025

TripleWhy Jan 21, 2025

blazoncek commented Jan 20, 2025

willmmiles commented Jan 20, 2025

blazoncek commented Jan 21, 2025

blazoncek commented Jan 21, 2025

willmmiles commented Jan 21, 2025

TripleWhy commented Jan 21, 2025 •

edited

Loading

blazoncek commented Jan 21, 2025

blazoncek commented Jan 23, 2025

Target	`a98685d`	`244e0c9`	Cost
nodemcuv2	879847	879879	32
esp8266_2m	878183	878215	32
esp01_1m_full	886715	886763	48
nodemcuv2_160	879851	879883	32
esp8266_2m_160	878187	878219	32
esp01_1m_full_160	886719	886767	48
nodemcuv2_compat	866399	866447	48
esp8266_2m_compat	864719	864767	48
esp01_1m_full_compat	873347	873379	32
esp32dev	1347273	1347709	436
esp32dev_V4	1250905	1251001	96
esp32_eth	1357633	1358029	396
lolin_s2_mini	1280009	1280137	128
esp32c3dev	1152868	1152938	70
esp32s3dev_16MB_opi	1150241	1150337	96
esp32s3dev_8MB_opi	1150241	1150337	96
esp32s3_4M_qspi	1148601	1148697	96
esp32_wrover	1171514	1171506	-8

WWA strip support & parallel I2S for S2/S3 (bumping outputs from 5/4 to 12) #4484

Are you sure you want to change the base?

WWA strip support & parallel I2S for S2/S3 (bumping outputs from 5/4 to 12) #4484

Conversation

blazoncek commented Jan 12, 2025 • edited Loading

DedeHai commented Jan 13, 2025

blazoncek commented Jan 13, 2025

blazoncek commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijspear commented Jan 19, 2025

blazoncek commented Jan 19, 2025

willmmiles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TripleWhy Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazoncek commented Jan 20, 2025

willmmiles commented Jan 20, 2025

blazoncek commented Jan 21, 2025

blazoncek commented Jan 21, 2025

willmmiles commented Jan 21, 2025

TripleWhy commented Jan 21, 2025 • edited Loading

blazoncek commented Jan 21, 2025

blazoncek commented Jan 23, 2025

blazoncek commented Jan 12, 2025 •

edited

Loading

TripleWhy Jan 21, 2025 •

edited

Loading

TripleWhy commented Jan 21, 2025 •

edited

Loading