Add support for Inovelli VTM31-SN Dimmer Switch #1754
Conversation
nickolas-deboom
requested review from
tpmanley,
ctowns,
z-michel and
hcarter-775
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 64 files 402 suites 0s ⏱️ Results for commit 3405324. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 3405324 |
|
|
||
| local LATEST_CLOCK_SET_TIMESTAMP = "latest_clock_set_timestamp" | ||
|
|
||
| local preference_map_inovelli_vtm31sn = { |
There was a problem hiding this comment.
It would make sense to move preferences that are specific to a certain device to a sub driver if we are able to. Or these configs could be moved to their own file at least to keep it separate from the base driver where we should try to handle things as generically as possible
There was a problem hiding this comment.
I moved the inovelli-specific handling to a subdriver in 2e8b4b5
| for id, value in pairs(device.preferences) do | ||
| if args.old_st_store.preferences[id] ~= value and preferences and preferences[id] then | ||
| local new_parameter_value = preferences_to_numeric_value(device.preferences[id]) | ||
| local req = clusters.ModeSelect.server.commands.ChangeToMode(device, preferences[id].parameter_number, new_parameter_value) |
There was a problem hiding this comment.
If the mode cluster is going to be used here, this would be the first use of it in the matter-switch driver. This means we'd need to add it as an embedded cluster so it could be used on older FW that doesn't support the mode cluster via the lua libs definitions yet.
There was a problem hiding this comment.
It looks like ModeSelect has been been in the spec since matter 1.0. Is there a way to check when it was first added to the lua libs?
| return | ||
| end | ||
|
|
||
| if device.manufacturer_info.vendor_id == fingerprint_profile_overrides[1].vendor_id and |
There was a problem hiding this comment.
This code is somewhat confusing and I think it could use some comments or documentation somewhere since there is some device specific knowledge here. For example, why is the time diff here and what is the significance of the values?
There was a problem hiding this comment.
I believe this code is used to prevent preferences from being changed faster than every two seconds. This was the same way that Inovelli implemented this for the zigbee driver, so it could be a device limitation. I will add some comments to make things a little more clear.
| @@ -571,6 +585,15 @@ local function initialize_switch(driver, device) | |||
| device:set_field(COMPONENT_TO_ENDPOINT_MAP_BUTTON, component_map, {persist = true}) | |||
| end | |||
|
|
|||
| -- If there is a custom static profile for the device, configure buttons if needed and | |||
There was a problem hiding this comment.
Since the inovelli switch already has a sub driver, I would say we should remove this override from the main driver and just add the special handling for the init event in the inovelli sub driver. This way we can try to keep the base driver as generic as possible (although there are already exceptions to this).
So, I would just copy whatever functionality is needed from the base driver and then put it in the sub driver for the init function, and then trim whatever is not needed. Better yet, you could move the configure_buttons function to its own file so that it can be shared between the base and sub driver, similar to what is done with the embedded-cluster-utils module.
Type of Change
Checklist
Description of Change
CHAD-14347
This change adds support for the Inovelli VTM31-SN Dimmer Switch. Initial support for this device was added by PR 1663. This PR includes a profile for this device with embedded preferences as well as handling for these preferences.
The preferences were created to match those defined in Inovelli's matter driver for this device.
Summary of Completed Tests
See here for screenshots from testing with the device.
Also see test_matter_multi_button_switch_mcd.lua which tests a mock device with a similar endpoint structure as the Inovelli device.