-
Notifications
You must be signed in to change notification settings - Fork 498
Pull request for zunzunbee Slate Switch zigbee button edge driver #2224
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?
Conversation
Mfr: zunzunbee, model:SSWZ8T 1. Updated fingerprints.yml 2. New profile- eight-buttons-temp-battery.yml, along with new device configuration
if tonumber(zone_status.value) > 0x0400 then | ||
number_of_buttons = 1 | ||
end | ||
if tonumber(zone_status.value) > 0x0800 then | ||
number_of_buttons = 2 | ||
end | ||
if tonumber(zone_status.value) > 0x0C00 then | ||
number_of_buttons = 3 | ||
end | ||
if tonumber(zone_status.value) > 0x1000 then | ||
number_of_buttons = 4 | ||
end | ||
if tonumber(zone_status.value) > 0x1400 then | ||
number_of_buttons = 5 | ||
end | ||
if tonumber(zone_status.value) > 0x1800 then | ||
number_of_buttons = 6 | ||
end | ||
if tonumber(zone_status.value) > 0x1C00 then | ||
number_of_buttons = 7 | ||
end | ||
if tonumber(zone_status.value) > 0x2000 then | ||
number_of_buttons = 8 | ||
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.
It seems like you just want the 3 most significant bits:
if tonumber(zone_status.value) > 0x0400 then | |
number_of_buttons = 1 | |
end | |
if tonumber(zone_status.value) > 0x0800 then | |
number_of_buttons = 2 | |
end | |
if tonumber(zone_status.value) > 0x0C00 then | |
number_of_buttons = 3 | |
end | |
if tonumber(zone_status.value) > 0x1000 then | |
number_of_buttons = 4 | |
end | |
if tonumber(zone_status.value) > 0x1400 then | |
number_of_buttons = 5 | |
end | |
if tonumber(zone_status.value) > 0x1800 then | |
number_of_buttons = 6 | |
end | |
if tonumber(zone_status.value) > 0x1C00 then | |
number_of_buttons = 7 | |
end | |
if tonumber(zone_status.value) > 0x2000 then | |
number_of_buttons = 8 | |
end | |
number_of_buttons = tonumber(zone_status.value) >> 10 |
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.
But also, you're writing this for a specific device. Is it possible to configure the device so that it does not have 8 buttons?
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.
Yes, by default it is one button. User may configure touchscreen to have up to 8 buttons. Will place comments in code describing bit pattern.
if tonumber(zone_status.value) > 0x2000 then | ||
number_of_buttons = 8 | ||
end | ||
event = capabilities.button.numberOfButtons({ value = number_of_buttons }, { visibility = { displayed = true } }) |
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.
This should only be emitted once, rather than on every button press. The changes you made in supported_values
should handle this.
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.
Ok. I think capabilities.button.numberOfButtons does nothing. Was expecting this call to dynamically update the number of buttons. Since this did not work, used visible condition (note below) to achieve desired behavior.
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.
Note that the event to emit numberOfButtons is needed by the visible condition logic in presentation layer. The user may dynamically configure number of buttons, so it needs to be evaluated on every button press.
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 makes sense now that I understand you are dynamically hiding components.
if zone_status:is_alarm2_set() and zone_status:is_alarm1_set() then | ||
button_name = "button1" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_alarm2_set() then | ||
button_name = "button1" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_tamper_set() and zone_status:is_alarm1_set() then | ||
button_name = "button2" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_tamper_set() then | ||
button_name = "button2" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_battery_low_set() and zone_status:is_alarm1_set() then | ||
button_name = "button3" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_battery_low_set() then | ||
button_name = "button3" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_supervision_notify_set() and zone_status:is_alarm1_set() then | ||
button_name = "button4" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_supervision_notify_set() then | ||
button_name = "button4" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_restore_notify_set() and zone_status:is_alarm1_set() then | ||
button_name = "button5" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_restore_notify_set() then | ||
button_name = "button5" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_trouble_set() and zone_status:is_alarm1_set() then | ||
button_name = "button6" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_trouble_set() then | ||
button_name = "button6" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_ac_mains_fault_set() and zone_status:is_alarm1_set() then | ||
button_name = "button7" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_ac_mains_fault_set() then | ||
button_name = "button7" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
elseif zone_status:is_test_set() and zone_status:is_alarm1_set() then | ||
button_name = "button8" | ||
event = capabilities.button.button.held(additional_fields) | ||
elseif zone_status:is_test_set() then | ||
button_name = "button8" | ||
event = capabilities.button.button.pushed(additional_fields) | ||
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.
It might be easier to read here if you had a map of the various bit patterns to the associated component and event
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.
Will add comments
-- zone_status: button press bit pattern
-- Bit 0 : Held action status (1 if held, 0 if not)
-- Bit 1 : Button 1 pressed (1 if pressed, 0 if not)
-- Bit 2 : Button 2 pressed
-- Bit 3 : Button 3 pressed
-- Bit 4 : Button 4 pressed
-- Bit 5 : Button 5 pressed
-- Bit 6 : Button 6 pressed
-- Bit 7 : Button 7 pressed
-- Bit 8 : Button 8 pressed
-- Bits 10-12: Number of buttons in the product (value from 1 to 8)
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.
Code will also be refactored to use bit patterns.
mnmn: SmartThingsCommunity | ||
vid: b85e2d6f-0ea4-38e2-b5f4-31c3873b1920 |
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.
in general, we discourage using custom metadata for our WWST devices, can you elaborate on what changes you've made in this custom metadata that make it necessary?
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.
Product supports dynamic assignment of buttons 1-8. We use the visibility feature to hide buttons. As an example by default product is one button. User will see remaining 7 buttons disabled in UI.
Secondly we wanted the ability to identify product. We used the tone capability for this. When activated, product has a buzzer that will sound to locate product.
Followed recommendations from dev support (1. https://community.smartthings.com/t/visible-condition-with-embedded-configs/256247/12)
2. https://community.smartthings.com/t/identify-find-button-capability/296472/8
components: | ||
- id: main | ||
capabilities: | ||
- id: button |
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 button
capability here seems to never emit any actual pressed
or held
events
I might suggest changing your setup here to either remove button
from the main
component, or eliminate the button1
component and have main
handle what were formerly button1
events
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.
That was done for UI feedback on the App Home screen. If main-button capability is not used, button (1-8) presses are not visible on home page dashboard.
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 missed L150. I see now where you are emitting button events from main
.
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 write some unit tests for your device.
Also, we generally use two spaces for our tabs. You've got a mix here.
1. Added unit test - src\test\zunzunbee_8_button.lua 2. Removed extra indentation in eight-buttons-temp-battery.yml 3. Updated code based on pull request comments- added bit pattern usage comments, refactored to use fewer if statements
Unit test has been added and code updated with comments, indentation corrected in profile file |
Check all that apply
Type of Change
Checklist
Description of Change
New zigbee multi button sub driver added for support of zunzunbee Slate Switch (model: SSWZ8T)
Summary of Completed Tests