WWSTCERT-10701 Add Sonoff SNZB-01M Smart Scene Button into zigbee-button.#2510
WWSTCERT-10701 Add Sonoff SNZB-01M Smart Scene Button into zigbee-button.#2510Oniums wants to merge 13 commits intoSmartThingsCommunity:mainfrom
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 70dd9ec |
|
| }, | ||
| SONOFF_BUTTON_4 = { | ||
| MATCHING_MATRIX = { | ||
| { mfr = "SONOFF", model = "SNZB-01M" } | ||
| }, | ||
| SUPPORTED_BUTTON_VALUES = { "pushed", "double", "held", "pushed_3x" }, | ||
| NUMBER_OF_BUTTONS = 4 |
There was a problem hiding this comment.
Since you overwrote added in your handler (because you did not include the button capability on your main component, this info is never used.
There was a problem hiding this comment.
Thanks for the suggestion. I checked the existing profiles: four-buttons-without-main-button has no battery capability, while four-buttons-battery includes button on main, which doesn’t match the SNZB-01M capability layout. Would you prefer that I: use four-buttons-without-main-button and accept no battery on main, or use the profile I added earlier.
There was a problem hiding this comment.
I understand why you've added the new profile, but I'll advise you on why our multi-button profiles include the button capability on the main component: without it (and duplicating any other buttons' events to main, the remote on the device list will not change to indicate button presses. You'll only see them within the device details page.
I didn't notice four-buttons-without-main-button didn't have battery. I definitely think you should maintain that.
So you can go with your new profile if you like, but this comment was merely pointing out that, because these values are only used in the zigbee-multi-button added handler, your additions to this file are not used.
There was a problem hiding this comment.
I'll advise you on why our multi-button profiles include the button capability on the main component: without it (and duplicating any other buttons' events to main, the remote on the device list will not change to indicate button presses. You'll only see them within the device details page.
It seems that you are going to go forward with the profile as is. I am commenting to make sure you have tested the device on the platform to ensure it works the way you want, since this is different than what we have for our multibutton profiles. We recommend having the button capability on the main component, and duplicating events for any button press to that component also, so that in the device list view of the app, you can see the events affect the device rather than in just the details view. Doing so also gives users a hook for routines that will trigger on any button press of the remote.
|
|
|
|
driver needs to be rebased |
|
Is the rebase based on this PR? If I submit a new PR, will it cause any issues if it doesn’t match the PR used for the Samsung certification application? |
|
@Oniums you should be able to set your fork's upstream to our main repo, update the main branch there, then rebase your changes on your local branch onto In your fork's directory: Then you should be able to resolve the conflicts and |
… lines Update copyright year from 2022 to 2026,remove remove whitespace-only lines
remove log.info
greens
changed the title
cbaumler
requested review from
ctowns and
hcarter-775
and removed request for
pegor-karoglanian and
wkhenon
KKlimczukS
changed the title
KKlimczukS
left a comment
KKlimczukS
left a comment
There was a problem hiding this comment.
Please update copyright sections
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
LGTM, just note the suggestion to reconsider adding the button capability to the main component of the device and emitting events for it when any button is affected.
| return false | ||
| end | ||
|
|
||
| return sonoff_can_handle No newline at end of file |
There was a problem hiding this comment.
nit: newline in this file and all the other files.
| }, | ||
| SONOFF_BUTTON_4 = { | ||
| MATCHING_MATRIX = { | ||
| { mfr = "SONOFF", model = "SNZB-01M" } | ||
| }, | ||
| SUPPORTED_BUTTON_VALUES = { "pushed", "double", "held", "pushed_3x" }, | ||
| NUMBER_OF_BUTTONS = 4 |
There was a problem hiding this comment.
I'll advise you on why our multi-button profiles include the button capability on the main component: without it (and duplicating any other buttons' events to main, the remote on the device list will not change to indicate button presses. You'll only see them within the device details page.
It seems that you are going to go forward with the profile as is. I am commenting to make sure you have tested the device on the platform to ensure it works the way you want, since this is different than what we have for our multibutton profiles. We recommend having the button capability on the main component, and duplicating events for any button press to that component also, so that in the device list view of the app, you can see the events affect the device rather than in just the details view. Doing so also gives users a hook for routines that will trigger on any button press of the remote.
|
I tested it on the platform and it works as expected. The two screenshots above are from the SmartThings app during testing. |
|
@Oniums the view that Steven and I are mentioning where button presses will not be shown is the list view for the devices. This is why we typically include the button capability on the main component and then issue events for it whenever one of the buttons is pressed. This is the app view I am referencing: 
|
- Reuse existing four-buttons-battery profile instead of custom sonoff-buttons-battery - Mirror button events to main component via emit_event for device list visibility - Update test profile reference and add main component event assertions - Remove unused sonoff-buttons-battery.yml profile
|
I've updated the SNZB-01M implementation based on the review feedback:
I tested on a Hub V3 with a physical SNZB-01M device, and everything works as expected. |
|
@Oniums it looks like there are still test failures that need to be addressed. |
- replace unsupported build_custom_report_attribute usage with an explicit custom Zigbee attribute report builder - add missing main component expectations for the added lifecycle test - add min_api_version guards to Sonoff test cases - keep Sonoff button event assertions aligned with the four-buttons-battery profile behavior
|
Hello @Oniums, it looks like this PR branch is out of date with main. Please update it with the latest main and push the result so it can be merged. |
8 participants
Check all that apply
Type of Change
Checklist
Description of Change
This PR adds support for the Sonoff SNZB-01M Smart Scene Button to the existing zigbee-button driver.
Changes made:
fingerprints.ymlto recognize the SONOFF SNZB-01M devicesonoff-buttons-battery.ymlwith 4 button components and battery capabilitysrc/zigbee-multi-button/sonoff/init.luato handle SONOFF-specific Zigbee communicationtest_sonoff_button.luafor verification purposesSummary of Completed Tests
Device Testing:
Code Testing:
Compatibility: