Fix OnOff client cluster commands not updating attribute cache
#635
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #635 +/- ##
==========================================
+ Coverage 97.10% 97.32% +0.21%
==========================================
Files 62 62
Lines 10710 10716 +6
==========================================
+ Hits 10400 10429 +29
+ Misses 310 287 -23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We may need a similar fix for alarm control panels, though they were already broken somewhat before these changes. |
tests/test_binary_sensor.py
Outdated
| on_off_ch.cluster_command( | ||
| 106, # tsn | ||
| OnOff.ServerCommandDefs.on_with_timed_off.id, | ||
| [0, 1800, 0], # on_off_control, on_time, off_wait_time | ||
| ) |
There was a problem hiding this comment.
We probably shouldn't directly call the cluster_command method on the client cluster handler.
Instead, we can use listener_event with "cluster_command" or use make_zcl_header:
# Option 1: call `listener_event` with `"cluster_command"`, similar to existing `async_test_iaszone_on_off` test
cluster.listener_event(
"cluster_command", 1, OnOff.ServerCommandDefs.on_with_timed_off.id, [0, 1800, 0]
)
# Option 2: Use `make_zcl_header`
hdr = make_zcl_header(
OnOff.ServerCommandDefs.on_with_timed_off.id, global_command=False
)
cluster.handle_message(hdr, [0, 1800, 0])What do we prefer? handle_message with make_zcl_header, so we're not relying on listener_event cluster_command staying the same over time?
There was a problem hiding this comment.
I've opted to use handle_message with make_zcl_header in the tests now. It's different to the async_test_iaszone_on_off test, which also checks cluster handler functionality, but that can be changed in the future. This also tests more of the "whole stack".
| def cluster_command(self, tsn, command_id, args): | ||
| """Handle commands received to this cluster.""" | ||
| cmd = parse_and_log_command(self, tsn, command_id, args) | ||
|
|
||
| if cmd in ( | ||
| OnOff.ServerCommandDefs.off.name, | ||
| OnOff.ServerCommandDefs.off_with_effect.name, | ||
| ): | ||
| self.cluster.update_attribute(OnOff.AttributeDefs.on_off.id, t.Bool.false) | ||
| elif cmd in ( | ||
| OnOff.ServerCommandDefs.on.name, | ||
| OnOff.ServerCommandDefs.on_with_recall_global_scene.name, | ||
| ): | ||
| self.cluster.update_attribute(OnOff.AttributeDefs.on_off.id, t.Bool.true) | ||
| elif cmd == OnOff.ServerCommandDefs.on_with_timed_off.name: | ||
| should_accept = args[0] | ||
| on_time = args[1] | ||
| # 0 is always accept 1 is only accept when already on | ||
| if should_accept == 0 or (should_accept == 1 and bool(self.on_off)): | ||
| if self._off_listener is not None: | ||
| self._off_listener.cancel() | ||
| self._off_listener = None | ||
| self.cluster.update_attribute( | ||
| OnOff.AttributeDefs.on_off.id, t.Bool.true | ||
| ) | ||
| if on_time > 0: | ||
| self._off_listener = asyncio.get_running_loop().call_later( | ||
| (on_time / 10), # value is in 10ths of a second | ||
| self.set_to_off, | ||
| ) | ||
| elif cmd == "toggle": | ||
| self.cluster.update_attribute( | ||
| OnOff.AttributeDefs.on_off.id, not bool(self.on_off) | ||
| ) | ||
|
|
||
| def set_to_off(self, *_): | ||
| """Set the state to off.""" | ||
| self._off_listener = None | ||
| self.cluster.update_attribute(OnOff.AttributeDefs.on_off.id, t.Bool.false) |
There was a problem hiding this comment.
I've removed the entire cluster_command handling for the server OnOff cluster handler. Unfortunately, it was never covered by tests, hence we didn't catch this.
I don't think it can be used at all for the server handler, so should be fine to remove.
|
Tested these changes with an IKEA TRADFRI motion sensor and it works properly again. |
2 participants
#615 removed
handle_on_off_output_cluster_exception, so theOnOffClusterHandleris no longer used as a cluster handler for client/output devices. However, thecluster_commandlogic foron_with_timed_offwas only present there.This PR adds it to the
OnOffClientClusterHandlerto fix the issue of motion binary sensors not updating on cluster commands. The logic is also removed from the (server)OnOffClusterHandler, as that should (hopefully) not be used anymore.