Repository navigation
Conversation
1、Add Sonoff valve subdriver lazy loading. 2、Modify comment language and file name
|
|
|
I have tested this with a physical Sonoff SWV2C device. It pairs successfully in the SmartThings app, and both valve controls work normally. |
That's the easiest part, but does it stay on for longer than 10 minutes or does it automatically turn itself off after 10 minutes? What happens after - say - 15 minutes? The device is completely useless when it turns itself off after 10 minutes. Users don't want to turn it on every 10 minutes if they want to irrigate for an hour. Discussion: It's a bit more complicated (link to (outdated) code and technical documentation): |
|
Another issue: before a channel is turned on, the opposite channel must be turned off. According to the official manual only one channel should be open at any time. |
|
I tested this with a physical Sonoff SWV2C device. For the 10-minute behavior: yes, the device has an internal default auto-close/manual watering duration. In my test, if a valve is opened normally, the device closes it automatically after the configured duration. The default value appears to be 10 minutes. Users can change the irrigation/manual watering duration through the device's own configuration path outside of this basic valve control behavior. For the dual-channel behavior: I also tested the mutual exclusion behavior on the physical device. The two channels cannot stay open at the same time. If channel 1 is open and then channel 2 is opened, channel 1 is automatically closed and channel 2 opens. The device firmware reports the valve state automatically when a valve is operated, and the SmartThings app state updates correctly from those reports. If preferred, I can update the driver to explicitly send an Off command to the opposite endpoint before opening the requested channel, so the SmartThings state stays aligned with the device behavior. |
KKlimczukS
changed the title
|
Thanks for the feedback. I’d prefer to keep this PR focused on SWV2C. SWV2C is a different hardware product from SWV1C. SWV1C is a single-channel valve, while SWV2C is a dual-channel valve with two endpoints and mutual-exclusion behavior between the two channels. There is already a separate PR for SWV1C, and that PR is focused on the single-channel device. Since SWV2C requires different handling for the second endpoint and child device, I think it is clearer and safer to review SWV2C in this dedicated PR instead of combining both hardware products into one submission. This also keeps the scope of each PR aligned with one hardware product and one WWST certification flow. |
|
Profile category check: ✅ Passed - all profiles have a category defined. |
|
Duplicate profile check: Warning - duplicate profiles detected. |
|
Invitation URL: |
|
zigbee-valve_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 8252738 |
Test Results 73 files 545 suites 0s ⏱️ Results for commit 8252738. ♻️ This comment has been updated with latest results. |
|
I tested the updated Edge driver (commit a4565e0) with a physical SONOFF SWV2C on a SmartThings v3 Hub. Both valve channels can be controlled from the SmartThings app, and the app reflects the device-reported valve states. Switching from channel 1 to channel 2 closes channel 1 and opens channel 2 as expected. No device firmware changes were made. |
|
Thank you for the review. I pushed e5e09ae to remove a duplicate health_check assignment reported by Luacheck; this does not change driver behavior. I also checked the Driver Tests failures on the previous commit. The GitHub Actions test results reported 3,294/3,294 passing, and I ran all 44 zigbee-valve tests locally against both the 0.57 and 0.58 Lua libraries, with 44/44 passing on each. The external Jenkins statuses for 57, 58, and dev do not include a log or a link to the failing test cases, so I cannot yet determine whether those failures involve this driver. Could you please share the failing test names/logs, or rerun those checks on this commit? I will address any failures attributable to this PR. Thank you. |
|
Quick update: I merged the current upstream main into this PR. Driver Tests for 57, 58, 59, 60, 61, and dev now all pass. This suggests the earlier 57/58/dev failures were related to the outdated base branch, so the failure logs I requested are no longer needed. This update did not change the SWV2C driver logic or device firmware. Thank you, and I would appreciate a re-review when convenient. |
|
Conflicts will need to be fixed prior to merging. Note the single failing tests is a known intermittent failure in matter test framework. |
|
Thanks for the heads-up. I updated the SWV2C branch with the latest main and resolved the conflicts introduced by the merged SWV1C changes in commit 93e614a. The single-channel SONOFF models remain supported, while SWV2C retains its dual-channel child-device handling. All 54 zigbee-valve Lua tests passed locally, and Luacheck reported no warnings or errors. The PR now merges cleanly with main. I would appreciate a re-review when convenient. |
|
Heads up that this also changes SWV1C behavior. Before this PR the Sonoff sub-driver had no lifecycle handlers. Now every Sonoff valve gets |
| if is_dual_channel(device) and ep == 0x02 then | ||
| local child = find_child(device, 2) | ||
| if child then target = child end | ||
| end |
There was a problem hiding this comment.
If ep == 2 but find_child(device, 2) returns nil, for example before the child is created or if creation failed, target stays the parent. A channel 2 report then overwrites channel 1’s valve state. Could you return early in that case instead of falling back to the parent? Something like:
if is_dual_channel(device) and ep == 0x02 then
target = find_child(device, 2)
if not target then return end
end
| end | ||
|
|
||
| --- A device button press prompts a refresh of both SWV2C channels. | ||
| local function identify_handler(driver, device, zb_rx) |
There was a problem hiding this comment.
identify_handler is registered on Basic.server.commands.ResetToFactoryDefaults, but the name and the comment (“a device button press”) describe something different. What message does the SWV2C actually send on a button press? If it is a ResetToFactoryDefaults command, please rename the handler and fix the comment to say so. If it is Identify or something else, the cluster or command binding looks wrong.
| local function battery_percentage_handler(driver, device, value) | ||
| local raw = value.value | ||
| local percent = utils.round(raw / 2) | ||
| device:emit_event(capabilities.battery.battery(percent)) | ||
| device:emit_event(capabilities.battery.battery(utils.round(value.value / 2))) | ||
| end | ||
|
|
There was a problem hiding this comment.
battery_percentage_handler duplicates the default PowerConfiguration handler. Can it be dropped?
|
Thanks for the review. I addressed the remaining comments in commit b9e3089:
All 58 zigbee-valve driver tests pass, and Luacheck reports 0 warnings and 0 errors. With the updated driver installed on a SmartThings v3 Hub, I verified valve control and battery display on the physical SWV2C. I had also verified the physical SWV1C valve behavior with the preceding update. |
| - id: powerSource | ||
| version: 1 | ||
| config: | ||
| values: | ||
| - key: "powerSource.value" | ||
| enabledValues: | ||
| - battery |
There was a problem hiding this comment.
| - id: powerSource | |
| version: 1 | |
| config: | |
| values: | |
| - key: "powerSource.value" | |
| enabledValues: | |
| - battery |
It doesnt seem like the device will ever change to be mains powered, so this capability is not necessary. The device having the battery capability shows that it is battery powered.
There was a problem hiding this comment.
Thanks for the clarification. I removed the redundant powerSource capability from the SWV2C-specific profile in 8252738, while keeping the battery capability and leaving the other valve profiles unchanged. The driver tests pass, and I confirmed on a physical SWV2C with a SmartThings v3 Hub that both valve controls and the battery percentage display still work as expected.
There was a problem hiding this comment.
@yezi289 in such case this device profile may not be needed (since it's the same as valve-battery profile)
| name: valve-battery-2 | ||
| components: | ||
| - id: main | ||
| capabilities: | ||
| - id: valve | ||
| version: 1 | ||
| - id: battery | ||
| version: 1 | ||
| - id: firmwareUpdate | ||
| version: 1 | ||
| - id: refresh | ||
| version: 1 | ||
| categories: | ||
| - name: WaterValve |
There was a problem hiding this comment.
@yezi289 since powerSource was removed, this device profile looks exactly the same as valve-battery.yml , so actually we don't need a new device profile here and valve-battery might be used in fingerprints.
| { | ||
| channel = "zigbee", | ||
| direction = "receive", | ||
| message = { mock_device.id, OnOff.attributes.OnOff:build_test_attr_report(mock_device, |
There was a problem hiding this comment.
nit: please re-format this part of the code
| message = { mock_device.id, OnOff.attributes.OnOff:build_test_attr_report(mock_device, | ||
| false) } |
There was a problem hiding this comment.
nit: please re-format this part of the code
| message = { mock_child.id, OnOff.attributes.OnOff:build_test_attr_report(mock_device, | ||
| true):from_endpoint(0x02) } |
There was a problem hiding this comment.
nit: please re-format this part of the code
| message = { mock_child.id, OnOff.attributes.OnOff:build_test_attr_report(mock_device, | ||
| false):from_endpoint(0x02) } |
There was a problem hiding this comment.
nit: please re-format this part of the code
| message = { mock_device.id, OnOff.attributes.OnOff:build_test_attr_report(mock_device, | ||
| true):from_endpoint(0x02) } |
There was a problem hiding this comment.
nit: please re-format this part of the code
| message = { mock_device.id, OnOff.attributes.OnOff:build_test_attr_report(mock_device, | ||
| true):from_endpoint(0x02) } |
There was a problem hiding this comment.
nit: please re-format this part of the code
|
Thanks for the follow-up review. I addressed the profile and formatting comments in c2a897f: both SWV2C fingerprints now reuse the existing valve-battery profile, the duplicate valve-battery-2 profile is removed, and the six highlighted OnOff test messages have been reformatted. The SWV2C and related zigbee-valve tests pass locally (57/57), and Luacheck reports 0 warnings and 0 errors. The capabilities are unchanged from the physical SWV2C version previously verified for valve control and battery display. |
6 participants
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests