-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Refactoring of Energy Management App #35616
base: master
Are you sure you want to change the base?
Refactoring of Energy Management App #35616
Conversation
…EPM out of EVSE and into bespoke files.
Review changes with SemanticDiff. |
PR #35616: Size comparison from 514e810 to 80a9f1c Full report (47 builds for bl602, bl702, bl702l, cyw30739, efr32, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
PR #35616: Size comparison from 514e810 to b1f742b Full report (66 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #35616: Size comparison from 514e810 to f60fd7d Increases above 0.2%:
Full report (45 builds for bl602, bl702, bl702l, cyw30739, esp32, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
|
||
chip::app::Clusters::DeviceEnergyManagement::DeviceEnergyManagementDelegate * GetDEMDelegate(); | ||
// TODO investigate this why isn't in in DEM? | ||
extern std::unique_ptr<chip::app::Clusters::DeviceEnergyManagementManager> gDEMInstance; | ||
extern std::unique_ptr<chip::app::Clusters::DeviceEnergyManagement::DeviceEnergyManagementDelegate> gDEMDelegate; |
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.
A general comment - can global variables be avoided? There is an accessor function GetDEMDelegate for the DEM delegate singleton but no accessor functions for the DEM instance.
Also not sure why gDEMDelegate is being exposed when there is a function to access it?
|
||
// Electrical Energy Measurement cluster uses ember to initialise | ||
extern std::unique_ptr<chip::app::Clusters::ElectricalEnergyMeasurement::ElectricalEnergyMeasurementAttrAccess> gEEMAttrAccess; | ||
|
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 comment about global variables. Can they be replaced with accessor functions?
WhmManufacturer(WaterHeaterManagementInstance * whmInstance) { mWhmInstance = whmInstance; } | ||
WhmManufacturer(WaterHeaterManagementInstance * whmInstance, | ||
ElectricalPowerMeasurement::ElectricalPowerMeasurementInstance * aEPMInstance, | ||
PowerTopology::PowerTopologyInstance * aPTInstance, DeviceEnergyManagementManager * aDEMInstance) |
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 the parameters be passed in by-reference instead of pointers? This would avoid having to check thereafter that the pointers are not null.
…pology-stub.cpp in place of PowerTopologyDelegate which does similar things in example-energy-management-app in all-clusters-app.
PR #35616: Size comparison from 7fd3d59 to 1ce153a Full report (86 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #35616: Size comparison from 7fd3d59 to e54c613 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
This PR moves some of the file contents to more appropriate places, so that the WaterHeater code can implement
EEM/EPM functions which were previously only in the EVSE source files.