Commodity tariff example. Using MockClock to provide the test time environment in the example #41552
+217
−78
Conversation
This file contains hidden or 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
… class. Some related fixes in server implementation
… class. Some related fixes in application
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a MockClock to provide a test time environment for the Commodity Tariff example. The changes include modifications to CommodityTariffInstance.h and CommodityTariffInstance.cpp to incorporate the mock clock, as well as updates to CommodityTariffEventTriggers.cpp to use the new test time control API. The goal is to enable easier testing and manipulation of time-dependent tariff behavior. I have identified some areas where improvements can be made, particularly around error handling and code clarity.
src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffInstance.cpp
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffEventTriggers.cpp
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffEventTriggers.cpp
Show resolved
Hide resolved
|
PR #41552: Size comparison from 7e83b9f to 67b49cd Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp
Outdated
Show resolved
Hide resolved
|
PR #41552: Size comparison from 7e83b9f to ce9fc92 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41552 +/- ##
=======================================
Coverage 51.05% 51.06%
=======================================
Files 1385 1385
Lines 100882 100882
Branches 13055 13054 -1
=======================================
+ Hits 51508 51516 +8
+ Misses 49374 49366 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the time management in the commodity tariff example to use MockClock, which is a great improvement for creating a controlled test time environment. The changes are generally well-implemented, replacing a manual time-tracking mechanism with a more robust solution that leverages the system's mock clock capabilities. I've identified a high-severity issue where a timer is not cancelled upon disabling the test time, which could lead to resource leaks and continuous errors. I've also noted a few medium-severity issues, including an unused variable and a minor typo in a log message. Overall, this is a solid enhancement to the example's testability.
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffInstance.cpp
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/include/CommodityTariffInstance.h
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffEventTriggers.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp
Outdated
Show resolved
Hide resolved
|
PR #41552: Size comparison from 7e83b9f to 89c957b Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the time management in the commodity tariff example to use MockClock for testing purposes. This is a good improvement, centralizing time control and making it more aligned with the SDK's testing practices. The changes are well-structured, replacing the old time tracking mechanism with a new, cleaner API (EnableTestTime, AdvanceTestTime).
I've identified a few minor issues: one related to an incorrect function call after disabling test time, a logging issue that could cause an overflow, and a suggestion to improve encapsulation of the mock clock state. Overall, this is a solid contribution that improves the testability of the example.
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffEventTriggers.cpp
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffInstance.cpp
Outdated
Show resolved
Hide resolved
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffInstance.cpp
Outdated
Show resolved
Hide resolved
|
PR #41552: Size comparison from 7e83b9f to d72ab5a Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #41552: Size comparison from 4f17295 to 623580a Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
jamesharrow
reviewed
Oct 23, 2025
src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp
Outdated
Show resolved
Hide resolved
jamesharrow
reviewed
Oct 23, 2025
examples/energy-gateway-app/commodity-tariff/src/CommodityTariffInstance.cpp
Outdated
Show resolved
Hide resolved
|
PR #41552: Size comparison from 4f17295 to 97ce5b7 Increases above 0.2%:
Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41552: Size comparison from ad60c95 to 9b37306 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41552: Size comparison from ad60c95 to 2ab01e2 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41552: Size comparison from ad60c95 to 2edb16b Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
…e related python tests
Small style fix
|
PR #41552: Size comparison from ad60c95 to b486e2c Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
jamesharrow
reviewed
Oct 24, 2025
src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp
Outdated
Show resolved
Hide resolved
jamesharrow
reviewed
Oct 24, 2025
src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp
Outdated
Show resolved
Hide resolved
|
PR #41552: Size comparison from ad60c95 to 181cd4c Full report (22 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #41552: Size comparison from e156205 to 839804b Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
jamesharrow
approved these changes
Oct 27, 2025
|
PR #41552: Size comparison from e156205 to 3cd66e4 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
jamesharrow
reviewed
Oct 28, 2025
|
PR #41552: Size comparison from e156205 to b1fda7a Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Using MockClock to provide the test time environment in the example
Related issues
MockClock:
Other:
Testing
Ran the example app locally during development and debugging
Fixes: #41552