-
Notifications
You must be signed in to change notification settings - Fork 511
ZLL: Use Native / Default handlers #2440
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
Invitation URL: |
Test Results 71 files 455 suites 0s ⏱️ For more details on these failures, see this check. Results for commit 407d268. ♻️ This comment has been updated with latest results. |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 407d268 |
f541dd9
to
5a0c5ab
Compare
cce4958
to
7fc35dc
Compare
32d969f
to
6e40aa4
Compare
|
||
test.socket.zigbee:__expect_send({ mock_device.id, OnOff.attributes.OnOff:read(mock_device) }) | ||
test.socket.zigbee:__expect_send({ mock_device.id, Level.attributes.CurrentLevel:read(mock_device) }) | ||
if version.api <= 15 then |
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 was having issues running the setColorTemperature
tests locally with this operator set to <=
(no issues with <
). Would this be pointing to a problem with how I'm using the version? Or is there some Lua magic that I'm missing?
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.
Well the way you've written things, wouldn't the initial on/offs also not be sent when the native handlers are used?
@cjswedes probably has more insight here.
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.
Why are there version checks at all for the polling/read messages? I expected there to only be a version check on the native handler registration call. The polling messages after the fact should remain the same whether the lua libs default handler is called or the subdriver is used because.
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 entire zll-dimmer-bulb
subdriver will no longer be necessary on api version 16 since we will put zll handling into the default handlers, so I think rather than changing the can_handle, it would be preferable to instantiate the subdriver conditionally based on the api version in the base driver
@cjswedes is there that much difference between having the lazy-loaded |
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.
LGTM!
I think there is a decent sized difference. When we startup and "lazily" load the subdrivers, what were actually doing is the following:
So some differences with what I suggested are:
I dont think the latency and memory savings are that much, but it would be nice to get rid of a subdriver since it isnt needed anymore. Why cut out every individual handler when you can just cut out the whole subdriver? |
3cfeae0
to
10939c4
Compare
add: `profile_id` to unit test
…or testing with version 16): - adding `E13-N11` to `zll-polling` allowed `doConfigure` test of `test_sengled_dimmer_bulb_with_motion_sensor.lua` to pass - requiring the `zll-dimmer-bulb/ikea-xy-color-bulb` subdriver in the `zigbee-switch` driver allowed all the tests to pass in `test_zll_rgb_bulb.lua`
Type of Change
Checklist
Description of Change
This pull request defaults handing for
on
,off
,set_level
, andset_color_temperature
in the ZLL sub-driver back to the mainzigbee-switch
driver.Summary of Completed Tests