Skip to content
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.

Sign up for GitHub

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

Add optimistic option. #4542 #4572

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Add optimistic option. #4542 #4572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/extension/publish.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ class EntityPublish extends Extension {
if (topic.type === 'set' && converter.convertSet) {
logger.debug(`Publishing '${topic.type}' '${key}' to '${resolvedEntity.name}'`);
const result = await converter.convertSet(actualTarget, key, value, meta);
if (result && result.state) {
const optimistic = !options.hasOwnProperty('optimistic') || options.optimistic;
if (result && result.state && optimistic) {
const msg = result.state;

if (endpointName) {
Expand All @@ -238,7 +239,7 @@ class EntityPublish extends Extension {
this.publishEntityState(resolvedEntity.settings.ID, msg);
}

if (result && result.membersState) {
if (result && result.membersState && optimistic) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

@Koenkk Koenkk Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends if you setup reporting, if not you wont' get state updates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I'll be setting this on my groups then.
That won't fix #3461 but it will remove one of the updates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not expected behaviour, I expect what you mentioned last, can you provide the debug logging of this?

for (const [ieeeAddr, state] of Object.entries(result.membersState)) {
this.publishEntityState(ieeeAddr, state);
}
Expand Down
11 changes: 11 additions & 0 deletions test/publish.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,17 @@ describe('Publish', () => {
expect(zigbeeHerdsman.groups.group_2.command).toHaveBeenCalledWith("genOnOff", "on", {}, {});
});

it('Shouldnt publish new state when optimistic = false', async () => {
const device = zigbeeHerdsman.devices.bulb_color;
const endpoint = device.getEndpoint(1);
settings.set(['devices', device.ieeeAddr, 'optimistic'], false);
await MQTT.events.message('zigbee2mqtt/bulb_color/set', stringify({brightness: '200'}));
await flushPromises();
expect(endpoint.command).toHaveBeenCalledTimes(1);
expect(endpoint.command).toHaveBeenCalledWith("genLevelCtrl", "moveToLevelWithOnOff", {"level": 200, "transtime": 0}, {});
expect(MQTT.publish).toHaveBeenCalledTimes(0);
});

it('Should handle non-valid topics', async () => {
await MQTT.events.message('zigbee2mqtt1/bulb_color/set', stringify({state: 'ON'}));
await flushPromises();
Expand Down