-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix for #4643 - Missing MQTT updates #4771
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
Conversation
WalkthroughThe code changes introduce a call to Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings
wled00/ws.cpp (4)
wled00/wled_server.cpp (3)
⏰ 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
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 (
|
What is required to get this merged since it's been 2 months out of curiosity? |
@ekristen some testing... and reporting success. |
I'll attempt to compile and flash today and report back. Thanks! |
No need to compile (unless you need usermods or other custom stuff) just use CI builds. Click on the green checkmark at last commit. |
@blazoncek I'm happy to report I was able to install and this resolves my issue, now when calling the JSON API through HA, I see the MQTT messages go out. Thanks! Hope to see this hit main soon, for now I'm going to run the alpha. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK for now, but I do think we need to do a thorough review of the update notification subsystem in the not too distant future. It doesn't make sense for the various sources making state changes having to be responsible for deciding which (types of) clients get notified.
Fixes #4643
Summary by CodeRabbit
New Features
Bug Fixes