Make sync send/receive modes configurable at compile-time #4810
Conversation
Simplifies device setup by being able to configure the desired sync behavior at compile-time.
WalkthroughThe update modifies the initialization of global boolean variables controlling notification synchronization in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: in the wled codebase, syslog configuration variables (sysloghost, syslogport, syslogprotocol, syslog...Applied to files:
📚 Learning: in wled, the wled_max_panels macro is intentionally defined as a fixed constant value (18) with no r...Applied to files:
📚 Learning: in wled, `bri` is a global variable used for brightness control....Applied to files:
📚 Learning: esp8266 and esp32 platforms have different maximum segment name lengths in wled, which can cause tru...Applied to files:
📚 Learning: in wled, essential configuration files that require backup have short, controlled names (like "/cfg....Applied to files:
📚 Learning: in wled's fx.h, mode_count represents the highest fx_mode_ id + 1, not the total count of fx_mode_ d...Applied to files:
📚 Learning: in wled, maximum segment name length varies by platform: - esp8266: 32 characters (wled_max_segname_...Applied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (2)
✨ 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 (
|
|
We should not be adding compile flags for configuration, if you want your device to have a certain config after you flash it, then flash a filesystem with suitable config file, we need to reduce the number of build flags, not adding more |
|
@netmindz The issue with WLED's code is that the "Sync" button in WLED's main UI does absolutely nothing whatsoever since WLED 0.15 and newer. Activate "Sync" = Nothing happens. No code runs. Nothing gets activated. My preferred fix would literally be:
That's a two-line change and it makes the Sync button work as users expect it: Clicking the Sync button will make their other WLED devices on the network sync with the master's settings. It's also a harmless change since nothing is sent/processed unless the user clicks the main UI's "Sync" button. What do you think about solving it that way? Old: Lines 737 to 740 in 3f90366 New: WLED_GLOBAL bool notifyDirect _INIT(true); // send notification if change via UI or HTTP API
WLED_GLOBAL bool notifyButton _INIT(true); // send if updated by button or infrared remote
WLED_GLOBAL bool notifyAlexa _INIT(false); // send notification if updated via Alexa
WLED_GLOBAL bool notifyHue _INIT(false); // send notification if Hue light changes |
|
Flashing a filesystem has several issues:
So I'd prefer to fix the Sync button issue in WLED's code instead, either via compile-flags if there's no agreement on what the defaults should be (hence this PR), or alternatively by fixing WLED to have sane defaults for the Sync button. :) Defaults should be something that matches the vast majority of users and how they are likely to use WLED. With syncing, and pressing the "Sync" button, the majority of users expect that it begins syncing with other WLED devices on the network. That can be fixed by improving the defaults. And obviously, while I've fixed this issue personally, I want it to work out of the box for everyone who visits wled.me and downloads a binary, because that puts the project in a better light. Having a non-functional "Sync" button by default in the main UI doesn't really make sense. I'd be happy to make the PR to fix the defaults to make the Sync UI button work. ❤️ |
|
I agree that users have found the new behaviour confusing, there have been several reports of if being "broken" but I'm still confused as to why it was changed and what the expected new behaviour is (out of the box) |
|
@netmindz Yeah. While I was investigating this and talking to people, @blazoncek said that the original pre-0.15 behavior was that clicking on "Sync" in the UI automatically enabled the relevant sync flags internally. The latest WLED code uses a two-tier system: Tier 1:
Lines 662 to 663 in 3f90366
Tier 2:
Lines 737 to 740 in 3f90366 My proposal is to change those flag values to something that makes sense by default:
WLED_GLOBAL bool notifyDirect _INIT(true); // send notification if change via UI or HTTP API
WLED_GLOBAL bool notifyButton _INIT(true); // send if updated by button or infrared remote
WLED_GLOBAL bool notifyAlexa _INIT(false); // send notification if updated via Alexa
WLED_GLOBAL bool notifyHue _INIT(false); // send notification if Hue light changesWith that change, the WLED UI's "Sync" button would make sense to users again. When people's master "Sync" device changes its settings, other WLED devices in the network would then follow it as users expect. I think it should be improved like this, so that WLED is pre-configured to sync with other WLED devices whenever a user clicks on "Sync". I have not created a pull request to change the defaults because I wasn't sure if you want to do that, but I honestly think the change above should be made so that WLED is pre-configured to work out of the box. The current situation where people ask "is it broken?" isn't ideal. :) I can make a quick PR if it helps, or anyone can commit the change directly. I think the improvement should also happen in 0.15.2. |
Previous behaviour was tied to "notify on direct change". So if you wanted to "notify" other "stuff" you were always notifying everything (aka sync). I agree to set "notify on direct change" to true by default as it will no longer also trigger sync behaviour. |
|
@blazoncek Ahh that makes sense. So in the past, setting them to "true" would have made the devices always sync by default. When things were refactored to have the master on/off switch and granular control, this flag's default was simply forgotten. I was sick for a few days. I am preparing the pull requests to fix this for 0.16 and 0.15.2 now, since it's a "major" user-facing bug in expected behavior, and it would make user's lives easier if Sync just works. :) Edit: Pull requests are ready: |
Update: Replaced with a different PR instead: #4844
Simplifies device setup by being able to configure the desired Sync button behavior at compile-time.
Usage:
Summary by CodeRabbit