Change use of the EnergyExported attribute to EnergyImported in matter switch #1830
Conversation
hcarter-775
changed the title
|
Invitation URL: |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 438b625 |
Test Results 64 files 402 suites 0s ⏱️ Results for commit 438b625. ♻️ This comment has been updated with latest results. |
| @@ -0,0 +1,68 @@ | |||
| local cluster_base = require "st.matter.cluster_base" | |||
There was a problem hiding this comment.
Would we be able to remove the CumulativeEnergyExported and PeriodicEnergyExported embedded cluster defs now that we are just using the imported variation?
There was a problem hiding this comment.
We could. I figured why not leave them in, but it's just as well to take them out
There was a problem hiding this comment.
removed in 438b6252f08fba1d
| 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 }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
To note: I have not removed anything- only renamed the word export to import.
There was a problem hiding this comment.
Oh, nevermind I see what you mean. Thanks for pointing that out!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Check all that apply
Type of Change
Checklist
Description of Change
According to internal review as well as MTR-873, the ElectricalEnergyMeasurement cluster's imported attributes should be used for positive energy values rather than the exported attributes. This change commits that statement to the drivers.
Summary of Completed Tests
All of the extensive unit tests have been converted from exported to imported tests, which properly handles all of the cases we're most interested in.