POC implementation for deferred settings #4808
Conversation
Add a basic function queue to the main loop, and have settings-set operations defer operation to it. Rejigger the JSON lock as a binary semaphrore, as it needs to pass ownership from one task to another. First step towards a global fix for wled#4779.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
FYI, the crashes in 1D->2D expansions I saw may have had another main cause, I think I forgot to mention that in the PR: |
|
I did not yet look at the code but the general approach of not handling settings at any random moment has a lot of benefits. One of them is that it (probably) allows for cleaner heap allocation when FX change. I ran many tests and the main issue with heap fragmentation is that themporary buffers are placed all over and leave holes when freed. |
When a state changing request is received, defer handling that request until a known point on the main loop task. This mitigates any possible cases of editing memory out from underneath the render task (or, for that matter, any other integration or usermod that's serviced on the main task).
To implement this, the JSON buffer lock is reimplemented as a binary semaphrore, as it needs to pass ownership from one task to another. This required contexts which can back off and retry to indicate such to the kernel. There might be better solutions, but this sufficies for demonstrating the overall approach.
This is IMO the quickest path to safety re #4779, though not the most performant. It soft-guarantees that we don't alter the FX state while rendering, since all those operations are now handled on the one task. I'd talked about setting up a mutex around WS2812FX in #4779; this approach is simpler to implement now and buys us time to arrange a more granular locking approach in the future.
The biggest drawback is that high FPS systems might observe lag during setting-set operations. Still that's better than risking crashes, I think!