Matter Switch: Lazy load subdrivers if possible #2525
Conversation
This change utilizes the new `lazy_load_sub_driver_v2` api to lazy load subdrivers.
nickolas-deboom
requested review from
cjswedes,
ctowns,
gharveymn and
hcarter-775
|
Invitation URL: |
Test Results 71 files 464 suites 0s ⏱️ Results for commit cec569c. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against cec569c |
drivers/SmartThings/matter-switch/src/utils/lazy_load_subdriver.lua
Outdated
Show resolved
Hide resolved
|
|
||
| return function(opts, driver, device ) | ||
| local EVE_MANUFACTURER_ID = 0x130A | ||
| if device.network_type == require("st.device").NETWORK_TYPE_MATTER and |
There was a problem hiding this comment.
similar to my other comment, I generally believe the require's should be outside the functions in the case it is a static require like this, where we know what we're requiring ahead of time. same goes for the other can_handle functions
There was a problem hiding this comment.
I believe that limiting the scope of require's like this was brought up as part of edge drivers api 2.0, to avoid holding onto references unless needed as well as avoiding pulling in files at the top level when possible.
There was a problem hiding this comment.
Hmmm, that makes sense I think. The thing is though, this st.device is already being pulled in at the top level in init.lua, so this is kinda just an extra operation, right?
There was a problem hiding this comment.
We probably need to better structure some stuff to make it more clear what is actually loaded at a given time, but in this case, st.device will always already be loaded in, so it's fine if you do a static require. Alternatively, maybe you can access it through the device argument of the function?
There was a problem hiding this comment.
This is just comparing the network type retrieved from the device argument against a string value from the lua libs - so I could replace require("st.device").NETWORK_TYPE_MATTER with the string, but not sure if that's worth it considering this module is already loaded at this point as Harrison pointed out.
There was a problem hiding this comment.
IMO the require("module").some_field is not the easiest to read and would prefer a local variable for the require; whether that is local to the module or function isnt a huge deal, but it seems like we are favoring making it local to the function at this point.
There was a problem hiding this comment.
I am of the opinion we should do the requires at the module level for now, if only for general consistency across the Matter drivers. To me, making things "the same" creates a more cohesive environment with more clearly set expectations.
|
|
||
| return function(opts, driver, device) | ||
| local THIRD_REALITY_MK1_FINGERPRINT = { vendor_id = 0x1407, product_id = 0x1388 } | ||
| if device.network_type == require("st.device").NETWORK_TYPE_MATTER and |
There was a problem hiding this comment.
Separately but related, I was thinking we could pull this device-specific sub-driver stuff into the SwitchFields.vendor_overrides table? Then here we could call something like if switch_utils.get_product_override_field(device, "is_third_reality_mk1") then ... end or something like that?
We could do that for all these can_handle functions, since this may be good for moving all this "same" data into one central place
There was a problem hiding this comment.
It's technically a separate idea, but it's related enough where I may suggest implementing it here?
There was a problem hiding this comment.
That would definitely be nice, but I might favor doing that in a separate PR because currently, only the 3rd reality can_handle uses an exact vid and pid while the other two have slightly different checks, and I'm not sure if I want to change those here.
drivers/SmartThings/matter-switch/src/sub_drivers/aqara_cube/can_handle.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/eve_energy/can_handle.lua
Outdated
Show resolved
Hide resolved
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
Can you update the copywrite on all the files? Its nice to get that done when there are actual code changes for a driver in a release cycle. You can do in a follow up PR too, if you get it in this release cycle.
|
Also, with the PR I just merged, the switch_utils file now has a different name and is in a different directory (thus a conflict). Should be a simple fix, but I'm not sure how nicely |
|
Please make sure tests are run against 59 lua libs locally before merging to exercise these changes with |
This change utilizes the new
lazy_load_sub_driver_v2api to lazy load subdrivers in the matter switch driver.