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.

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

Defer calling begin() on buses #4312

Merged
merged 1 commit into from
Nov 24, 2024
Merged

Conversation

willmmiles
Copy link
Collaborator

NeoPixelBus requires that all parallel I2S bus members be constructed before any of them call Begin(). Implement this by deferring the Begin() call to the end of the bus initialization process. This replaces the "reinit" logic previously used to work around a similar initialization ordering issue for ESP8266 pins.

Fixes #4301.

This PR introduces a new virtual begin() call on Bus types, although it's only currently used on digital buses. This seemed like a cleaner abstraction than requiring a bunch of type-checking logic at the point of use, and maps more closely to the NeoPixelBus expected call sequence. It also leaves a hook behind for future bus types that might need separate initialization phases.

NeoPixelBus requires that all parallel I2S bus members be constructed
before any of them call Begin().  Implement this by deferring the
call to the end of bus construction.

Fixes Aircoookie#4301.
@netmindz
Copy link
Collaborator

I haven't tested the runtime but nice design.

Maybe just add a comment to the begin interference that explains a little when this is called to help anyone writing a bus implementation what this method is for

@dosipod
Copy link
Contributor

dosipod commented Nov 24, 2024

Unit did not crash once I repeated that pin assignment in #4301 so seems fixed .
However since I was testing I thought to go further until I hit the supposedly limit of 10 but I never got any warning and the page allowed saving but it is broken afterword and all the values as you see in the pic . I did not edit cfg.json nor factory reset the unit which should be the workaround for that . This is similar to what I have seen in this case #4230

image

PR4312_WLED18_cfg.json

Edit: A reboot of the unit seems to have fixed that and outputs went back to 10 only

@softhack007
Copy link
Collaborator

softhack007 commented Nov 24, 2024

@Makuna (hope your reading this) we are still not 100% sure if mixing different led strip types on the same "parallel I2S bus" is supported (esp32), or it may lead to errors and spurious crashes? Our most important use case would be to have both ws2812b (RGB) and sk6812 (RGBW) on the same parallel I2S driver.

It this "safe", or would you say better to use the parallel bus for only one type, and have single RMT busses for other types?

@netmindz netmindz merged commit 2448e2a into Aircoookie:0_15 Nov 24, 2024
20 checks passed
@netmindz netmindz added this to the 0.15.0-final candidate milestone Nov 24, 2024
@Makuna
Copy link

Makuna commented Nov 24, 2024

@Makuna (hope your reading this) we are still not 100% sure if mixing different led strip types on the same "parallel I2S bus" is supported (esp32), or it may lead to errors and spurious crashes? Our most important use case would be to have both ws2812b (RGB) and sk6812 (RGBW) on the same parallel I2S driver.

It this "safe", or would you say better to use the parallel bus for only one type, and have single RMT busses for other types?

The "Feature" can be unique per parallel channel without any concern. So, mixing RGB with RGBW or even 8 bit per element and 16 bit per element isn't an issue.
The "Method" uses the i2s bus speed of the first NPB instanced (constructed) on that i2s bus as the speed for all channels that are latter constructed on that i2s bus. The reset time can be unique per i2s parallel channels without issues. The ramification of this is that not all methods use the same speed. In the NPB file "NeoBits.h" is the list and the key class member is "BitSendTimeNs". You can see that WS2805 has a different value than the WS2812x, but SK6812 has the same as WS2812x. So, if the speed is the same, there is no issue at all. If the speed is different, the only effect is that it will run at the speed of the first one instanced. Should not crash or cause other issues than timing.
NOW, with this all stated, the big caveat is that all parallel channels must be instanced (constructed) before the first Begin() is called and no other are instanced after the first Begin() is called. That could cause issues.

@@ -597,7 +597,7 @@ class PolyBus {
case I_HS_P98_3: busPtr = new B_HS_P98_3(len, pins[1], pins[0]); break;
case I_SS_P98_3: busPtr = new B_SS_P98_3(len, pins[1], pins[0]); break;
}
begin(busPtr, busType, pins, clock_kHz);
Copy link
Collaborator

@softhack007 softhack007 Nov 27, 2024

Choose a reason for hiding this comment

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

@willmmiles

the previous code had begin() with 4 arguments ( including clock_kHz), while the code in the previous reinit is calling PolyBus::begin(_busPtr, _iType, _pins); with 3 args - just wondering if this works as expected, and APA102 busses are still initialized with a custom SPI speed?

(late question, sorry. just spotted it now).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I've put a fix in #4327.

willmmiles added a commit to willmmiles/WLED that referenced this pull request Nov 27, 2024
netmindz added a commit that referenced this pull request Nov 27, 2024
Fix missing clock setting introduce by #4312
netmindz pushed a commit that referenced this pull request Nov 27, 2024
blazoncek pushed a commit to blazoncek/WLED that referenced this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants