Regression: Flickering when /presets.json is accessed

[gunjambi]

What happened?

Whenever /presets.json is retrieved, my leds flicker partially in random colors. This is mostly detectable if there is HomeAssistant in the network, which polls /presets.json every 10 seconds.

To Reproduce Bug

curl <IP>/presets.json

Expected Behavior

No flickering

Install Method

Self-Compiled or other

What version/release of MM WLED?

a7aa9eb

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

Anything else?

I've bisected the regression to a7aa9eb

Before this commit, the I can see the effect pause a momentarily (akin to skipping some frames) when /presets.json is accessed. After this commit, garbage gets rendered.

---

I also tried to see if sending the files in smaller chunks to see if that would help. I mutated the async file response as follows. This made downloading slow but did not fix the flickering. So this one is a dead end:

 size_t AsyncFileResponse::_fillBuffer(uint8_t *data, size_t len){
+  len = std::min(len, (size_t)128);
   return _content.read(data, len);
 }

Code of Conduct

• I agree to follow this project's Code of Conduct

[softhack007]

@gunjambi thanks for the "detective work" 👍

This is the commit when we moved from the very old framework (arduino 1.0.6, esp-idf 3.3.x) to the newer framework (arduino 2.0.9, esp-idf 4.4.4).

Not sure what we can do now, because going back to the old framework is not an option.

The problem with presets.json (triggered by HA) is well-known, but we only have a solution for boards with PSRAM, where we buffer the complete file in PSRAM to avoid very frequent file reading.

Maybe I can find a way to add a pause before reading presets.json, however there are several code paths for reading the file and it's not clear to me atm which of them would benefit from a slowdown.

[gunjambi]

Okay so polling presets.json is a bad idea because it's causing these problematic LittleFS flash reads.

I wonder if we could fix the polling in HA instead. The polling is performed by https://github.com/frenck/python-wled/blob/main/src/wled/wled.py#L259 to keep presets up to date.

Can you foresee any issues if we were to propose a patch where this python library would first check the fs.pmt field ( fs_info[F("pmt")] = presetsModifiedTime; ) in /json, and only fetch presets.json when the timestamp changes?

One possible issue is that presetsModifiedTime is 0 if there haven't been changes on that boot. So a user could change presets, boot the wled, and the integration would not notice this. But this can be detected by checking if uptime field decreases.

Do you see any concerns with this plan?

[softhack007]

Do you see any concerns with this plan?

Sounds interesting - for performance reasons, we could add a new json endpoint like /json/presetInfo that will just provide the necessary info - uptime and presetModifiedTime. This would be faster than serialising all the state, statistics and other info which is included in /json/info.

@netmindz @willmmiles as you both have more experience with HA integration, what do you think?

[gunjambi]

I'm not too keen on the idea of adding a new API. My issues with it are:

• json-wled's update() is already fetching /json and is exposing all the information retrieved. To keep the behavior, the new API would need to include most of the same information anyway.
• We would need some version detection logic, adding complexity.
• It will not provide value for existing WLED users. Some users, such as myself, will not be updating WLED since the new version is rendering a glitchy mess. I'm perfectly happy with my current custom setup.
• Continuing that, propagating a new WLED version to users takes years. All the projects here move very slowly and I believe users, such a myself, will rarely if ever update WLED.

I believe HA is calling the update() every 10 seconds to get the preset updates (I'm just guessing since there's no other reason I can think of).
json-wled is already listening for the changes using websocket but presets are not included in WS stream.
If we are able to add the "pmt changed -> fetch presets" logic in json-wled's WS listener, we could remove the polling from HA.
This means the cost of the HTTP api doesn't matter since we would be calling it only once, and then just listen to the WS.

[softhack007]

The websocket interface is "not my area", so I can't say much about that.

I understand you reasons about how long it takes to promote a new API to all clients, and what you say makes sense. But it's not very different from adding a new attribute into an existing endpoint. HA would need a fallback solution any way. And adding a special endpoint is not much effort in wled.

The reason I was proposing to add a special json endpoint is performance. /json/info collects a lot of stuff already, and some of the calls in serializeInfo are also known to cause flicker, especially this line

WLED-MM/wled00/json.cpp

Line 1118 in f9c7828

root[F("freeheap")] = ESP.getFreeHeap();

So polling json/info instead of reading presets.json might just replace one source of flicker with another source of flicker.

[willmmiles]

@netmindz @willmmiles as you both have more experience with HA integration, what do you think?

As usual, the "root-est" issue for IO-related random color flickering is interrupt latency; doing flash IO causes FreeRTOS scheduler events, which in turn block handler interrupts long enough to miss the timing requirements for LED chips. The most effective solution is to use a known flicker free hardware driver, such as the I2S or RMTHI drivers. (Note that the I2S driver has also proven to be a little bit off with its timing, which can cause problems for clone LED drivers -- see wled#5264 for the latest research. We are continuing to investigate possible driver improvements.) Using a timing-stable output driver will fix incorrect colors appearing.

That said: even with a functional hardware driver, it remains the case that the extra CPU load caused by HA's polling may still cause judder (frame delays), particularly on single-core chips such as ESP32-S2 and ESP32-C3. We will still need a solution to this. (Though i will reiterate that without a stable hardware driver, there is no solution that will guarantee non-flashing output - the best you might get is "works for me most of the time").

In that vein, my recommendation would be to add ETag cache headers to the presets.json endpoint so a client can be sent a 304 Not Modified without the filesystem hit. It'd be much cheaper than storing the whole file in RAM, and we already have a layer to use those tags for the static content. This would improve the interface performance for any regular browser as well.

Using ETags will make the actual HTTP transaction tiny -- a few bytes out, a few bytes back -- and almost certainly an imperceptible performance cost. If we find after that that the polling is still causing enough of a performance delay, we could consider adding the current ETag value to the JSON status endpoint. (Though I'd also like to do a thorough review of the internal notification system before committing to that!)

[softhack007]

The most effective solution is to use a known flicker free hardware driver, such as the I2S or RMTHI drivers

@willmmiles thanks for your ideas - while @gunjambi seems to start looking into this ETags thing, I've started an experiment to back-port your RMTHI driver (WLED-MM PR #308). Would be nice if you can take a look.

RMTHI does compile in MM now; tomorrow I can test it with a "known flickering" fixture.

[softhack007]

@gunjambi I've ported the RMTHI driver (from @willmmiles) back to WLED-MM in PR #308, and my own tests look good.

you seem to have LEDs that are very sensitive to timing. Can you test PR #308 with your hardware?
You need one of the "V4" builds - most likely you're already using one. The expected behaviour with #308 is that there is no glitching any more during file access.

Would be nice to have some early feedback for the PR, because I had to upgrade NeoPixelBus to 2.7.9 for the new driver, and its not the first time that an update of NPB introduces regressions for some users.

Edit: the new driver is only for esp32 and esp32-S3.

[gunjambi]

@softhack007 I tested PR #308 with esp32_4MB_V4_S build and I confirm the glitching is fixed.

(Weirdly though the dashboard html api calls now emit garbage bytes and I can't use the dashboard anymore, but the API works fine. Possibly because I didn't erase the flash.)

[softhack007]

@gunjambi thanks for testing. Not sure what your dashboard does - i've disabled a few wait loops (not needed any more due to the RTMHI driver). Possibly API responses arrive too fast now for your dashboard html?

Or it could be causes by a mismatch of firmware / bootloader / flash filesystem - this has caused other users some troubles, and the only known solution is to erase all flash, then do a clean re-install via USB.

[softhack007]

In that vein, my recommendation would be to add ETag cache headers to the presets.json endpoint so a client can be sent a 304 Not Modified without the filesystem hit. It'd be much cheaper than storing the whole file in RAM, and we already have a layer to use those tags for the static content. This would improve the interface performance for any regular browser as well.

@willmmiles I think we should still go for ETag cache headers for presets.json (and other "download" files) - maybe with a first implementation in upstream, because all this web browser stuff is "not my area" 😅