-
Notifications
You must be signed in to change notification settings - Fork 522
WWSTCERT-8180/8183/8186 Add support to smart sirens #2431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
WWSTCERT-8180/8183/8186 Add support to smart sirens #2431
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 481 suites 0s ⏱️ Results for commit 9b15c12. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 9b15c12 |
| @@ -1,4 +1,4 @@ | |||
| -- Copyright 2022 SmartThings | |||
| -- Copyright 2025 SmartThings | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please leave the copyright date as-is
| if compObj then | ||
| if component == "SirenVolume" then | ||
| device:set_field("sirenVolume", mode_set, {persist = true}) | ||
| elseif component == "SirenVoice" then | ||
| device:set_field("sirenVoice", mode_set, {persist = true}) | ||
| elseif component == "SquawkVolume" then | ||
| device:set_field("squawkVolume", mode_set, {persist = true}) | ||
| elseif component == "SquawkVoice" then | ||
| device:set_field("squawkVoice", mode_set, {persist = true}) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to use device:get_latest_state rather than setting these redundant fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend a test of the lifecycle events where PRIMARY_SW_VERSION isn't already set to mirror what the flow would look like for a new device join.
greens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the profile of a fingerprint is changed, already-installed devices do not switch the profile they're using.
I'm worried that these changes will break functionality for those already installed devices (which will lack the new components). I expect this is the case, and why the test_frient_siren test had to be updated.
I might suggest that you keep the fingerprint and original handler the same, other than adding a read of the fw version, then add a sub-driver that checks that value or the new model names.
| metadata: | ||
| mnmn: SmartThingsCommunity | ||
| vid: 6f595ea0-8f76-3a67-9e4f-e51ffdad970f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discourage custom metadata for WWST profiles. Can you elaborate on what is being included here?
You also have failing tests. |
| mode, | ||
| battery, | ||
| refresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your additions here are causing the tests in test_zigbee_siren to fail, as the test was not written with these defaults included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to update these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
| - id: WarningDuration | ||
| capabilities: | ||
| - id: mode | ||
| version: 1 | ||
| preferences: | ||
| - title: "Alarm duration (s)" | ||
| name: warningDuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the difference between the WarningDuration component and the warningDuration preference? They seem like they might come into conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WarningDuration mode is made to expose it in UI to make routines more configurable (along other modes). The goal was to create routines with different siren behavior for different triggering events. The preference allows user to input the exact value for the alarm duration, but is not applicable to routines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't they have different values, though? Can't that cause conflicts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please answer this question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not explained it well. The warningDuration from preferences should be named maxWarningDuration as it is max duration the siren can be turned on. So if the mode WarningDuration is set to for example 2 minutes, but preference warningDuration is set to 1 minute, then the siren will be on for 1 minute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So they can interact, and one can limit the other. If you're determined to keep both, would it be possible, then, to change the supported values for WarningDuration's mode attribute to only include values below the preference value? And also to change the wording of the preference to indicate it's a maximum value?
I still think it's confusing, and only having one would be ideal, but I think the current situation with both can be improved a little bit.
| "Capability(alarm) command(both) on should be handled", | ||
| { | ||
| { | ||
| channel = "capability", | ||
| direction = "receive", | ||
| message = { mock_device.id, { capability = "alarm", component = "main", command = "both", args = { } } } | ||
| }, | ||
| { | ||
| channel = "zigbee", | ||
| direction = "send", | ||
| message = { mock_device.id, IASWD.server.commands.StartWarning(mock_device, | ||
| SirenConfiguration(0xC1), | ||
| data_types.Uint16(0x00B4), | ||
| data_types.Uint8(00), | ||
| data_types.Enum8(00)) } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tell me what this command would have done for the older siren?
I'm very uncomfortable changing functionality like this, which is why I recommended a sub-driver based on the firmware version to change these sorts of commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been decided to not use the both command. However if it was used, the bit operations would change the SirenConfiguration so that bits 7 and 6 would store the siren volume value (0b11), bits 5 and 4 would store strobe value (0b00) and last 4 bits would store mode value. In this case that would mean that the siren would be turned on in mode 1 with the volume 3 and no strobe.
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests