Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Profile category check: ✅ Passed - all profiles have a category defined. |
|
Invitation URL: |
|
zigbee-valve_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 293f87c |
Test Results 73 files ± 0 542 suites +1 0s ⏱️ ±0s For more details on these failures, see this check. Results for commit 293f87c. ± Comparison against base commit 9d99e75. ♻️ This comment has been updated with latest results. |
|
Thanks for the review. Yes, I checked this behavior with the SONOFF SWV1C device. The device uses the Zigbee OnOff cluster for valve control and reports the valve state through the OnOff attribute. The custom handlers were added to explicitly map:
This is also needed because the device reports the OnOff value as I can also re-test without the custom handlers and confirm whether the default zigbee-valve handling covers this device correctly. |
Has this implementation been tested with the valve left open for longer than ten minutes? Several Hydro ONE users have reported that the valve closes itself after approximately ten minutes because of its device-side manual watering duration. From the current PR, it appears that Open only sends the standard Zigbee On command and does not read or update the SONOFF Could you confirm whether the tested devices remained physically open beyond ten minutes, and which model and firmware version were used? It would also be useful to know whether newer firmware has removed the default limit or whether the WWST integration is expected to configure |
|
To make it clear: the valve won't stay open for longer than 10 minutes, even when it was opened "manually" (Zigbee On command)? |
Yes. |
| sub_drivers = require("sub_drivers"), | ||
| health_check = false, | ||
| shared_device_thread_enabled = true, |
There was a problem hiding this comment.
Add the new sonoff subdriver to the sub_drivers.lua file in this driver and use lazy loading.
There was a problem hiding this comment.
@yanggx24 please change it according to @aleclorimer's remarks
| health_check = false, | ||
| shared_device_thread_enabled = true, |
There was a problem hiding this comment.
@yanggx24 please change it according to @aleclorimer's remarks
KKlimczukS
changed the title
|
@yanggx24 Please update Your branch. |
I have merged the latest code, is there anything else missing? |
|
@KKlimczukS Hello,what else do I need to do？ |
|
I have updated the branch with the Sonoff sub-driver lazy-loading changes, synchronized it with the latest main, and added the coverage test fix. The device was tested with SONOFF SWV1C firmware v1.1.0. The 10-minute automatic shutoff is device-side behavior, and the private 0xFC11/0x501D support is outside this WWST certification PR. Could you please re-review the latest commit and let me know if any blocking changes remain? |
KKlimczukS
changed the title
KKlimczukS
changed the title
I think this PR should include the changes required to configure private attributes if that's the plan for later. |
|
Thanks for the feedback. I’d prefer to keep this PR limited to SWV1C. SWV2C is a different hardware product, maintained by a different developer, and its development timeline is separate from SWV1C. There is already a dedicated PR for SWV2C, so I don’t think it would be appropriate to merge the two devices into this submission. The current SWV1C PR also does not include any private-attribute or custom-capability support. I would like to keep it that way. Private-attribute support is not part of this WWST submission. This is also consistent with the SmartThings PR guidance, which does not allow custom Capabilities for WWST submissions and may reject drivers using custom Zigbee, Z-Wave, or Matter clusters:
For these reasons, I suggest keeping this PR focused on the standard SWV1C implementation, while handling SWV2C and any future private-attribute support separately. |
@yanggx24 Here is the example how preferences are utilized: WWST policies allow using device preferences. |
@yanggx24 Please check previous remarks to the code. |
I have re-checked the places marked as needing changes, and I confirm that they have been rectified as required. If anything has been missed, please let me know. Thank you. |
@yanggx24 I haven't seen new commits here. Please check if You pushed Your latest changes. |
| -- Copyright 2022 SmartThings | ||
| -- | ||
| -- Licensed under the Apache License, Version 2.0 (the "License"); | ||
| -- you may not use this file except in compliance with the License. | ||
| -- You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, software | ||
| -- distributed under the License is distributed on an "AS IS" BASIS, | ||
| -- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| -- See the License for the specific language governing permissions and | ||
| -- limitations under the License. |
| -- Copyright 2022 SmartThings | ||
| -- | ||
| -- Licensed under the Apache License, Version 2.0 (the "License"); | ||
| -- you may not use this file except in compliance with the License. | ||
| -- You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, software | ||
| -- distributed under the License is distributed on an "AS IS" BASIS, | ||
| -- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| -- See the License for the specific language governing permissions and | ||
| -- limitations under the License. |
KKlimczukS
left a comment
KKlimczukS
left a comment
There was a problem hiding this comment.
Please update Your branch
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
This is close, it seems like the onoff handler override does justify the subdriver, but there are some other things that seem unnecessary in the subdriver.
| refresh | ||
| }, | ||
| cluster_configurations = { | ||
| [battery.ID] = { |
There was a problem hiding this comment.
Note this applies to all devices in this driver, I would prefer to jsut use the default configuration unless there is a good reason to report this so frequently. Here is the default:
local default_percentage_configuration = {
cluster = 0x0001,
attribute = 0x0021,
minimum_interval = 30,
maximum_interval = 21600,
data_type = data_types.Uint8,
reportable_change = 1
}
Why is this needed when you override it anyway in the subdriver?
| local PowerConfiguration = zcl_clusters.PowerConfiguration | ||
| local utils = require "st.utils" | ||
|
|
||
| local battery_configuration = { |
There was a problem hiding this comment.
Whats the reason for using this instead of the defaults? The only difference is the faster max reporting interval and typically for battery percent we dont need it to be as fast.
There was a problem hiding this comment.
So, I don’t need to explicitly call add_configured_attribute to register the reporting configuration for BatteryPercentageRemaining, right? Will they be sent down to the device by default?
There was a problem hiding this comment.
Thanks @cjswedes for catching this.
@yanggx24
Since the frawework does that already (supported_capabilities contain battery in the src/init.lua then the framework configures it) , this change may not be required here at all if You are ok with max reporting interval (from default configuration).
Please check it without those changes but before - make sure the cluster_configurations init.lua remains unchanged
5 participants
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests