WWSTCERT-9857 Add Zooz ZSE50 to zwave-siren (for WWST Cert)#2681
WWSTCERT-9857 Add Zooz ZSE50 to zwave-siren (for WWST Cert)#2681jtp10181 wants to merge 10 commits intoSmartThingsCommunity:mainfrom
Conversation
| tones_list[tone_id] = string.format("%s: %s (%ss)", tone_id, tone_name, duration) | ||
| tones_duration[tone_id] = duration | ||
| device:set_field("TONES_LIST_TMP", tones_list) | ||
| device:set_field("TONES_DURATION_TMP", tones_duration) |
There was a problem hiding this comment.
it seems a little redundant to use two maps here where the only unique information one of them includes is the tone_name. Could you just use one map like:
{ [tone_id] = {duration: X, name: Y} }
And construct the strings in the places you actually want to emit them?
Also, the value of capabilities.mode.supportedArguments will be identical to device:get_field("TONES_LIST") most of the time, so it seems especially redundant there.
greens
left a comment
There was a problem hiding this comment.
Please write and include some unit tests and remove your debug logging.
|
I will work on the remaining issues and follow up when complete. |
* Remove firmwareUpdate from driver_template * Fix copyright date * Remove unnecessary profile switch
| duration = duration * device.preferences.playbackLoop | ||
| end | ||
| end | ||
| log.debug(string.format("Playing Tone: %s, playbackMode %s, duration %ss", tone_id, playbackMode, duration)) |
There was a problem hiding this comment.
Please remove debug log once you are ok with functionality.
There was a problem hiding this comment.
Is it OK to keep some as Info logs? I removed a bunch but changed this one to Info.
| local soundSwitch_refresh = function() | ||
| local chime = device:get_latest_state("main", capabilities.chime.ID, capabilities.chime.chime.NAME) | ||
| local mode = device:get_latest_state("main", capabilities.mode.ID, capabilities.mode.mode.NAME) | ||
| log.debug(string.format("soundSwitch_refresh: %s | %s", chime, mode)) |
There was a problem hiding this comment.
I changed this one to Info log as well if thats OK.
There was a problem hiding this comment.
Reiterating some of the comments that Pegor left, be sure to remove all usages of log.debug() from this file was your implementation is complete.
|
Hi @jtp10181 let us know when you're ready for a re-review. |
Will do, I got caught up on a bunch of other stuff finally, so hope to circle back to this one soon. |
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 72 files 496 suites 0s ⏱️ Results for commit b7a24a7. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against b7a24a7 |
* Merged upstream and updated for lazy-load * Consolidated tones_list and tones_duration maps as suggested * Removed debug logs and commented out code
* Added units tests
Removed audioVolume from supported capabilities so main unit test does not fail.
Added more units tests to increase coverage
Ok @greens, @wkhenon, @cbaumler I really hate making unit tests so I decided to have Copilot help make then, worked great once I told it to ingest all the other ones in the same folder for examples. |
| if tones_list == nil or tones_list == {} then | ||
| rebuildTones(device) | ||
| end |
There was a problem hiding this comment.
If this statement is true, the tones list could be nil, and then there would be a nil index error on line 200.
| if tones_list == nil or tones_list == {} then | |
| rebuildTones(device) | |
| end | |
| if tones_list == nil or tones_list == {} then | |
| rebuildTones(device) | |
| return | |
| end |
There was a problem hiding this comment.
I did not want to bail on playing the tone here so I fixed the now line 201 to be nil safe so it will fail over to my default as intended.
| local tones_list = device:get_field("TONES_LIST") | ||
| local default_tone = device:get_field("TONE_DEFAULT") |
There was a problem hiding this comment.
These need to be nil checked to make sure we avoid nil index errors on lines 52 and 54.
There was a problem hiding this comment.
I added a nil fall back to 47
Then for the other one I rearranged some things below and added a nil check for tones_list to the only if block using it. The duration is now set to 0 by default when initialized as well.
Check all that apply
Type of Change
Checklist
Description of Change
Adding Zooz ZSE50 for WWST Certification
Summary of Completed Tests