-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Matter 1.4 Driver Release #1870
base: main
Are you sure you want to change the base?
Conversation
Invitation URL: |
matter-evse_coverage.xml
matter-hrap_coverage.xml
matter-switch_coverage.xml
matter-thermostat_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 6f0a524 |
7cd7eea
to
07a1493
Compare
This initial commit adds support for the mounted on/off control and mounted dimmable load control device types introduced by the matter 1.4 spec.
07a1493
to
d9cb0ed
Compare
Add initial driver support for HRAP devices
Duplicate profile check: Passed - no duplicate profiles detected. |
drivers/SmartThings/matter-hrap/src/ThreadBorderRouterManagement/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-hrap/src/ThreadBorderRouterManagement/server/attributes/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-hrap/src/ThreadBorderRouterManagement/types/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-hrap/src/WiFiNetworkManagement/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-hrap/src/ThreadBorderRouterManagement/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-hrap/src/WiFiNetworkManagement/init.lua
Outdated
Show resolved
Hide resolved
I left a few comments but everything else in the HRAP driver looks good to me! I can't remember but was there a file we've updated in the past to prevent github from checking test coverage of the embedded lua libs files? We could update that as part of this PR as well (I just can't remember where it is) |
I get an error when trying to package the hrap driver locally:
Is this happening due to the embedded capability definitions? |
The mounted device types will already join to the correct fingerprint and so do not require the logic added for them in initialize_switch to select the correct profile.
Signed-off-by: s-gatti <[email protected]> Co-authored-by: Hunsup Jung <[email protected]>
local function delete_reporting_timer(device) | ||
local reporting_poll_timer = device:get_field(RECURRING_REPORT_TIMER) | ||
if reporting_poll_timer ~= nil then | ||
device.thread:cancel_timer(reporting_poll_timer) | ||
device:set_field(RECURRING_REPORT_TIMER, nil) | ||
end | ||
end | ||
|
||
local function remove_timers(device) | ||
delete_reporting_timer(device) | ||
local poll_timer = device:get_field(RECURRING_POLL_TIMER) | ||
if poll_timer ~= nil then | ||
device.thread:cancel_timer(poll_timer) | ||
device:set_field(RECURRING_POLL_TIMER, nil) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like unnecessary breakdown of functionality to me. I would suggest:
local function delete_reporting_timer(device) | |
local reporting_poll_timer = device:get_field(RECURRING_REPORT_TIMER) | |
if reporting_poll_timer ~= nil then | |
device.thread:cancel_timer(reporting_poll_timer) | |
device:set_field(RECURRING_REPORT_TIMER, nil) | |
end | |
end | |
local function remove_timers(device) | |
delete_reporting_timer(device) | |
local poll_timer = device:get_field(RECURRING_POLL_TIMER) | |
if poll_timer ~= nil then | |
device.thread:cancel_timer(poll_timer) | |
device:set_field(RECURRING_POLL_TIMER, nil) | |
end | |
end | |
local function remove_timers(device) | |
-- remove report timer | |
local reporting_poll_timer = device:get_field(RECURRING_REPORT_TIMER) | |
if reporting_poll_timer ~= nil then | |
device.thread:cancel_timer(reporting_poll_timer) | |
device:set_field(RECURRING_REPORT_TIMER, nil) | |
end | |
-- remove poll timer | |
local poll_timer = device:get_field(RECURRING_POLL_TIMER) | |
if poll_timer ~= nil then | |
device.thread:cancel_timer(poll_timer) | |
device:set_field(RECURRING_POLL_TIMER, nil) | |
end | |
end |
local val = find_default_endpoint(device, clusters.Thermostat.ID) | ||
return val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this changed?
local previousTotalConsumptionWh = device:get_latest_state("main", capabilities.powerConsumptionReport | ||
.ID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local previousTotalConsumptionWh = device:get_latest_state("main", capabilities.powerConsumptionReport | |
.ID, | |
local previousTotalConsumptionWh = device:get_latest_state("main", capabilities.powerConsumptionReport.ID, |
clusters.ElectricalEnergyMeasurement.ID, | ||
{feature_bitmap = clusters.ElectricalEnergyMeasurement.types.Feature.CUMULATIVE_ENERGY }) | ||
if cumul_eps and #cumul_eps > 0 then | ||
local read_req = clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported:read(device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a loop here similar to below in case there are multiple endpoints containing the ElectricalEnergyMeasurement cluster? Or is it not expected to have multiple endpoints?
local read_req = clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported:read(device) | |
local read_req = clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported:read(device) | |
for i, ep in ipairs(cumul_eps) do | |
if i > 1 then | |
read_req:merge( clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported:read(device, cumul_eps[i])) | |
end | |
end |
if #electrical_sensor_eps > 0 then | ||
profile_name = "water-heater-power-energy-powerConsumption" | ||
end | ||
elseif #thermostat_eps > 0 and device_type ~= HEAT_PUMP_DEVICE_TYPE_ID then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this simple and implement it as:
elseif #thermostat_eps > 0 and device_type ~= HEAT_PUMP_DEVICE_TYPE_ID then | |
elseif device_type == HEAT_PUMP_DEVICE_TYPE_ID then | |
profile_name = "heat-pump" | |
elseif #thermostat_eps > 0 then |
This is more extensible and keeps the original logical flow. And to continue a comment I left earlier, rather than heat-pump
it should be
profile_name = "heat-pump-2-thermostat"
in the suggestion I made.
local thermostat_eps = get_endpoints_for_dt(device, THERMOSTAT_DEVICE_TYPE_ID) | ||
if #heat_pump_eps > 0 then | ||
local component_to_endpoint_map = { | ||
["thermostatOne"] = thermostat_eps[1], | ||
["thermostatTwo"] = thermostat_eps[2], | ||
} | ||
device:set_field(COMPONENT_TO_ENDPOINT_MAP, component_to_endpoint_map, {persist = true}) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local thermostat_eps = get_endpoints_for_dt(device, THERMOSTAT_DEVICE_TYPE_ID) | |
if #heat_pump_eps > 0 then | |
local component_to_endpoint_map = { | |
["thermostatOne"] = thermostat_eps[1], | |
["thermostatTwo"] = thermostat_eps[2], | |
} | |
device:set_field(COMPONENT_TO_ENDPOINT_MAP, component_to_endpoint_map, {persist = true}) | |
end | |
if #heat_pump_eps > 0 then | |
local thermostat_eps = get_endpoints_for_dt(device, THERMOSTAT_DEVICE_TYPE_ID) | |
local component_to_endpoint_map = { | |
["thermostatOne"] = thermostat_eps[1], | |
["thermostatTwo"] = thermostat_eps[2], | |
} | |
device:set_field(COMPONENT_TO_ENDPOINT_MAP, component_to_endpoint_map, {persist = true}) | |
end |
Though this is pretty un-extendable as-is. I do think we should extend this logic to permit 1, 2, or more thermostats, even if we only have a profile to support the 2 case for now.
if version.api < 11 then | ||
clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported:augment_type(ib.data) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my understanding, what is this logic doing?
if tbl_contains(cumul_eps, endpoint_id) then | ||
-- Since cluster at this endpoint supports both CUME & PERE features, we will prefer | ||
-- cumulative_energy_imported_handler to handle the energy report for this endpoint. | ||
return | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move this top the top of the function handler. There's no need to do any further logic if this is true.
} | ||
log.debug("component_to_endpoint_map " .. utils.stringify_table(component_to_endpoint_map)) | ||
device:set_field(COMPONENT_TO_ENDPOINT_MAP, component_to_endpoint_map, { persist = true }) | ||
local evse_eps = get_endpoints_for_dt(device, EVSE_DEVICE_TYPE_ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local evse_eps = get_endpoints_for_dt(device, EVSE_DEVICE_TYPE_ID) | |
local evse_eps = get_endpoints_for_dt(device, EVSE_DEVICE_TYPE_ID) or {} |
To prevent an error on the next line in the case that get_endpoints_for_dt
returns nil
[clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported.ID] = cumulative_energy_handler(TOTAL_CUMULATIVE_ENERGY_IMPORTED), | ||
[clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported.ID] = periodic_energy_handler(TOTAL_CUMULATIVE_ENERGY_IMPORTED), | ||
[clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported.ID] = cumulative_energy_handler(TOTAL_CUMULATIVE_ENERGY_EXPORTED), | ||
[clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported.ID] = periodic_energy_handler(TOTAL_CUMULATIVE_ENERGY_EXPORTED), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported.ID] = periodic_energy_handler(TOTAL_CUMULATIVE_ENERGY_EXPORTED), | |
[clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyExported.ID] = periodic_energy_handler(TOTAL_CUMULATIVE_ENERGY_EXPORTED), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was meant to be PeriodicEnergyExported
local energy_imported_Wh = utils.round(energy_imported_mWh / 1000) | ||
local cumulative_energy_imported = device:get_field(TOTAL_CUMULATIVE_ENERGY_IMPORTED_MAP) or {} | ||
cumulative_energy_imported[endpoint_id] = cumulative_energy_imported[endpoint_id] + energy_imported_Wh | ||
device:set_field(TOTAL_CUMULATIVE_ENERGY_IMPORTED_MAP, cumulative_energy_imported, { persist = true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we adding this map handling? Are we expecting devices that support energy control on multiple endpoints? As far as I see, neither of the two new thermostat device types support this.
local cumulative_energy_imported_mWh = ib.data.elements.energy.value | ||
local cumulative_energy_imported_Wh = utils.round(cumulative_energy_imported_mWh / 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local cumulative_energy_imported_mWh = ib.data.elements.energy.value | |
local cumulative_energy_imported_Wh = utils.round(cumulative_energy_imported_mWh / 1000) | |
local cumulative_energy_imported_Wh = utils.round( ib.data.elements.energy.value / 1000) -- convert mWh to Wh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing for the periodic energy handler
for i, mode in ipairs(supportWaterHeaterModes) do | ||
if i - 1 == currentMode then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this numerical ordering of the modes an assumption we can safely make?
) | ||
|
||
test.register_message_test( | ||
"Heating setpoint reports from child thermostat devices should emit correct events to the corret endpoint", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Heating setpoint reports from child thermostat devices should emit correct events to the corret endpoint", | |
"Heating setpoint reports from component thermostat devices should emit correct events to the corret endpoint", |
) | ||
|
||
test.register_message_test( | ||
"Cooling setpoint reports reports from child thermostat devices should emit correct events to the corret endpoint", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Cooling setpoint reports reports from child thermostat devices should emit correct events to the corret endpoint", | |
"Cooling setpoint reports reports from component thermostat devices should emit correct events to the correct endpoint", |
@@ -5,3 +5,13 @@ matterGeneric: | |||
- id: 0x0510 | |||
- id: 0x050C | |||
deviceProfileName: evse | |||
- id: "matter/solar-power" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change the name of this driver from matter-evse
to matter-energy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so.
@@ -115,6 +134,16 @@ local function tbl_contains(array, value) | |||
return false | |||
end | |||
|
|||
local get_total = function(map) | |||
if map ~= nil and type(map) == "table" then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave a debug message for the case where type(map) == "table"
if i > 1 then | ||
read_req:merge(clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported:read(device, ep)) | ||
read_req:merge( clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported:read(device, eps_to_read[i])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read_req:merge( clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported:read(device, eps_to_read[i])) | |
read_req:merge( clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported:read(device, ep) |
-- Consider only Solar Power / Battery Storage devices and sum up in case there are multiple endpoints. | ||
local battery_storage_eps = get_endpoints_for_dt(device, SOLAR_POWER_DEVICE_TYPE_ID) | ||
local solar_power_eps = get_endpoints_for_dt(device, BATTERY_STORAGE_DEVICE_TYPE_ID) | ||
if (tbl_contains(solar_power_eps, ib.endpoint_id) or tbl_contains(battery_storage_eps, ib.endpoint_id)) and ib.data.value then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic doesn't seem necessary, since we wouldn't be subscribing to this attribute in other device types in the first place.
|
||
active_power_map[endpoint_id] = watt_value | ||
local total_active_power = get_total(active_power_map) | ||
device:set_field(TOTAL_ACTIVE_POWER, active_power_map) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what devices do we expect to support multiple active power measurements?
local previousTotalConsumptionWh = device:get_latest_state(comp, capabilities.powerConsumptionReport | ||
.ID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local previousTotalConsumptionWh = device:get_latest_state(comp, capabilities.powerConsumptionReport | |
.ID, | |
local previousTotalConsumptionWh = device:get_latest_state(comp, capabilities.powerConsumptionReport.ID, |
-- 2. if the received setpoint command value is in range 86 ~ 176, it is inferred as *F | ||
local WATER_HEATER_MAX_TEMP_IN_C = 80.0 | ||
local WATER_HEATER_MIN_TEMP_IN_C = 30.0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a temperature range defined for Heat Pump? What would the expected range be for that device type? Currently it would use the 5-40 deg C range for thermostats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, a heat pump can just use the thermostat range, since it will be attached to regular thermostats.
end | ||
end | ||
|
||
local function periodic_energy_imported_handler(driver, device, ib, response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create test case(s) that exercise this function?
device:emit_event_for_endpoint(ib.endpoint_id, event) | ||
end | ||
|
||
local function water_heater_mode_handler(driver, device, ib, response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also create a test case for the water heater mode handler?
- name: SolarPanel | ||
- id: exportedEnergy | ||
capabilities: | ||
- id: powerConsumptionReport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reasoning for displaying the power Consumption report for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
powerConsumptionReport
is the capability used for Energy service
. In the long run, we wait for this information to be reported to the Energy service
.
capabilities: | ||
- id: thermostatMode | ||
version: 1 | ||
- id: relativeHumidityMeasurement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be a required trait. I think we should separate humidity into a separate profile that uses a -humidity
for each thermostat in the profile name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will modify it
Type of Change
Checklist
Description of Change
This PR contains changes to support new device types introduced by the Matter 1.4 specification. These device types include the following:
Summary of Completed Tests