Add optimistic option. #4542 #4572
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello @Koenkk. The base branch of this pull request has been updated to the |
sjorge
reviewed
Oct 6, 2020
| @@ -238,7 +239,7 @@ class EntityPublish extends Extension { | |||
| this.publishEntityState(resolvedEntity.settings.ID, msg); | |||
| } | |||
|
|
|||
| if (result && result.membersState) { | |||
| if (result && result.membersState && optimistic) { | |||
There was a problem hiding this comment.
Is this also true for say a group of blinds? I can't figure it out by reading more of the code around here if it is. I think so?
There was a problem hiding this comment.
It depends if you setup reporting, if not you wont' get state updates.
There was a problem hiding this comment.
Nice, I'll be setting this on my groups then.
That won't fix #3461 but it will remove one of the updates.
There was a problem hiding this comment.
It is indeed working for groups, but the effect is weird:
with 'optimistic' set to false:
- send /set to group
- reporting updates bulb1 and bulb2
- no update on the group at all
without 'optimistic' or set to true:
- send /set to group
- update of group
- reportining updates bubl1
- update of group
- reporting updates bulb2
- update of group
Is this the expected behavior?
I would have expected the following with optimistic set to false:
- send /set to group
- reporting for bulb1
- update of group
- reporting of bubl2
- update of group
There was a problem hiding this comment.
Not expected behaviour, I expect what you mentioned last, can you provide the debug logging of this?
|
My group config: This is when controlling the group via a remote: This is when controlling via mqtt No update of the group either way. |
Koenkk
added a commit
that referenced
this pull request
Oct 8, 2020
|
This seemed to conflict with the |
|
Something still seems of after that change That was with a remote And using mqtt... Neither seem to be correct? The one only coming in via reporting still don't update the group topic, while they should (there is nothing optimistic or oportunistic about it) Using mqtt... now gets an update... but the state is wrong? it is ON when it should be off and off when it should be on! |
When optimistic is used for groups, an attribute report won't update the group state (it's documented here: https://www.zigbee2mqtt.io/information/groups.html#state-changes). This was implemented because of #764 (comment) (I agree |
|
So there is no way to skip the update on set and only take reporting updates for groups? |
|
Currently not indeed and the existing |
|
When this is revisited we should also fix debounce for groups, there is an issue for that too. Do we have a list tracking these thing somewhere? |
|
We don't, but should do and tag this with 'breaking change'. About the debounce for groups, I don't think this is breaking, so this can be implemented in Zigbee2MQTT 1.x? |
|
Yes, a tag would be nice... can we then also have the stalebot leave those alone? I assume those will be long lived. #3461 is the debounce one, I had a brief looked but got super confused on how it works for devices. |
|
Done |
Koenkk
added a commit
that referenced
this pull request
Oct 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.