Skip to content
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.

Sign up for GitHub

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

Change use of the EnergyExported attribute to EnergyImported in matter switch #1830

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Change use of the EnergyExported attribute to EnergyImported in matter switch #1830

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
065a092
initial commit
hcarter-775 Dec 17, 2024
1100449
update: function names to reflect new attribute
hcarter-775 Dec 17, 2024
438b625
remove: unused embedded attribute defs
hcarter-775 Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions ...ter-switch/src/ElectricalEnergyMeasurement/server/attributes/CumulativeEnergyImported.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
local cluster_base = require "st.matter.cluster_base"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we be able to remove the CumulativeEnergyExported and PeriodicEnergyExported embedded cluster defs now that we are just using the imported variation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. I figured why not leave them in, but it's just as well to take them out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in 438b6252f08fba1d

local data_types = require "st.matter.data_types"
local TLVParser = require "st.matter.TLV.TLVParser"

local CumulativeEnergyImported = {
ID = 0x0001,
NAME = "CumulativeEnergyImported",
base_type = require "st.matter.generated.zap_clusters.ElectricalEnergyMeasurement.types.EnergyMeasurementStruct",
}

function CumulativeEnergyImported:new_value(...)
local o = self.base_type(table.unpack({...}))
self:augment_type(o)
return o
end

function CumulativeEnergyImported:read(device, endpoint_id)
return cluster_base.read(
device,
endpoint_id,
self._cluster.ID,
self.ID,
nil --event_id
)
end

function CumulativeEnergyImported:subscribe(device, endpoint_id)
return cluster_base.subscribe(
device,
endpoint_id,
self._cluster.ID,
self.ID,
nil --event_id
)
end

function CumulativeEnergyImported:set_parent_cluster(cluster)
self._cluster = cluster
return self
end

function CumulativeEnergyImported:build_test_report_data(
device,
endpoint_id,
value,
status
)
local data = data_types.validate_or_build_type(value, self.base_type)
self:augment_type(data)
return cluster_base.build_test_report_data(
device,
endpoint_id,
self._cluster.ID,
self.ID,
data,
status
)
end

function CumulativeEnergyImported:deserialize(tlv_buf)
local data = TLVParser.decode_tlv(tlv_buf)
self:augment_type(data)
return data
end

setmetatable(CumulativeEnergyImported, {__call = CumulativeEnergyImported.new_value, __index = CumulativeEnergyImported.base_type})
return CumulativeEnergyImported

68 changes: 68 additions & 0 deletions ...atter-switch/src/ElectricalEnergyMeasurement/server/attributes/PeriodicEnergyImported.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
local cluster_base = require "st.matter.cluster_base"
local data_types = require "st.matter.data_types"
local TLVParser = require "st.matter.TLV.TLVParser"

local PeriodicEnergyImported = {
ID = 0x0003,
NAME = "PeriodicEnergyImported",
base_type = require "st.matter.generated.zap_clusters.ElectricalEnergyMeasurement.types.EnergyMeasurementStruct",
}

function PeriodicEnergyImported:new_value(...)
local o = self.base_type(table.unpack({...}))
self:augment_type(o)
return o
end

function PeriodicEnergyImported:read(device, endpoint_id)
return cluster_base.read(
device,
endpoint_id,
self._cluster.ID,
self.ID,
nil --event_id
)
end

function PeriodicEnergyImported:subscribe(device, endpoint_id)
return cluster_base.subscribe(
device,
endpoint_id,
self._cluster.ID,
self.ID,
nil --event_id
)
end

function PeriodicEnergyImported:set_parent_cluster(cluster)
self._cluster = cluster
return self
end

function PeriodicEnergyImported:build_test_report_data(
device,
endpoint_id,
value,
status
)
local data = data_types.validate_or_build_type(value, self.base_type)
self:augment_type(data)
return cluster_base.build_test_report_data(
device,
endpoint_id,
self._cluster.ID,
self.ID,
data,
status
)
end

function PeriodicEnergyImported:deserialize(tlv_buf)
local data = TLVParser.decode_tlv(tlv_buf)
self:augment_type(data)
return data
end

setmetatable(PeriodicEnergyImported, {__call = PeriodicEnergyImported.new_value, __index = PeriodicEnergyImported.base_type})
return PeriodicEnergyImported

84 changes: 42 additions & 42 deletions drivers/SmartThings/matter-switch/src/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,12 @@ local child_device_profile_overrides = {
local detect_matter_thing

local CUMULATIVE_REPORTS_NOT_SUPPORTED = "__cumulative_reports_not_supported"
local FIRST_EXPORT_REPORT_TIMESTAMP = "__first_export_report_timestamp"
local EXPORT_POLL_TIMER_SETTING_ATTEMPTED = "__export_poll_timer_setting_attempted"
local EXPORT_REPORT_TIMEOUT = "__export_report_timeout"
local TOTAL_EXPORTED_ENERGY = "__total_exported_energy"
local LAST_EXPORTED_REPORT_TIMESTAMP = "__last_exported_report_timestamp"
local RECURRING_EXPORT_REPORT_POLL_TIMER = "__recurring_export_report_poll_timer"
local FIRST_IMPORT_REPORT_TIMESTAMP = "__first_import_report_timestamp"
local IMPORT_POLL_TIMER_SETTING_ATTEMPTED = "__import_poll_timer_setting_attempted"
local IMPORT_REPORT_TIMEOUT = "__import_report_timeout"
local TOTAL_IMPORTED_ENERGY = "__total_imported_energy"
local LAST_IMPORTED_REPORT_TIMESTAMP = "__last_imported_report_timestamp"
local RECURRING_IMPORT_REPORT_POLL_TIMER = "__recurring_import_report_poll_timer"
local MINIMUM_ST_ENERGY_REPORT_INTERVAL = (15 * 60) -- 15 minutes, reported in seconds
local SUBSCRIPTION_REPORT_OCCURRED = "__subscription_report_occurred"
local CONVERSION_CONST_MILLIWATT_TO_WATT = 1000 -- A milliwatt is 1/1000th of a watt
Expand All @@ -183,44 +183,44 @@ local function iso8061Timestamp(time)
return os.date("!%Y-%m-%dT%H:%M:%SZ", time)
end

local function delete_export_poll_schedule(device)
local export_poll_timer = device:get_field(RECURRING_EXPORT_REPORT_POLL_TIMER)
if export_poll_timer then
device.thread:cancel_timer(export_poll_timer)
device:set_field(RECURRING_EXPORT_REPORT_POLL_TIMER, nil)
device:set_field(EXPORT_POLL_TIMER_SETTING_ATTEMPTED, nil)
local function delete_import_poll_schedule(device)
local import_poll_timer = device:get_field(RECURRING_IMPORT_REPORT_POLL_TIMER)
if import_poll_timer then
device.thread:cancel_timer(import_poll_timer)
device:set_field(RECURRING_IMPORT_REPORT_POLL_TIMER, nil)
device:set_field(IMPORT_POLL_TIMER_SETTING_ATTEMPTED, nil)
end
end

local function send_export_poll_report(device, latest_total_exported_energy_wh)
local function send_import_poll_report(device, latest_total_imported_energy_wh)
local current_time = os.time()
local last_time = device:get_field(LAST_EXPORTED_REPORT_TIMESTAMP) or 0
device:set_field(LAST_EXPORTED_REPORT_TIMESTAMP, current_time, { persist = true })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag will have been persisted in the case on devices that have already joined to the platform and run this code. However, it wouldn't be needed anymore so we should set it to nil and remove it.

However, I don't think there are a lot/any devices like this yet, so in this case we are okay to just remove this, but just something to be aware of for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To note: I have not removed anything- only renamed the word export to import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nevermind I see what you mean. Thanks for pointing that out!

Copy link
Contributor Author

@hcarter-775 hcarter-775 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update on this: from the superset query counting all matter profiles in use, I'm seeing:
Matter Switch plug-power-energy-powerConsumption 510, so there are 510 of these devices that would have run this code already. Note: this code would only run if the device supports powerConsumption, so no need to count any devices that don't profile to this.

@ctowns Do you think we should add extra handling to transition these devices?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you see what device these are? If it's the eve energy plug, then those devices would use the eve-energy sub driver, so then we technically wouldn't need to worry about clean up for those since they wouldn't have run this code, they would have just used the sub driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctowns These wouldn't be for the eve-energy driver, since those devices still fingerprints to this power-energy-powerConsumption. You can see these in the query as well: Matter Switch power-energy-powerConsumption 9516. The 512 would only have come from the 1.3 support

Copy link
Contributor

@ctowns ctowns Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for clarifying! So is it reasonable to assume that the 510 devices just aren't working in terms of power reports right now? If we are reading the wrong attribute, then I would assume the data we are getting is wrong so I'm not sure what "transition" we'd need to do clean up wise since the data would've just been wrong. I assume we can't change it once it's made it to ST energy. So in that case, I guess the only clean up we could do would be setting the export fields to nil. FWIW, ultimately this would save just a few bytes of memory per device.

I am surprised there are this many devices using this driver already, I would suggest we figure out what these devices are and try to procure one so we can test this functionality with a real device! That could give us some more confidence moving forward.

Copy link
Contributor Author

@hcarter-775 hcarter-775 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should be the case. In fact, if these devices are correctly using the 'Imported' attributes, we would not have read anything at all, so nothing would have even been reported to ST Energy in the first place, since the report reads TOTAL_IMPORTED_ENERGY, which is only set in the handlers.

Further, if anyone were incorrectly using the 'exported' attribute like us, there's this logic gate in the report:
if previous_imported_report and previous_imported_report.energy then energy_delta_wh = math.max(latest_total_imported_energy_wh - previous_imported_report.energy, 0.0) end
which will make sure it never reported a negative value.

Therefore, by this logic we shouldn't have reported anything incorrectly so far to ST Energy except sending a lot of nil values from the fact that we're never updating TOTAL_IMPORTED_ENERGY.

And as for saving a few bytes like you mentioned, I'm not sure this is worth it either.

local last_time = device:get_field(LAST_IMPORTED_REPORT_TIMESTAMP) or 0
device:set_field(LAST_IMPORTED_REPORT_TIMESTAMP, current_time, { persist = true })

-- Calculate the energy delta between reports
local energy_delta_wh = 0.0
local previous_exported_report = device:get_latest_state("main", capabilities.powerConsumptionReport.ID,
local previous_imported_report = device:get_latest_state("main", capabilities.powerConsumptionReport.ID,
capabilities.powerConsumptionReport.powerConsumption.NAME)
if previous_exported_report and previous_exported_report.energy then
energy_delta_wh = math.max(latest_total_exported_energy_wh - previous_exported_report.energy, 0.0)
if previous_imported_report and previous_imported_report.energy then
energy_delta_wh = math.max(latest_total_imported_energy_wh - previous_imported_report.energy, 0.0)
end

-- Report the energy consumed during the time interval. The unit of these values should be 'Wh'
device:emit_event(capabilities.powerConsumptionReport.powerConsumption({
start = iso8061Timestamp(last_time),
["end"] = iso8061Timestamp(current_time - 1),
deltaEnergy = energy_delta_wh,
energy = latest_total_exported_energy_wh
energy = latest_total_imported_energy_wh
}))
end

local function create_poll_report_schedule(device)
local export_timer = device.thread:call_on_schedule(
device:get_field(EXPORT_REPORT_TIMEOUT),
send_export_poll_report(device, device:get_field(TOTAL_EXPORTED_ENERGY)),
"polling_export_report_schedule_timer"
local import_timer = device.thread:call_on_schedule(
device:get_field(IMPORT_REPORT_TIMEOUT),
send_import_poll_report(device, device:get_field(TOTAL_IMPORTED_ENERGY)),
"polling_import_report_schedule_timer"
)
device:set_field(RECURRING_EXPORT_REPORT_POLL_TIMER, export_timer)
device:set_field(RECURRING_IMPORT_REPORT_POLL_TIMER, import_timer)
end

local function set_poll_report_timer_and_schedule(device, is_cumulative_report)
Expand All @@ -234,18 +234,18 @@ local function set_poll_report_timer_and_schedule(device, is_cumulative_report)
return
elseif not device:get_field(SUBSCRIPTION_REPORT_OCCURRED) then
device:set_field(SUBSCRIPTION_REPORT_OCCURRED, true)
elseif not device:get_field(FIRST_EXPORT_REPORT_TIMESTAMP) then
device:set_field(FIRST_EXPORT_REPORT_TIMESTAMP, os.time())
elseif not device:get_field(FIRST_IMPORT_REPORT_TIMESTAMP) then
device:set_field(FIRST_IMPORT_REPORT_TIMESTAMP, os.time())
else
local first_timestamp = device:get_field(FIRST_EXPORT_REPORT_TIMESTAMP)
local first_timestamp = device:get_field(FIRST_IMPORT_REPORT_TIMESTAMP)
local second_timestamp = os.time()
local report_interval_secs = second_timestamp - first_timestamp
device:set_field(EXPORT_REPORT_TIMEOUT, math.max(report_interval_secs, MINIMUM_ST_ENERGY_REPORT_INTERVAL))
device:set_field(IMPORT_REPORT_TIMEOUT, math.max(report_interval_secs, MINIMUM_ST_ENERGY_REPORT_INTERVAL))
-- the poll schedule is only needed for devices that support powerConsumption
if device:supports_capability(capabilities.powerConsumptionReport) then
create_poll_report_schedule(device)
end
device:set_field(EXPORT_POLL_TIMER_SETTING_ATTEMPTED, true)
device:set_field(IMPORT_POLL_TIMER_SETTING_ATTEMPTED, true)
end
end

Expand Down Expand Up @@ -700,7 +700,7 @@ end

local function device_removed(driver, device)
log.info("device removed")
delete_export_poll_schedule(device)
delete_import_poll_schedule(device)
end

local function handle_switch_on(driver, device, cmd)
Expand Down Expand Up @@ -979,33 +979,33 @@ local function occupancy_attr_handler(driver, device, ib, response)
device:emit_event(ib.data.value == 0x01 and capabilities.motionSensor.motion.active() or capabilities.motionSensor.motion.inactive())
end

local function cumul_energy_exported_handler(driver, device, ib, response)
local function cumul_energy_imported_handler(driver, device, ib, response)
if ib.data.elements.energy then
local watt_hour_value = ib.data.elements.energy.value / CONVERSION_CONST_MILLIWATT_TO_WATT
device:set_field(TOTAL_EXPORTED_ENERGY, watt_hour_value)
device:set_field(TOTAL_IMPORTED_ENERGY, watt_hour_value)
device:emit_event(capabilities.energyMeter.energy({ value = watt_hour_value, unit = "Wh" }))
end
end

local function per_energy_exported_handler(driver, device, ib, response)
local function per_energy_imported_handler(driver, device, ib, response)
if ib.data.elements.energy then
local watt_hour_value = ib.data.elements.energy.value / CONVERSION_CONST_MILLIWATT_TO_WATT
local latest_energy_report = device:get_field(TOTAL_EXPORTED_ENERGY) or 0
local latest_energy_report = device:get_field(TOTAL_IMPORTED_ENERGY) or 0
local summed_energy_report = latest_energy_report + watt_hour_value
device:set_field(TOTAL_EXPORTED_ENERGY, summed_energy_report)
device:set_field(TOTAL_IMPORTED_ENERGY, summed_energy_report)
device:emit_event(capabilities.energyMeter.energy({ value = summed_energy_report, unit = "Wh" }))
end
end

local function energy_report_handler_factory(is_cumulative_report)
return function(driver, device, ib, response)
if not device:get_field(EXPORT_POLL_TIMER_SETTING_ATTEMPTED) then
if not device:get_field(IMPORT_POLL_TIMER_SETTING_ATTEMPTED) then
set_poll_report_timer_and_schedule(device, is_cumulative_report)
end
if is_cumulative_report then
cumul_energy_exported_handler(driver, device, ib, response)
cumul_energy_imported_handler(driver, device, ib, response)
elseif device:get_field(CUMULATIVE_REPORTS_NOT_SUPPORTED) then
per_energy_exported_handler(driver, device, ib, response)
per_energy_imported_handler(driver, device, ib, response)
end
end
end
Expand Down Expand Up @@ -1174,8 +1174,8 @@ local matter_driver_template = {
[clusters.ElectricalPowerMeasurement.attributes.ActivePower.ID] = active_power_handler,
},
[clusters.ElectricalEnergyMeasurement.ID] = {
[clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported.ID] = energy_report_handler_factory(true),
[clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyExported.ID] = energy_report_handler_factory(false),
[clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported.ID] = energy_report_handler_factory(true),
[clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported.ID] = energy_report_handler_factory(false),
},
[clusters.ValveConfigurationAndControl.ID] = {
[clusters.ValveConfigurationAndControl.attributes.CurrentState.ID] = valve_state_attr_handler,
Expand Down Expand Up @@ -1234,8 +1234,8 @@ local matter_driver_template = {
clusters.PowerSource.attributes.BatPercentRemaining,
},
[capabilities.energyMeter.ID] = {
clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyExported,
clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyExported
clusters.ElectricalEnergyMeasurement.attributes.CumulativeEnergyImported,
clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported
},
[capabilities.powerMeter.ID] = {
clusters.ElectricalPowerMeasurement.attributes.ActivePower
Expand Down
Loading
Loading