energy-management-app: refactor WH and EVSE into separate endpoints and fix conformance issues #36201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR adds the missing Water Heater device to matter-devices.xml. The description was generated using the Alchemy tool (https://github.com/project-chip/alchemy) with the following command: `alchemy zap --attribute="in-progress" --sdkRoot=./connectedhomeip/ --specRoot=./connectedhomeip-spec/ ./connectedhomeip-spec/src/device_types/WaterHeater.adoc` I manually fixed the device nae from `Matter Water Heater` to `Water Heater`.
This PR refactors the energy-management-app into 2 separate endpoints (one for EVSE and another for WaterHeater). This is the first step in making this app spec-conformant. `TC_DeviceBasicComposition.py` failed on this app before this PR and now passes. Changes: * Split Water Heater and EVSE into two separate endpoints (1 and 2). Updated zap and code. * Dinamically disable unused endpoint at runtime. Based on the app choice (command line argument on linux or #define in ESP32 or SIlabs), initialize the clusters in the correct endpoint and disable the other endpoint. For example, for Water Heater, initialize clusters on endpoint 2 and disable endpoint 1 (EVSE). * Refactor/move the init code related to ElectricalSensor (PowerTopology, EPM and EEM) from inside EVSE into ElectricalSensorInit.h/.cpp so they can be easier to reuse by both WaterHeater and EVSE. * Refactor/move DEM cluster init code into its own file so it can be better reused outside EVSE. Test performed: 1. Check basic composition for EVSE: ``` scripts/run_in_python_env.sh out/python_env './scripts/tests/run_python_test.py --app ./out/linux-x64-energy-management-no-ble/chip-energy-management-app --app-args "--application evse --trace-to json:log" --script src/python_testing/TC_DeviceBasicComposition.py --script-args "--qr-code MT:-24J0AFN00KA0648G00"' ``` 2. Check basic composition for WaterHeater: ``` scripts/run_in_python_env.sh out/python_env './scripts/tests/run_python_test.py --app ./out/linux-x64-energy-management-no-ble/chip-energy-management-app --app-args "--application water-heater --trace-to json:log" --script src/python_testing/TC_DeviceBasicComposition.py --script-args "--qr-code MT:-24J0AFN00KA0648G00"' ``` 3. Check app against `TC_EEVSE_2_6.py`: ``` rm -f evse.bin; ./out/linux-x64-energy-management-no-ble/chip-energy-management-app --enable-key 000102030405060708090a0b0c0d0e0f --KVS evse.bin --featureSet 0x3d python src/python_testing/TC_EEVSE_2_6.py --endpoint 1 -m on-network -n 1234 -p 20202021 -d 3840 --hex-arg enableKey:000102030405060708090a0b0c0d0e0f ```
...y-management-app/energy-management-common/common/include/EnergyManagementAppCmdLineOptions.h
Outdated
Show resolved
Hide resolved
...ples/energy-management-app/energy-management-common/device-energy-management/src/DEMInit.cpp
Outdated
Show resolved
Hide resolved
...ergy-management-app/energy-management-common/energy-reporting/include/ElectricalSensorInit.h
Show resolved
Hide resolved
|
PR #36201: Size comparison from 0b93b0d to ea06d89 Full report (7 builds for cc32xx, qpg, stm32, tizen)
|
tcarmelveilleux
approved these changes
Oct 22, 2024
|
PR #36201: Size comparison from 0b93b0d to f05afff Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #36201: Size comparison from 692983e to 9dee3de Full report (49 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
It seems there is an issue with |
|
Could you restart the |
|
@lboue re-ran that job but it failed again so it doesn't seem like a flake. |
Thanks, @lboue and @kiel-apple for double checking the REPL tests. I found the culprit!
With this command: So that pointed me to likely an issue with the CI scripts itself. I investigated a bit and found out the |
Remove PowerSource from root node and move to EP1/evse Bump cluster revisions where needed Fix device type descriptions for each EP Remove unused Thermostat cluster and added a TODO. We need to properly implement this cluster for this app for temperature control.
|
PR #36201: Size comparison from e1fbaf6 to c0d14a2 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36201: Size comparison from e1fbaf6 to 2d92654 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
@soares-sergio Thank you for your patches. The |
soares-sergio
changed the title
soares-sergio
changed the title
jamesharrow
reviewed
Nov 21, 2024
.../energy-management-app/energy-management-common/common/src/EnergyManagementAppCommonMain.cpp
Show resolved
Hide resolved
PeterC1965
reviewed
Nov 25, 2024
...anagement-app/energy-management-common/device-energy-management/src/DEMTestEventTriggers.cpp
Show resolved
Hide resolved
|
This all looks good to me. |
|
This branch has conflicts with the file src/python_testing/TC_WHM_1_2.py (commit 926ab43) |
|
PR #36201: Size comparison from 29165e9 to d2f26a1 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Just fixed it, thanks! And thanks for reviewing @PeterC1965 . |
yyzhong-g
pushed a commit
to yyzhong-g/connectedhomeip
that referenced
this pull request
Dec 12, 2024
…nd fix conformance issues (project-chip#36201) * Add missing Water Heater device to matter-devices.xml This PR adds the missing Water Heater device to matter-devices.xml. The description was generated using the Alchemy tool (https://github.com/project-chip/alchemy) with the following command: `alchemy zap --attribute="in-progress" --sdkRoot=./connectedhomeip/ --specRoot=./connectedhomeip-spec/ ./connectedhomeip-spec/src/device_types/WaterHeater.adoc` I manually fixed the device nae from `Matter Water Heater` to `Water Heater`. * zap regen * energy-management-app: Split WH and EVSE into 2 endpoints This PR refactors the energy-management-app into 2 separate endpoints (one for EVSE and another for WaterHeater). This is the first step in making this app spec-conformant. `TC_DeviceBasicComposition.py` failed on this app before this PR and now passes. Changes: * Split Water Heater and EVSE into two separate endpoints (1 and 2). Updated zap and code. * Dinamically disable unused endpoint at runtime. Based on the app choice (command line argument on linux or #define in ESP32 or SIlabs), initialize the clusters in the correct endpoint and disable the other endpoint. For example, for Water Heater, initialize clusters on endpoint 2 and disable endpoint 1 (EVSE). * Refactor/move the init code related to ElectricalSensor (PowerTopology, EPM and EEM) from inside EVSE into ElectricalSensorInit.h/.cpp so they can be easier to reuse by both WaterHeater and EVSE. * Refactor/move DEM cluster init code into its own file so it can be better reused outside EVSE. Test performed: 1. Check basic composition for EVSE: ``` scripts/run_in_python_env.sh out/python_env './scripts/tests/run_python_test.py --app ./out/linux-x64-energy-management-no-ble/chip-energy-management-app --app-args "--application evse --trace-to json:log" --script src/python_testing/TC_DeviceBasicComposition.py --script-args "--qr-code MT:-24J0AFN00KA0648G00"' ``` 2. Check basic composition for WaterHeater: ``` scripts/run_in_python_env.sh out/python_env './scripts/tests/run_python_test.py --app ./out/linux-x64-energy-management-no-ble/chip-energy-management-app --app-args "--application water-heater --trace-to json:log" --script src/python_testing/TC_DeviceBasicComposition.py --script-args "--qr-code MT:-24J0AFN00KA0648G00"' ``` 3. Check app against `TC_EEVSE_2_6.py`: ``` rm -f evse.bin; ./out/linux-x64-energy-management-no-ble/chip-energy-management-app --enable-key 000102030405060708090a0b0c0d0e0f --KVS evse.bin --featureSet 0x3d python src/python_testing/TC_EEVSE_2_6.py --endpoint 1 -m on-network -n 1234 -p 20202021 -d 3840 --hex-arg enableKey:000102030405060708090a0b0c0d0e0f ``` * Use anon namespace instead of static. * disable enpoint on esp32 and silabs * address Tennessee's PR feedback * Review suggestion: move GetMainAppEndpointId to another file * zap regen * fix matter-devices white space diffs * App now builds on all-clusters app * Fix typo on init * fix all-clusters breakage on esp32 * fix misuse of namespace in header * Fix breakage for silabs water heater * Update WaterHeater endpoint for CI tests This fixes REPL test CI breakage. * Update WaterHeater tests to endpoint 2 * Bumped ClusterRevisions Cluster 40 (0x28) BasicInformation - 3 -> 4 Cluster 47 (0x2f) PowerSource - 2 -> 3 Cluster 48 (0x30) GeneralCommissioning 1-> 2 Cluster 3 (0x03) Identify 4 -> 5 Cluster 153 (0x99) EnergyEvse 2 -> 3 Cluster 157 (0x9d) EnergyEvseMode 1-> 2 Cluster 159 (0x9f) DeviceEnergyManagementMode 1-> 2 * Update AccessControl featureMap to enable Extension attribute * Remove AccessControl extension attribute * Remove kStateForecastReporting from the default feature map That feature can't be enabled together with kPowerAdjustment. * Update TC_WHM_1_2 endpoint to 2 * Fix various conformance issues Remove PowerSource from root node and move to EP1/evse Bump cluster revisions where needed Fix device type descriptions for each EP Remove unused Thermostat cluster and added a TODO. We need to properly implement this cluster for this app for temperature control. * TC_WHM_1_2: Use endpoint id passed as argument instead of hardcoded
This was referenced Dec 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the energy-management-app into 2 separate endpoints
(EP1 for EVSE and EP2 for WaterHeater) and fixes almost all of the device conformance issues.
Main contributions from this PR:
TC_DeviceBasicComposition.pyfailed on this app before this PR and now passes.TC_DeviceConformance.pyfailed more than 13 tests for each app and now only fails one forevseand two for water-heater.Changes:
(command line argument on linux [
evseofwater-heater] or #define in ESP32 or SIlabs).disable endpoint 1 (EVSE).
evseand into a common directory that can be shared between EVSE and WaterHeater.evse, only unitializeevserelated clusters. Same for WaterHeaterTC_WHM_xyztest scripts to use the correct endpoint passed in as argumentTests performed:
TC_EEVSE_2_6.py:TC_WHM_1_2andTC_WHM_2_1EVSE app Conformance failures before this PR:
EVSE app conformance failures after this PR (only one :)):
This failure is expected as the example apps have both ET and WI implemented, while this is disallowed in a real product.