[Aqara] Aqara Dual Relay Module T2 - #3146
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
|
zigbee-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 6d0eb03 |
Test Results 73 files 541 suites 0s ⏱️ Results for commit 6d0eb03. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
This should include the firmwareUpdate capability like the other device profiles do. This allows the device FW version to be displayed to the user in the app.
There was a problem hiding this comment.
Since this device does not support firmware updates in the SmartThings environment, I have removed the firmwareUpdate capability from the device profile as suggested by Samsung.
There was a problem hiding this comment.
If the device supports reporting its firmware version, I think we should still have it (which pretty much all zigbee devices do support).
| data_type = data_types.Uint8, | ||
| value_map = { rocker = 0x01, rebound = 0x02 }, | ||
| }, | ||
| -- External switch wiring type (same attribute as stse.electricSwitchType, with an extra "nc" |
There was a problem hiding this comment.
Why is this needed? Can we add the nc variant t othe existing electricSwitchType preference?
Since they map to the same attribute, this could get confusing in the future.
There was a problem hiding this comment.
If we add the 'nc' variant to the existing electricSwitchType preference, I am wondering if there will be any impact or side effects on devices that previously did not support the 'nc' variant.
I would appreciate it if you could share your thoughts on whether this might cause any issues regarding backward compatibility.
There was a problem hiding this comment.
Your concern is valid; adding the variant to the preference will expose it for all devices that have the preference even if they do not support it. IMO this is probably okay, but would depend on how existing devices that do not support it handle the nc option being set in the attribute write. If the attribute write is rejected and it is essentially a no-op (what I would expect), then we can just add text in the preference definition that the option may not be supported.
IMO, we should combine them, but I am curious if @hcarter-775 or @tpmanley have any thoughts
| local SUPPORTED_INTERLOCK = { "normal", "interlock" } | ||
| local COMPONENT_DEVICE_MODE = "devicemode" | ||
| -- Relay working modes mapped to their raw device values. dry_contact_open_pulse_mode (raw 2) is | ||
| -- intentionally not exposed; the remaining modes keep their original device values (on_off stays 3). |
There was a problem hiding this comment.
What is the reason for not exposing this? The mode capability can take arbitrary strings, so it could be represented.
If a device reports this mode, the driver silently drops it, and then the device state is no longer correct; this seems misleading to the user. Although I am not sure I fully understand these modes, so perhaps I am wrong.
There was a problem hiding this comment.
Mode support varies depending on the device using this driver, which is why I separated them
There was a problem hiding this comment.
Will some devices report dry_contact_open_pulse_mode?
|
Profile category check: ✅ Passed - all profiles have a category defined. |
2 participants
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests