Add BinaryOutputCluster switch support - #467
Conversation
c3f2e6c to
e76d70b
Compare
|
Thanks for making a PR for this feature! For something that'll be used by DIY devices, I think it would be best to gate this behind the presence of a def _is_supported(self) -> bool:
if self._cluster_handler.description is None:
return False
return super()._is_supported()This should get all of the failing entity unit tests passing as well. |
1fe0aed to
ed91294
Compare
Good point! I've added your suggested code. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #467 +/- ##
==========================================
+ Coverage 96.88% 96.90% +0.02%
==========================================
Files 63 63
Lines 10361 10409 +48
==========================================
+ Hits 10038 10087 +49
+ Misses 323 322 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
~~Also noticed that the async_update implementation (for polling with homeassistant.update_entity service call) seems to be missing. Already the case for the OnOff variant. Only the ConfigurableAttributeSwitch has it (for config entities).
Just a reminder to add this when we also add it for OnOff. It's not that crucial, but we should still implement it in the future.~~
EDIT: Never mind, it's enough to implement async_update in the cluster handler and we magically call it from PlatformEntity if implemented:
zha/zha/application/platforms/__init__.py
Lines 461 to 468 in b4f1386
| if self._binary_output_cluster_handler.present_value is None: | ||
| return False | ||
| return bool(self._binary_output_cluster_handler.present_value) |
There was a problem hiding this comment.
Now that we use bool(.present_value), we should be able to remove the present_value is None check, since that will return False. That also fixes the coverage issue 😄
There was a problem hiding this comment.
True. I was keeping it around for future review because I'm not sure if I agree with this behavior in general, it seems like unavailable would be a better state?
There was a problem hiding this comment.
Yeah, I also thought about that. It makes sense for sensors and so on (and I think we unfortunately have this same behavior for binary sensors), but I'm not sure how this behaves with switches. Can you still turn on/off a switch if it's unavailable in HA?
Like, if it's an issue that present_value or on_off or whatever just wasn't read properly during pairing OR if there's some buggy device that just [makes zigpy] return None until the value was written once, do we really want to block doing that? (Assuming unavailable switch in HA means we can't turn on or off).
There was a problem hiding this comment.
Since async_update is apparently already supported, since we magically call that method in the cluster handler from the base PlatformEntity here, mind also copying the async_update part from one of the OnOff switch tests to get coverage for that in the cluster handler (if you're not already working on that 😄)?
Otherwise, this looks good now.
This PR adds support for BinaryOutputCluster as a switch. Currently, binary output clusters are correctly set up but not exposed into home assistant at all. Created switches look like this: