Matter Switch: Support fan/light devices #1991
Conversation
nickolas-deboom
force-pushed
the
support-fan-with-light-devices
branch
2 times, most recently
from
bbbc400 to
4b7727c
Compare
nickolas-deboom
force-pushed
the
support-fan-with-light-devices
branch
from
9946633 to
ee09b1d
Compare
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 65 files 420 suites 0s ⏱️ Results for commit a5896f7. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against a5896f7 |
nickolas-deboom
force-pushed
the
support-fan-with-light-devices
branch
8 times, most recently
from
b631bb6 to
2d59ee9
Compare
This change adds support for fan/switch combination devices. A new multicomponent profile contains the fan capability in the main component as well as capabilities for a color light in a secondary comoponent. New logic is also introduced in the matter switch driver to support the component mapping for this profile.
nickolas-deboom
force-pushed
the
support-fan-with-light-devices
branch
from
2d59ee9 to
a5896f7
Compare
|
|
||
| --- device_type_supports_multicomponent_configuration helper function used to check | ||
| --- whether the device is currently supported by a profile for a multicomponent configuration. | ||
| local function device_type_supports_multicomponent_configuration(device, main_endpoint, secondary_endpoints, secondary_device_type) |
There was a problem hiding this comment.
I think pulling both the endpoints and the device type in as separate parameters is a little confusing. It could be better in this case to grab the endpoints based on the device type within this function
| if ep ~= main_endpoint then | ||
| local button_component = "button" .. component_num | ||
| component_map[button_component] = ep | ||
| local function build_component_map(device, main_endpoint, secondary_endpoints, secondary_device_type) |
There was a problem hiding this comment.
I think we should grab and sort the "secondary endpoints" in the function itself rather than pulling it in as a separate parameter
| configure_buttons(device) | ||
| return | ||
| build_component_map(device, main_endpoint, button_eps, GENERIC_SWITCH_ID) | ||
| build_profile(device, main_endpoint, button_eps, GENERIC_SWITCH_ID) |
There was a problem hiding this comment.
build_profile seems a little bit generic?
| @@ -56,6 +56,7 @@ local COMPONENT_TO_ENDPOINT_MAP = "__component_to_endpoint_map" | |||
| -- containing both button endpoints and switch endpoints will use this field | |||
| -- rather than COMPONENT_TO_ENDPOINT_MAP. | |||
| local COMPONENT_TO_ENDPOINT_MAP_BUTTON = "__component_to_endpoint_map_button" | |||
| local COMPONENT_TO_ENDPOINT_MAP_FAN = "__component_to_endpoint_map_fan" | |||
There was a problem hiding this comment.
This seems like a dark road to go down... since it only leads to more and more component to endpoint maps that are the same other than the name?
There was a problem hiding this comment.
Mainly, I worry about this because this is the kind of system that can't necessarily be removed once we add it.
| @@ -194,6 +196,21 @@ local child_device_profile_overrides_per_vendor_id = { | |||
| } | |||
| } | |||
|
|
|||
| local supported_mcd_configs_per_main_endpoint_device_type = { | |||
There was a problem hiding this comment.
I think modular profiles will make this kind of a moot point, no? Since we can just have all capabilities we could ever need in the main component, and then support any/all of the capabilties we'd ever need in x number of components? Then, we'd only really have to account for that "x" value, and the rest of this would already be handled.
I understand that this is not the case now, so I it's a little less clear how to proceed.
Type of Change
Checklist
Description of Change
This change adds support for an extended color light/fan combination device. A new profile for this device type was added by PR 2012. New logic is included in the matter switch driver to support the component mapping for this profile. This change also refactors some of the initialization logic to help support expanding the driver to cover more multi-type devices in the future.
Summary of Completed Tests
Tested with an Orein bathroom fan device. New unit tests also included.