Add the processing logic of motion sensor on button devices #2422
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against ac5ed1b |
| capabilities: | ||
| - id: motionSensor | ||
| version: 1 | ||
| - id: firmwareUpdate |
There was a problem hiding this comment.
firmwareUpdate and refresh can be removed from the motion component here, since they are already in the main component.
| - id: firmwareUpdate | ||
| version: 1 | ||
| - id: refresh | ||
| version: 1 |
| build_button_profile(device, main_endpoint, #button_eps, #motion_eps) | ||
| -- All button endpoints found will be added as additional components in the profile containing the main_endpoint. | ||
| -- The resulting endpoint to component map is saved in the COMPONENT_TO_ENDPOINT_MAP field | ||
| build_button_component_map(device, main_endpoint, button_eps) |
There was a problem hiding this comment.
| build_button_component_map(device, main_endpoint, button_eps) | |
| build_button_component_map(device, main_endpoint, button_eps, motion_eps) |
We could pass in the motion endpoints here just like you did for build_button_profile, and then add the motion endpoint to the component-endpoint map within build_button_component_map rather than introduce a new build_motion_component_map function
There was a problem hiding this comment.
Ok, thank you for your kind advice! We have revised the places you mentioned, please check whether there is still room for improvement.
|
I left a few comments but overall the proposed changes look fine to me, and thank you for providing unit tests. Have you tested this change on the device and confirmed that the plugin renders correctly? |
Yes, we have tested these changes on the device, and the display on the plugin is as expected. |
| - name: RemoteController | ||
| - id: motion | ||
| capabilities: | ||
| - id: motionSensor |
There was a problem hiding this comment.
I guess I'm wondering why we're putting motion on its own component in the first place? Seems like this should be in the main component. Same for the other
There was a problem hiding this comment.
To clarify, our current process is to put all capabilities that don't require special handling to be placed on the main component. Doing this will make the changes in the component map moot, and the event should be emitted on main by default. Thanks!
There was a problem hiding this comment.
Ah yeah, that's a good callout
There was a problem hiding this comment.
We have tried this modification before, but the results are somewhat different from what we expected. We put the details in the comments on this page (https://smartthings.atlassian.net/browse/MTR-939). Please review.
Check all that apply
Type of Change
Checklist
Description of Change
In the past, the button devices with motion sensor did not support their complete functions. Now add relevant processing logic, so that this kind of devices can display the sensor normally in the plugin.
Summary of Completed Tests
The unit test file is contained in the commit, and all tests can pass.