Add support for Irrigation System device type #2684
Conversation
This adds support for the Irrigation System device type, introduced with Matter 1.5.
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 481 suites 0s ⏱️ For more details on these errors, see this check. Results for commit 00cad14. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 00cad14 |
|
|
||
| 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
system This commit adds support for optional clusters: * OperationalState * FlowMeasurement Additionally, the level, operationalState, and flowSensor capabilities are supported via modular profiling.
nickolas-deboom
force-pushed
the
feature/irrigation-system
branch
from
66bf122 to
00cad14
Compare
| for _, ep in ipairs(device.endpoints) do | ||
| if ep.endpoint_id == valve_ep_id then |
There was a problem hiding this comment.
we can use something like local ep_info = switch_utils.get_endpoint_info(valve_ep_id) helper to do this a little more easily
|
|
||
| for _, ep in ipairs(device.endpoints) do | ||
| if ep.endpoint_id == valve_ep_id then | ||
| if switch_utils.find_cluster_on_ep(ep, clusters.ValveConfigurationAndControl.ID) then |
There was a problem hiding this comment.
this helper can take the feature map as a third parameter, might simplify this a bit more
| local is_irrigation_system = false | ||
| for _, ep in ipairs(device.endpoints) do | ||
| for _, dt in ipairs(ep.device_types) do | ||
| if dt.device_type_id == fields.DEVICE_TYPE_ID.IRRIGATION_SYSTEM then |
There was a problem hiding this comment.
maybe use the get_endpoints_by_device_type helper here?
| 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
| 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?
| 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?
| 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
This adds support for the Irrigation System device type, introduced with Matter 1.5.