Matter Switch: Include Ikea subdriver support for knob capability #2678
Conversation
hcarter-775
requested review from
Kwang-Hui,
ctowns,
greens,
nickolas-deboom and
tpmanley
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 482 suites 0s ⏱️ Results for commit d3b8761. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against d3b8761 |
nickolas-deboom
left a comment
nickolas-deboom
left a comment
There was a problem hiding this comment.
I left a comment about some confusion I had, but the rest all looks good from my perspective. As long as this has been testing on device and everything's working, I don't see any reason to hold up any of these changes. Nice work!
hcarter-775
force-pushed
the
feature/knob
branch
from
05b47bd to
94a1e0e
Compare
| if switch_utils.tbl_contains(scroll_fields.ENDPOINTS_PUSH, ib.endpoint_id) then | ||
| generic_event_handlers.initial_press_handler(driver, device, ib, response) | ||
| else | ||
| device:set_field(scroll_fields.LATEST_NUMBER_OF_PRESSES_COUNTED, 1) |
There was a problem hiding this comment.
I think the "ticks" calculations are incorrect for the events sent by Bilresa.
For example, when fast scrolling the wheel for 14 "ticks", the messages sent by the Bilresa are as follows:
- InitialPress(1)
- InitialPress(1)
- MultiPressOngoing(1, 11)
- InitialPress(1)
- MultiPressOngoing(1, 14)
which means that instead of 14 "ticks", this event handler will count 28 "ticks" (1 + 1 + 11 + 1 + 14) because LATEST_NUMBER_OF_PRESSES_COUNTED is reset to 1 after each InitialPress event.
I think it would be better to use the MultiPressComplete event to "reset" the ticks counter, while also adding a time-based safeguard in case the MultiPressComplete is lost.
There was a problem hiding this comment.
Hi @perexis, thanks for the investigation on your part. That is interesting that the MultiPressComplete event is not always being sent during fast scrolling. I will look into this further, though on first glance it seems unclear whether InitialPress or MultiPressComplete should be considered the "final say" for how these scroll events are interpreted.
There was a problem hiding this comment.
Hi @hcarter-775
I'm not saying that MultiPressComplete is not generated when fast scrolling. It is not generated because the driver does not subscribe to this event as it was not needed.
I am saying that it is not possible to correctly handle the rotate sequence without subscribing to this event. The fast scrolling was just an example to show the huge error in the calculations.
When the driver subscribes to the MultiPressComplete, the sequence changes to:
- InitialPress(1)
- InitialPress(1)
- MultiPressOngoing(1, 11)
- InitialPress(1)
- MultiPressOngoing(1, 14)
- MultiPressComplete(1, 14)
I modified a little your code and successfully tested it - the full 360 degrees rotation generates knob event with rotateAmount=100 in total. Please take a look at my suggestion and feel free to integrate it into your code it if you think it's worth applying.
Subscribe also to MultiPressComplete:
IkeaScrollFields.switch_scroll_subscribed_events = {
clusters.Switch.events.InitialPress.ID,
clusters.Switch.events.MultiPressOngoing.ID,
clusters.Switch.events.MultiPressComplete.ID,
}
Attach handler for the MultiPressComplete event:
matter_handlers = {
event = {
[clusters.Switch.ID] = {
[clusters.Switch.events.InitialPress.ID] = event_handlers.initial_press_handler,
[clusters.Switch.events.MultiPressOngoing.ID] = event_handlers.multi_press_ongoing_handler,
[clusters.Switch.events.MultiPressComplete.ID] = event_handlers.multi_press_complete_handler,
}
}
},
Handle the MultiPressComplete event:
IkeaScrollFields.SCROLL_STARTED = "__scroll_started"
function IkeaScrollEventHandlers.initial_press_handler(driver, device, ib, response)
-- use the generic handler logic for the push endpoints. Else, use custom logic.
if switch_utils.tbl_contains(scroll_fields.ENDPOINTS_PUSH, ib.endpoint_id) then
generic_event_handlers.initial_press_handler(driver, device, ib, response)
else
-- prevent multiple executions in the same scroll sequence
if not device:get_field(scroll_fields.SCROLL_STARTED) then
device:set_field(scroll_fields.LATEST_NUMBER_OF_PRESSES_COUNTED, 1)
rotate_amount_event_helper(device, ib.endpoint_id, 1)
device:set_field(scroll_fields.SCROLL_STARTED, 1)
end
end
end
function IkeaScrollEventHandlers.multi_press_complete_handler(driver, device, ib, response)
if switch_utils.tbl_contains(scroll_fields.ENDPOINTS_PUSH, ib.endpoint_id) then
generic_event_handlers.multi_press_complete_handler(driver, device, ib, response)
else
-- end the scroll sequence
device:set_field(scroll_fields.SCROLL_STARTED, nil)
end
end
There was a problem hiding this comment.
Hi @perexis, thank you for reporting this. I did some investigation and saw the same behavior. In the current implementation, I have chosen to ignore the InitialPress notification altogether in favor of MultiPressComplete, and in the interest of simplicity, to otherwise keep the logic the same as before.
Description of Change
Update scroll profile to support knob capability. Include unique event handlers for knob and initial press.
This breaks apart the work found in #2669 so that the knob capability support can be reviewed as an isolated unit.
Summary of Completed Tests
Unit test included. Tested on-device.