Add support for Irrigation System device type #2684
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 484 suites 0s ⏱️ Results for commit a78d435. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against a78d435 |
|
|
||
| SwitchFields.operational_state_command_map = { | ||
| [clusters.OperationalState.commands.Pause.ID] = "pause", | ||
| [clusters.OperationalState.commands.Resume.ID] = "resume" |
There was a problem hiding this comment.
Note that only pause and resume (and not start or stop) are supported for Irrigation System by the spec
| capabilities: | ||
| - id: valve | ||
| version: 1 | ||
| - id: level |
There was a problem hiding this comment.
Note that in 66bf122 I reimplemented level as an optional capabilty in this profile (as well as added flowSensor + operationalState). I think this makes sense but let me know if you agree with this approach or not
nickolas-deboom
force-pushed
the
feature/irrigation-system
branch
from
66bf122 to
00cad14
Compare
drivers/SmartThings/matter-switch/src/switch_utils/device_configuration.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_utils/device_configuration.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_utils/device_configuration.lua
Outdated
Show resolved
Hide resolved
| if #valve_ep_ids > 1 then | ||
| table.remove(valve_ep_ids, 1) -- the first valve ep is accounted for in the main irrigation system profile | ||
| ValveDeviceConfiguration.create_child_devices(driver, device, valve_ep_ids, default_endpoint_id) |
There was a problem hiding this comment.
I might move this logic into the create_child_devices function itself- see the create_child_devices for onOff eps to see what I'm suggesting (since we do the same behavior there as well)
There was a problem hiding this comment.
We could also plug this logic within the second check, rather than doing an if-else.
Like:
if #valve_ep_ids > 0 then
... logic for default ep ...
if #valve_ep_ids > 1 then
... child stuff ...
end
end
There was a problem hiding this comment.
Since the default endpoint would be the one with the Irrigation System endpoint, but the parent device would also include the first Valve endpoint, I don't know if the logic in create_child_devices would work, so I implemented the second suggestion you provided. Lemme know how that looks!
| device:set_field(fields.SUPPORTED_COMPONENT_CAPABILITIES, total_supported_capabilities, { persist = true }) | ||
| end | ||
| else | ||
| device:try_update_metadata({profile = profile_name}) |
There was a problem hiding this comment.
I don't think this api should have 2 calls in 2 different places for try_update_metadata. I think we should work towards there being a single "path" that this logic follows.
I haven't looked at it super closely, but most of this could/should just be handled within the if #valve_ep_ids > 1 then logic block below?
There was a problem hiding this comment.
Yeah I agree, and re-did a lot of the logic in match_profile to match the flow for other device types in the latest commit. I think it's a lot more cohesive now, see this commit: 07eba51
| local valve_ep_ids = device:get_endpoints(clusters.ValveConfigurationAndControl.ID) | ||
|
|
||
| if is_irrigation_system then | ||
| updated_profile = "irrigation-system" |
There was a problem hiding this comment.
I think I'm a little bit unsure why we even have to make an irrigation-system profile? Why not just use a single modular water valve profile? This can also be used for the child devices as well?
There was a problem hiding this comment.
Also, if we're adding flow measurement, the spec defines that all water valves might have flow support optionally... may want to add that as well?
This would mean that as far as the profile, the only difference we'd see between irrigation-system and water-valve would be the operational state capability at this time. Since that can be handled modularly,
There was a problem hiding this comment.
However, I see you're using a different category for IrrigationSystem. So I like the idea of having the separate profile. We still may want to add flow to water valve though
| local flow_bound = ib.data.value / 10.0 | ||
| local unit = "m^3/h" | ||
| set_field_for_endpoint(device, FLOW_BOUND_RECEIVED..minOrMax, ib.endpoint_id, flow_bound) | ||
| local min = get_field_for_endpoint(device, FLOW_BOUND_RECEIVED..FLOW_MIN, ib.endpoint_id) |
There was a problem hiding this comment.
I don't think these fields are actually mapped to anything as they are now, since these are defined in fields.lua. Can you add some tests for flow measurement?
There was a problem hiding this comment.
Nice catch, I fixed the issues with these fields and added in some test cases!
| table.insert(total_supported_capabilities[MAIN_COMPONENT_IDX][CAPABILITIES_LIST_IDX], capabilities.refresh.ID) | ||
| table.insert(total_supported_capabilities[MAIN_COMPONENT_IDX][CAPABILITIES_LIST_IDX], capabilities.firmwareUpdate.ID) | ||
|
|
||
| device:set_field(fields.SUPPORTED_COMPONENT_CAPABILITIES, total_supported_capabilities, { persist = true }) |
There was a problem hiding this comment.
If we're going to add support for pre-api 15, we'll also have to extend in init and info_changed some logic like this:
if device:get_field(fields.SUPPORTED_COMPONENT_CAPABILITIES) and (version.api < 15 or version.rpc < 9) then
-- assume that device is using a modular profile on 0.57 FW, override supports_capability_by_id
-- library function to utilize optional capabilities
device:extend_device("supports_capability_by_id", aqs_utils.supports_capability_by_id_modular)
end
I'm not totally convinced this is a worthwhile path to take though... but that may just be me
There was a problem hiding this comment.
I removed this block, because I also don't really think it's worth supporting. Plus, it's not being done for other modular devices such as Fan so it probably makes more sense to keep it consistent
This adds support for the Irrigation System device type, introduced with Matter 1.5.
nickolas-deboom
force-pushed
the
feature/irrigation-system
branch
from
00cad14 to
70d2a00
Compare
This adds support for the Irrigation System device type, introduced with Matter 1.5.