Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #3930 +/- ##
==========================================
+ Coverage 91.18% 91.19% +0.01%
==========================================
Files 334 334
Lines 10862 10891 +29
==========================================
+ Hits 9904 9932 +28
- Misses 958 959 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
FWIW, I have a similar PR minus the |
|
Thank you @fgsch, I've updated the code. I didn't see your PR, I'm sorry. |
👍
I haven't tried it yet. IIUC you are using the attribute for the automatic valve close if there is no water together with the timing irrigation command for this purpose? Is this correct?
No worries. I changed the name to match other devices. Might as well do it here but really up to you. |
I'm sorry I don't understand your question. I added the I added also the two alarms, because I found your PR #3346 that added the attributes and I read that the other PRs in ZHA needed for the |
That attribute, 0x5011, is used to turn the valve off if there is no water for the specified amount of time (as per https://github.com/Koenkk/zigbee-herdsman-converters/blob/master/src/devices/sonoff.ts#L1447). This PR uses this attribute as a timer, which is later used with command 0x42 for timed irrigation (as per https://github.com/Koenkk/zigbee-herdsman-converters/blob/master/src/devices/sonoff.ts#L329-L363). Effectively, this is changing what 0x5011 is used for, assuming I understood correctly. |
|
Now I understand. My idea was to keep a single switch but let the user set a timeout and then send a 42 instead of a 01. I found no way to add and store that value. Trying to figure it out I found that parameter, but I didn't get it was used for water shortage. Could you point me in the right direction of adding an optional timeout in the device config? |
I'm not sure. Perhaps one of the devs could help. |
|
@fgsch, are you the reviewer who needs to respond based on the label? |
|
No, it's a label for me (or other reviewers) that an answer/review/help is needed. |
|
Ok, thank you. |
|
I've updated the PR for version 0.0.136, considering the merging of #3910. |
|
I've already tested your quirk with my 2 Sonoff SWV and it works perfectly fine :) In my opinion it is ready for merge :) I am currently trying to implement the few missing features that you may find in z2m, although it's not that useful. |
puddly
left a comment
There was a problem hiding this comment.
I've added a few code quality comments. I do have a question about this functionality as a whole, however: it looks like this PR basically adds a software-only "auto close time" configurable entity to the device and then intercepts OnOff:on commands, substituting them with OnOff:on_with_timed_off.
Since OnOff:on_with_timed_off is a standard ZCL cluster command, it feels odd to change the behavior of this one device. We should maybe instead consider implementing this as an action for all devices with OnOff cluster support? @TheJulianJES thoughts?
| cluster_id = OnOff.cluster_id | ||
| cmd_values = OnOff.commands_by_name.values() |
There was a problem hiding this comment.
| cluster_id = OnOff.cluster_id | |
| cmd_values = OnOff.commands_by_name.values() |
| kwargs["off_wait_time"] = 1 | ||
| self.create_catching_task(self._turn_off_later(on_time_value)) | ||
|
|
||
| return await self.request( |
There was a problem hiding this comment.
You should call return await super().command(*args, **kwargs) instead, since this method is effectively proxying the call to command.
| CustomSonoffCluster.AttributeDefs.on_time.id | ||
| ) | ||
| if on_time_value is not None and on_time_value != 0: | ||
| command = self.server_commands[0x42] # on_with_timed_off |
There was a problem hiding this comment.
| command = self.server_commands[0x42] # on_with_timed_off | |
| command = self.ServerCommandDefs.on_with_timed_off |
What does this line do? command isn't used anywhere.
| self._is_manuf_specific or command.is_manufacturer_specific | ||
| ): | ||
| manufacturer = self._manufacturer_id | ||
| if command_id == 0x01: |
There was a problem hiding this comment.
| if command_id == 0x01: | |
| if command_id == self.ServerCommandDefs.on.id: |
| async def _turn_off_later(self, delay): | ||
| """We are not receiving the auto off event, so we force an update.""" | ||
| await asyncio.sleep(delay + 1) | ||
| await self.endpoint.on_off.read_attributes(["on_off"]) |
There was a problem hiding this comment.
Does the device not report the attribute change on its own, when it turns back off?
Yeah, I don't think this implementation is optimal. If needed, this can already be done using a HA automation that sends a cluster command. The " |
This is a different feature from the device. https://github.com/Koenkk/zigbee-herdsman-converters/blob/4275931105788ec04b5b671640a2c86efa97f131/src/devices/sonoff.ts#L278 function is how timed / cyclic irrigation is managed by Z2M, using '0x5008' attribute |
@fcrozat I think your are referring to something else. This branch is about sending an on_with_timed_off command (0x42) with a parameter that tells the valve to close after a certain amount of time. The review questions were how to send the command and where to store this interval. I would love to see the cyclic irrigation options implemented in ZHA as well. But that seems to be out of scope for this branch. |
|
Sorry, I wasn't clear enough. I wanted to expand a bit @TheJulianJES answer: the way timed irrigation is handled on Z2M is simply through a 1 cycle of cycled irrigation (if I read the code right) and not (mis)-using "on_time" (0x5011). |
|
This is my first attempt at creating a quirk, so I'm really open to help and suggestions. Looking at z2m code, if I get it right, I saw that it's possible to send the I wanted to send this command using the already available switch if the I checked the cyclic code in z2m, but it was too difficult to translate and test in a real case scenario. If using the Thank you for your support. |
|
@puddly @TheJulianJES Do I have to fix the requested changes, or is it necessary to verify if this is the right approach as stated here? |
|
I just realized that ZHA doesn't actually implement the HA Valve platform! I think, since this is a standard ZCL command and theoretically is supported by many other valves in a standard way, we could just globally implement this for all valves, since this functionality likely isn't useful for other device types. @TheJulianJES thoughts? |
|
Have Matter implanting valve control in its cluster library ? |
|
Hi @micz , I had to dig a lot to come to this PR ( and thanks for proposing the changes) as these are the changes I have been looking for: I hope your changes are implemented as otherwise I need to use your code an implement it as Custom Quirk as I don't want the existing basic valve features at all. Some users are saying its because of old Firmware so updating it to 0x00001004 to see if additional features appear for ZHA or not. |
You need to update the firmware to use those features and also this PR as a quirk. |
For reference, there is now an open pull request in the zha repo that will add support for HA's Valve platform to ZHA, see: Also be aware of these related feature requests and discussions (which in turn links to other indirectly related discussions):
ZCL R8 spec does not actually support valves in any way, but the Matter 1.3 spec does, so support might come in ZCL R9 |
|
Unfortunately, no update to add cluster command to pass on timer off to valve:
|
I added these features to the Sonoff SWV-BSP Smart Water Valve (see #3298):
on_timeattribute, representing the number of seconds after opening before the valve automatically closes.on_time == 0it's a normal On command (0x01)on_time != 0it sends a on_with_timed_off (0x42), with theon_timenumbers of seconds sets in the attributeThis is my first attempt at creating a Quirk.
Any advice on whether I'm following the correct way would be appreciated.
Thank you!
Checklist
pre-commitchecks pass / the code has been formatted using Black