MQTT fixes #779
MQTT fixes #779
Conversation
if there was no change, isMqttDirty() will be false and no packet will be sent
|
Hi @ccutrer ! Thanks for help |
|
I'd say it's not ready yet. My ESP crashes quite a bit. I know when it happens, so it doesn't bother me too much, but I haven't had time to debug it. Once I finally get around to it, I'll be sure to post a compiled image for you. |
|
Hi ! Thank you very much ! |
|
Turns out my issues weren't with the ESP crashing, but with an unrelated issue in openHAB. So this PR is ready. See https://github.com/ccutrer/esp8266_milight_hub/releases/tag/20221021.1 for a pre-compiled binary for a NodeMCU v2 style board. |
| staleGroups.push(bulbId); | ||
| // if this was to group 0, we need to enqueue an update for all child groups as well | ||
| if (bulbId.groupId == 0) { |
There was a problem hiding this comment.
Just group 0? Depending on your remote type you have 4 or 8 groups.
Also, this code reads as:
If group == 0 then update this current bulb in all groups with this new state.
While i'd expect it to read as:
if this bulb is a group, set the state to all bulbs in this group to this new state.
I could be completely wrong here, just explaining how i read the code.
There was a problem hiding this comment.
yes, you're reading it incorrectly. a BulbId consists of a deviceId and a groupId (and a deviceType, which tells us if the device supports 4 or 8 groups). the deviceId corresponds to the particular fut089 remote for example. groupId 0 is a special group that means "all groups on this device". so this code says "if we're marking group 0 stale, find out how many groups this particular device type supports, and mark all groups on that device (note that it's reusing bulbId, without modifying its deviceId) as stale as well".
note that all "bulbs" are a "group", since you can synchronize as many physical bulbs as you want to a given group id (but not 0!). For example, I have a FUT089. The groups are:
- Group 1: a single channel LED strip controller
- Group 2: 4 RGB+CCT downlights
- Group 3: 12 RGB+CCT downlights
- Group 4: 4 RGB+CCT adjustable downlights
- Group 5: two RGB+CCT LED strip controllers
- Group 6: a RGB+CCT LED strip controller
- Group 7: a single channel LED strip controller
- Group 8: 4 RGB+CCT downlight
And then Group 0 is the All group.
There was a problem hiding this comment.
Thank you for that explanation, that's much appreciated!
Perhaps a comment in code explaining this a little would be helpful?
The people actively hacking on this code probably know it and to them - like you - it's "common knowledge" by now. But newcomers are going to have a hard time understanding what's going on.
Side note, not a review at all. Just a comment from a fellow developer :)
There was a problem hiding this comment.
The people actively hacking on this code
Lulz. Soo.... no one apparently. Two commits in the last two years. This PR having gotten no response from @sidoh in a year and a half. That's why I added the precompiled binary as a release in my fork. If @sidoh indicates interest in actually merging this, I'll worry about it then.
ensuring that the state in MQTT is always up to date