Commodity_tariff_example_MultipleIssues_fix #40648
Open
+3,650
−1,259
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
…er.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
PR #40648: Size comparison from 07b68c9 to 36e810a Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40648 +/- ##
==========================================
- Coverage 50.78% 50.62% -0.17%
==========================================
Files 1358 1360 +2
Lines 99477 100031 +554
Branches 12877 13038 +161
==========================================
+ Hits 50521 50640 +119
- Misses 48956 49391 +435 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bzbarsky-apple
previously requested changes
Aug 19, 2025
examples/all-clusters-app/realtek/bee/main/include/CHIPDeviceManager.h
Outdated
Show resolved
Hide resolved
|
PR #40648: Size comparison from 6e50acf to 6d83d5b Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #40648: Size comparison from 6e50acf to ea751c7 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
/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 comprehensive example for the Commodity Tariff cluster within the energy gateway app. The changes include new source files for the cluster's instance, delegate, main logic, event triggers, and sample data. A significant part of this PR is a well-executed refactoring of the underlying commodity tariff server implementation, which greatly improves its modularity, thread-safety, and overall design. My review identified a few issues to address: a potential bug related to a shadowed member variable in a derived class, a minor copy-paste error in a log message, and an incorrect code comment.
| void TariffTimeTrackingSetOffset(uint32_t offset); | ||
|
|
||
| private: | ||
| CommodityTariffDelegate * mDelegate; |
There was a problem hiding this comment.
The mDelegate member is re-declared here, which shadows the member in the base CommodityTariff::Instance class. This is redundant and can lead to bugs, as the base class already stores the delegate. Please remove this declaration and the corresponding assignment in the constructor on line 45. The GetDelegate() method on line 57 will also need to be updated to return the delegate from the base class, likely with a static_cast.
| SetTestEventTrigger_TimeShift4h(); | ||
| break; | ||
| case CommodityTariffTrigger::kTimeShiftDisable: | ||
| ChipLogProgress(Support, "[CommodityTariff-Test-Event] => Forced DayEntry Forward"); |
There was a problem hiding this comment.
The log message for kTimeShiftDisable appears to be a copy-paste from the kTimeShift4h case, which could be misleading during debugging. It should be updated to reflect that the time shift is being disabled.
ChipLogProgress(Support, "[CommodityTariff-Test-Event] => Time Shift Disabled");| { | ||
| assert(!UpdCtx.CalendarPeriodsDayPatternIDs.empty()); // Something went wrong if CP has no DP IDs | ||
|
|
||
| // Checks that all ID_DE_IDs are in main DE list: |
There was a problem hiding this comment.
This comment seems to be a copy-paste error from the IndividualDays validation logic. The code is checking DayPatternKeyIDs, not ID_DE_IDs. The comment should be updated to accurately describe the check being performed for better code maintainability.
// Checks that all DayPatternIDs in CalendarPeriods are in the main DayPatterns list:
…example_MultipleIssues_fix
|
PR #40648: Size comparison from 71aed77 to aafc28a Full report (29 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, psoc6, qpg, realtek, stm32, telink)
|
|
PR #40648: Size comparison from 71aed77 to 48349c8 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
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.
Description
The commodity tariff cluster added to energy gateway app example.
Automated tests
N/A
Manual tests
Apps starting
CHIP-TOOL command to change attributes
The CHIP-TOOL commands to read attributes
commoditytariff read <ATTR> 0x12344321 1Where ATTR can have one of the following values:
The CHIP-TOOL commands to cluster's commands sending
Testing
Commodity Tariff server is integrated into the energy-gateway app. Follow these steps to validate functionality:
1. Initial Setup
2. Default Attribute Validation
Nullbefore any triggers.tariff-info):./chip-tool commoditytariff read tariff-info 0x12344321 1Null, ensure the server initializes correctly and no stale data exists.3. Trigger Tariff Data Load (Event 0)
Read dynamic attributes (e.g.,
current-day,next-day-entry) to confirm they are now populated:./chip-tool commoditytariff read current-day 0x12344321 1Nullvalues matching the test dataset.4. Test Time Shifts (Events 2 & 3)
Check time-sensitive attributes (e.g.,
current-day-entry-date) for expected shifts:./chip-tool commoditytariff read current-day-entry-date 0x12344321 15. Disable Time Shift (Event 4)
6. Cluster Commands
<ID>with a valid index):<ID>with a valid index):7. Cleanup (Event 1)
Null.