zigbee-switch: Filter incorrect network type #2445
Conversation
|
Invitation URL: |
Test Results 71 files 455 suites 0s ⏱️ Results for commit 9bbd9f5. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 9bbd9f5 |
tandres
requested review from
greens,
varzac,
aleclorimer and
cjswedes
and removed request for
greens
|
I think rather than including the entire driver.lua file here (since we often have to leave workarounds like this in for a long time due to slow to update hubs), I would rather we just override the individual functions. |
|
|
||
| function Driver:_filter_network_type(raw_device_table) | ||
| if self.device_network_type_filter then | ||
| return self.device_network_type_filter[raw_device_table.network_type] ~= nil |
There was a problem hiding this comment.
| return self.device_network_type_filter[raw_device_table.network_type] ~= nil | |
| return self.device_network_type_filter[raw_device_table.network_type] or false |
Not critical, but in case we ever add an entry with false, this would still work.
| end | ||
|
|
||
| test.set_test_init_function(test_init) | ||
| test.register_test("mismatched_prot_ignored", function() end, nil) |
There was a problem hiding this comment.
I don't understand this test.
There was a problem hiding this comment.
I pushed up some additional comments on the test. It's just making sure the driver doesn't crash when you add a ZWave device to it.
|
I agree with @varzac . These "temporary" fixes tend to stick around for a while. I think it would be better to write this as a sub-driver full of no-ops. |
I hadn't considered a subdriver full of noops. Originally I was thinking that may not be sufficient because any functions in the driver class may be assuming a specific device type. But looking at the zigbee driver class, the only place the device object is referenced is if the |
tandres
force-pushed
the
CHAD-16558
branch
2 times, most recently
from
a675b27 to
0843dc6
Compare
| local log = require "log" | ||
|
|
||
| local function can_handle(opts, driver, device) | ||
| if device.network_type ~= st_device.NETWORK_TYPE_ZIGBEE then |
There was a problem hiding this comment.
Child devices shouldn't use this subdriver either.
There was a problem hiding this comment.
Can you add a test, where a non zigbee device has a capability command come in, to make sure that doesnt crash. I also think that infoChanged could cause problems, so please add that to the existing test.
One thing I liked about monkey patching the driver, is that it more deeply prevented the device from being used in by anything in the zigbee driver than the subdriver approach does.
A small number of zwave devices failed to migrate correctly and ended up as zwave devices attached to the zigbee switch driver. This change adds a subdriver (non_zigbee_devices) which handles non-zigbee devices safely without crashing the driver. https://smartthings.atlassian.net/browse/CHAD-16558
Info changed added, also added a command test which doesn't seem to break anything. My driver skillz are weak so I appreciate the in-depth review! |
A small number of zwave devices failed to migrate correctly and ended up as zwave devices attached to the zigbee switch driver. This change adds a patched driver.lua which includes a mechanism to filter devices when building device info which don't match device type filters added to the driver.
https://smartthings.atlassian.net/browse/CHAD-16558
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests