zha: Support switches/buttons/remotes#12528
zha: Support switches/buttons/remotes#12528rcloran merged 1 commit intohome-assistant:devfrom rcloran:zha-button
Conversation
There was a problem hiding this comment.
line too long (81 > 79 characters)
There was a problem hiding this comment.
line too long (84 > 79 characters)
There was a problem hiding this comment.
local variable 'v' is assigned to but never used
There was a problem hiding this comment.
line too long (87 > 79 characters)
There was a problem hiding this comment.
line too long (88 > 79 characters)
There was a problem hiding this comment.
line too long (105 > 79 characters)
There was a problem hiding this comment.
undefined name 'SINGLE_OUTPUT_CLUSTER_DEVICE_CLASS'
There was a problem hiding this comment.
undefined name 'SINGLE_INPUT_CLUSTER_DEVICE_CLASS'
|
@igorbernstein2 : Does this work with your GE switches/dimmers? |
|
You should look at exposing remotes button presses as events since they are state less. |
|
I'm happy to work this to just generate events instead of create a binary_sensor, if there's some consensus that'd be better. I'm not sure how other components in hass deal with devices like this, so happy to take pointers. binary_sensor (or sensor, I guess) has the advantage of having some common bits already wired together, but the disadvantage of not supporting "on, on" type event streams. |
|
I had this discussion when integrating the deConz component. There are some arguments in the pr #10321 |
|
Doesnt seem to work with my GE ZigBee Lighting Switch 45856GE. The switch is detected on startup: However it doesn't seem to update its state when I toggle the switch, I don't see any changes when I evaluate the following template: Which always outputs: For reference here is the output from I should have more time next weekend to play with it. |
try removing the switch from the network with zha.remove and rejoin it. I thinks it creates the binding from client on/off cluster to NCP upon device discovery (if I'm understanding the code correctly) |
Is it possible to have both? I concur with the point of "having everything wired together", but in the same time somewhat miss the flexibility which "on, on" event sequence could offer. |
There was a problem hiding this comment.
move to level command_id should be 0x00.
command_id == 0x01 is "move" command with [MoveMode(enum8), Rate(uint8)] arguments
There was a problem hiding this comment.
Thanks. Not sure why I had 0 there. I've changed it to 0, and added a real implementation of move. Seems to be working well with my Osram dimmer switch.
There was a problem hiding this comment.
should we update the 'state' since this is a "with_on_off" command?
There was a problem hiding this comment.
Never mind. I see now it's being handled in the _move_level. Although per ZCL specs, move, move_to_level, step, stop commands shall be ignored when the device is in 'Off' state.
|
@rcloran so what's the fate of this pull request? any chance it is going to be merged or is there something else in the works? I'm running a private fork of it and I'm pretty happy with it. Just discovered some typos with LevelListener which should be pretty trivial to fix |
There was a problem hiding this comment.
was this meant to be self._level = min(255, max(0, self._level + change)) ?
There was a problem hiding this comment.
Return code from bind and configure_reporting should be evaluated and not just ignored. There is a good change a device not supports it or went to sleep. At least an error log entry would be helpful
There was a problem hiding this comment.
If I remember right, the reason this is wrapped in safe() is that some devices don't support one or the other method of setup. It's logged at info level in safe() -- I'll bump that to warning.
|
@Adminiuga : I agree having both makes sense. I'm going to clean up and merge this today, I'd be happy to review a PR that added events. |
|
|
||
| _domain = DOMAIN | ||
|
|
||
| class OnOffListener: |
There was a problem hiding this comment.
Why nest the listener classes?
There was a problem hiding this comment.
The idea was that they are pretty tightly bound to the Switch class, and are only classes because of the zigpy API. shrug
| # Normally the entity itself is the listener. Base classes may set this to | ||
| # a dict of cluster ID -> listener to receive messages for specific | ||
| # clusters separately | ||
| _in_listeners = {} |
There was a problem hiding this comment.
Having mutable objects as class attributes is dangerous and prone to bugs.
There was a problem hiding this comment.
Looks like these are used as a class wide registry of listeners. Will there never be a problem of one instance overwriting a listener of another instance? Are cluster_id s unique to each entity?
There was a problem hiding this comment.
So it looks like we replace the class attribute with an instance attribute of the same name when instantiating the child entity. So then it's not a class wide registry anymore, which is good. But I'd suggest to rewrite this logic anyway. See my comment about a safer way to handle state update callbacks.
There was a problem hiding this comment.
It's not ever replaced.
~/src/home-assistant/homeassistant/components$ egrep -r '(in|out)_listeners' .
./zha/__init__.py: _in_listeners = {}
./zha/__init__.py: _out_listeners = {}
./zha/__init__.py: cluster.add_listener(self._in_listeners.get(cluster_id, self))
./zha/__init__.py: cluster.add_listener(self._out_listeners.get(cluster_id, self))
./binary_sensor/zha.py: self._out_listeners = {
Anyways, doing in in async_added_to_hass would be better.
There was a problem hiding this comment.
The last line in the grep replaces the class attribute with an instance attribute in the entity name space.
There was a problem hiding this comment.
Uh, I see what you mean. Yes, you're right (except it doesn't need to be an instance attribute).
| self._state = True | ||
| self._level = 255 | ||
| from zigpy.zcl.clusters import general | ||
| self._out_listeners = { |
There was a problem hiding this comment.
It's safer to register state update callbacks in async_added_to_hass entity coroutine. If a callback is called before the entity has been added to home assistant, the call to schedule_update_ha_state will error.
I suggest you define and initialize the listener dicts (_in_listeners and _out_listeners) as instance attributes on the parent class, instead of class attributes. And set the listener items in async_added_to_hass in this child class.
| self._level = 0 | ||
| self._level = min(255, max(0, self._level + change)) | ||
| self._state = bool(self._level) | ||
| self.schedule_update_ha_state() |
There was a problem hiding this comment.
Will these callbacks move_level, set_level etc, be called from a thread and not from within the event loop, ie that's why we're using the sync version of this method?
There was a problem hiding this comment.
None of the current radios run on a different thread. And I think it's reasonable to expect them not to. I'll update.
ghost
removed
the
8 participants
Description:
Tested to work with:
Tested and does not work with:
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass