-
Notifications
You must be signed in to change notification settings - Fork 48
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
Feature/K08 Get Composite Schedule #745
Feature/K08 Get Composite Schedule #745
Conversation
FYI, I have not been able to figure out how to fix the Codacy markdown linting issues. @shankari could you please review. |
FYI, I brought up the Codacy markdown linting issues in today's EVerest Cloud Communication Working Group meeting, and they indicated that they would file an issue on this. |
@folkengine note that my name has an 'i' at the end (I am not sure what 'shankar' made of your unexpected tag 😄). Although I prefer having the task in the appropriate column rather than a tag anyway since tags can get lost in the flow of email. |
Oh no, I'm so sorry. You'd think I'd have mastered copy and paste by now. |
Quick update on progress: I am currently at 54/70 files, although some of the files left are the more complex ones. I anticipate finishing this up tomorrow (Monday, Aug 19) |
Thank you @shankari |
9640e7a
to
958d3cc
Compare
Rebased with latest code from main. |
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.
Some additional comments apart from the ones in line:
- Handling for multiple ChargingSchedules in a ChargingProfile is missing
- In the
SmartChargingCtrlr.json
you can add the value"A,W"
forChargingScheduleChargingRateUnit
- As you mentioned in the PR description the algorithm used for the composite schedule calculation is heavily based on the one in v16. There are a lot of code duplications so I would like to discuss the options to refactor this (maybe as part of a separate PR) to keep this DRY. Let's do this in the WG meeting and/or in a separate meeting
OCTT test case results:
TC_K_39_CS Get Composite Schedule - No ChargingProfile installed on Charging Station: ✔️
TC_K_40_CS Get Composite Schedule - Stacking ChargingProfiles: ✔️
TC_K_41_CS [Beta] Get Composite Schedule - Combining chargingProfilePurposes: 🚫
TC_K_42_CS Get Composite Schedule - chargingRateUnit not supported: ✔️
TC_K_47_CS Get Composite Schedule - Unknown EVSEId: ✔️
I can share the logs for the failed test case which I did not yet investigate further.
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 good set of changes and I like how the approach is similar to 1.6 giving the opportunity in the future to have common scheduling processing and just the to/from JSON being different between the two. With OCPP 2.1 coming this is good to see and a great starting point.
Some general points:
- not all files have the correct license header
- some files are missing a blank line at the end
- I think there is a possibility of making profile code common perhaps with inheritance to cover the multiple schedules in OCPP 2.0.1 (perhaps a separate PR)
- some unit test only code is included in the main library code. I recommend separating that code out and keeping it with the test code. Having dead code in a library file is not recommended. (an alternative would be to use conditional compilation, #ifdef UNIT_TEST but I think it is cleaner to move it)
when I run the unit tests I'm getting a segfault in ChargePointFixture.K01FR02_CallbacksValidityChecksIfOptionalVariableChangedCallbackIsNotSetOrNotNull
[2024-08-23 10:59:31.266123] [0x00007a3c379a7740] [info] Disconnecting websocket...
[2024-08-23 10:59:31.266127] [0x00007a3c379a7740] [info] Closing websocket:
[2024-08-23 10:59:31.266138] [0x00007a3c379a7740] [info] stop()
[2024-08-23 10:59:31.266167] [0x00007a3c04a006c0] [info] There are 1 messages in the normal message queue.
[2024-08-23 10:59:31.266182] [0x00007a3c04a006c0] [info] There are 124 messages in the transaction message queue.
[2024-08-23 10:59:31.266188] [0x00007a3c04a006c0] [info] Message queue stopped processing messages
[2024-08-23 10:59:31.266224] [0x00007a3c379a7740] [info] stop() notified message queue
[2024-08-23 10:59:31.266282] [0x00007a3c02c006c0] [info] Closed websocket of NetworkConfigurationPriority: 1 which is configurationSlot 1
[2024-08-23 10:59:31.266307] [0x00007a3c054006c0] [info] Triggered authorization cache cleanup
[2024-08-23 10:59:31.266373] [0x00007a3c02c006c0] [info] pause()
[2024-08-23 10:59:31.266383] [0x00007a3c02c006c0] [info] pause() notified message queue
Segmentation fault (core dumped)
looks to be related to ocpp::common::DatabaseConnection::close_connection_internal
Core was generated by `./libocpp_unit_tests'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007a3c380a8354 in sqlite3VdbeMemSetNull () from /lib/x86_64-linux-gnu/libsqlite3.so.0
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.
I want to start by appreciating the detailed documentation and sequence diagram in this PR. It really helped a lot with the review.
My top level comment is that this doesn't seem to support tariffs, either in the data structure (https://github.com/EVerest/libocpp/pull/745/files#diff-8d323d412965fe8a0e36a81391917726636742ca099cad43402ed5ef9171bb85R25) or in the examples (https://github.com/EVerest/libocpp/pull/745/files#diff-6db7040d530e9d5fecf07cd97aef7c865c9529a781150ed2576dc111c26d5d2aR9 and similar). I understand that the tariffs are optional, but they are part of the spec, and in all conversations I have had, they are expected to be a more popular mode of control than hard limits. I am fine with deferring this to a subsequent PR, but I would like to see an issue filed to track this gap before we merge this PR, so that we don't forget to make the changes.
Note also that I did not carefully validate the expected outputs of the tests. For example, the numbers (2000, 1, 1, 0) seem resonable but I have not done the math to verify them. I assume that the community review will check this with a fine-tooth comb.
ASSERT_EQ(period_01.limit, 2000);
ASSERT_EQ(period_01.numberPhases, 1);
ASSERT_EQ(period_01.stackLevel, 1);
ASSERT_EQ(period_01.startPeriod, 0);
Detailed comments follow...
@shankari & @james-ctc I will have to respectively disagree with your comments concerning operators against Domain entities that are only used in unit tests being placed under the testing directory. I come from the school that sees these elements as part of the general contract for the entities. Since this is a library, and we want to support using this library for things other than itself, such as Appenzell, these would be considered a standard part of what Joshua Block calls "their general contracts". -- Joshua Bloch, Chapter 3 Effective Java, 3rd Edition Please forgive me for citing a Java book, but it is one of my favorites, and had a massive impact on my craft as a developer. Nevertheless, in the spirit of Git-R-Done I am going to move them and table this discussion until a later date. Thanks for the feedback. |
My programming background is firmware in safety critical systems where different approaches are common. MISRA compliance is often a requirement. There are a number of things to consider.
Using the out of class definition operator== simplifies the unit tests. |
Very informative. That makes total sense, and completely changes my mind. Thanks! |
7c35477
to
9a04113
Compare
@Pietfried we have created an issue for TC_K_41_CS on our board to isolate if this is an issue with the test or with this code. Thank you for the information. |
@james-ctc is it possible for you to Details of the system you are using to run the tests, and anything else that you might think would be helpful to isolate the issue would be most welcome. |
There isn't an issues tab available to me more details on the core dump (which occurs in different places)
I did a clean rebuild and got a segfault in test
|
Me neither 🤦 @james-ctc thanks, this is very helpful. I've added an issue on our board. |
@james-ctc is it possible for you to run the tests against the main EVerest/libocpp? I'm curious if you are getting the same error against K01FR02_CallbacksValidityChecksIfOptionalVariableChangedCallbackIsNotSetOrNotNull there. Thanks! |
core dump with test
|
… per PR comments Signed-off-by: Christoph <[email protected]>
… per PR comments Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
…test as per PR review Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
…er PR review. Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
…per PR review Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
c985265
to
94316cf
Compare
@Pietfried resolved |
Signed-off-by: Christoph <[email protected]>
94316cf
to
da7b871
Compare
@Pietfried This build is passing on our REPO |
* * Added OCPP 2.0.1 Calculate Composite Schedule functionality and tests * Added Generic optional type equality function for testing option types * Create v201/profile.hpp to match file structure used in 1.6 CompositeSchedule work * Added to_string functions to v201/utils to facilitate easier testing, logging and debugging * Added OCPP 1.6 test_composite_schedule.cpp suite. * Added ability to use serialized JSON Profiles scenarios for testing in v1.6 and 2.0.1. * Added JSON Profiles based on specific testing scenarios for v1.6 and 2.0.1. * Enabled now working 1.6 Composit Schedule tests * Added v201/test_profile.cpp suite mirroring but refactoring tests done for v1.6 version * Removed excessive logging Signed-off-by: Christoph <[email protected]> * Added sorting of vectors in SmartChargingTestUtils::get_charging_profiles_from_directory() to fix order issue in tests on CI Signed-off-by: Christoph <[email protected]> * Updated READMEs to fix linting issues Signed-off-by: Christoph <[email protected]> * Updated READMEs to fix more linting issues Signed-off-by: Christoph <[email protected]> * Removed unused LimitStackLevelPair struct caught by linter Signed-off-by: Christoph <[email protected]> * Added const to functions as per linter Signed-off-by: Christoph <[email protected]> * Added pass by const reference to functions as per linter Signed-off-by: Christoph <[email protected]> * Updated status doc Signed-off-by: Christoph <[email protected]> * Add get_valid_profiles() to Smart Charging Handler. When retrieving the valid profiles for calculating composite schedules, this function returns all of the profiles for a given EVSE ID, as well as the station-wide profiles. It also ensures that the profiles are valid. The calculations for composite schedule handle the start and end time, so this function does not account for it at this stage. Signed-off-by: Christopher Davis <[email protected]> Signed-off-by: Gianfranco Berardi <[email protected]> * added calculation for charging station external constraints. added tests to cover external constraints. refactored calculation for both max and external to a method. Signed-off-by: Coury Richards <[email protected]> * smart_charging: Make more functions mockable Make `calculate_composite_schedule()` and `get_valid_profiles()` part of the `SmartChargingHandlerInterface` so they may be overriden for mocking. Signed-off-by: Christopher Davis <[email protected]> * smart_charging: Take chargingRateUnit as optional in `calculate_composite_schedule()` The internal functions (and the v1.6 impl) take this as optional, but we were requiring a value. Signed-off-by: Christopher Davis <[email protected]> * charge_point: Handle GetCompositeScheduleRequest Adds a message handler for GetCompositeScheduleRequest and associated tests within `test_charge_point.cpp`. Due to the core functionality being tested within `test_composite_schedule.cpp`, we primarily test that `get_valid_profiles()` and `calculate_composite_schedule()` are being called within the right contexts: * We calculate a composite schedule from a list of valid profiles * We do not calculate a composite schedule when the EVSE ID is unknown * We do not calculate a composite schedule when the `chargingRateUnit` sent in the request is not configured. Signed-off-by: Christopher Davis <[email protected]> * doc: Update OCPP v2.0.1 status doc We now cover: - K08.FR.03 - K08.FR.04 - K08.FR.05 - K08.FR.07 Signed-off-by: Christopher Davis <[email protected]> * Resolving PR comments from @Pietfried Signed-off-by: Christoph <[email protected]> * Moving location of json profiles as per PR comment Signed-off-by: Christoph <[email protected]> * Moving static functions only used in file into anonymous namespace as per PR comments Signed-off-by: Christoph <[email protected]> * Moving static functions only used in file into anonymous namespace as per PR comments Signed-off-by: Christoph <[email protected]> * Added documentation to test utility function as per PR review Signed-off-by: Christoph <[email protected]> * Moving equality operators to smart_charging_test_utils as per PR review Signed-off-by: Christoph <[email protected]> * Corrected mistake in Status doc as per PR review Signed-off-by: Christoph <[email protected]> * Updated constants as per PR review Signed-off-by: Christoph <[email protected]> * Updated profile comments as per PR review Signed-off-by: Christoph <[email protected]> * Moved functions only used in tests to tests as per PR review Signed-off-by: Christoph <[email protected]> * Added link to issue resolved in Case One composite schedule scenario test as per PR review Signed-off-by: Christoph <[email protected]> * Removed unused struct as per PR review Signed-off-by: Christoph <[email protected]> * Moved functions only used in tests to tests as per PR review Signed-off-by: Christoph <[email protected]> * Rolled back accidental changes to 1.6 code Signed-off-by: Christoph <[email protected]> * Remove profile scenario not used in PR Signed-off-by: Christoph <[email protected]> * Added more detailed Grid foundation test as per PR review. Signed-off-by: Christoph <[email protected]> * Fixed CalculateProfileEntryType_Param_Test as per PR review. Signed-off-by: Christoph <[email protected]> * Updated CalculateChargingSchedule_Overlap test names as per PR review. Signed-off-by: Christoph <[email protected]> * Updated CalculateChargingScheduleCombined_CombinedOverlapT names as per PR review. Signed-off-by: Christoph <[email protected]> * Moved functions only used in tests to tests as per PR review Signed-off-by: Christoph <[email protected]> * Added in grabbing transaction session start for relative profiles as per PR review Signed-off-by: Christoph <[email protected]> * Added check for evse_id == 0 as per PR review Signed-off-by: Christoph <[email protected]> * Touch to rerun GitHub Actions Signed-off-by: Christoph <[email protected]> * Touch to rerun GitHub Actions Signed-off-by: Christoph <[email protected]> * Touch to rerun GitHub Actions Signed-off-by: Christoph <[email protected]> --------- Signed-off-by: Christoph <[email protected]> Signed-off-by: Christopher Davis <[email protected]> Signed-off-by: Gianfranco Berardi <[email protected]> Signed-off-by: Coury Richards <[email protected]> Co-authored-by: Gianfranco Berardi <[email protected]> Co-authored-by: Coury Richards <[email protected]> Co-authored-by: Christopher Davis <[email protected]> Signed-off-by: Soumya Subramanya <[email protected]>
Describe your changes
This PR contains the complete support for the K08 requirement within the OCPP 2.0.1 standard. In includes the follow work:
Composite Schedule
algorithm from v1.6 to v2.0.1.Charging Station External Constraints Profiles
, which are new in 2.0.1.GetCompositeScheduleRequest
.Core
Composite Schedule
AlgorithmThis PR is for the core algorithm for calculating an individual
Composite Schedule
from a selection of Profiles within a given time range. This code is a refactoring of the code recently updated for the 1.6 version of the standard.NOTE: As a part of this PR we have decided to keep the same names for methods to match the 1.6 version even though we felt there might be an opportunity to refactor them to clearly distinguish the different parts of the process of generating a
Composite Schedule
.Here is the flow for the code in question:
NOTE: A big change between this and the original 1.6 version of the code is the way we are testing the code in question, as well as adding existing tests that were written against the early version of the 1.6 code, that include ones that were failing before, but are now happily fixed with the earlier 1.6 update.
Here are the major refactorings in question:
Composite Schedule
.smart_charging_test_utils.hpp
that include methods to simplify the creation ofDateTime
objects used for creating test ranges.Composite Schedule
tests in thetest_composite_schedule.cpp
test suite.Message handler for
GetCompositeScheduleRequest
Adds a message handler for GetCompositeScheduleRequest and associated
tests within
test_charge_point.cpp
.Due to the core functionality being tested within
test_composite_schedule.cpp
,we primarily test that
get_valid_profiles()
andcalculate_composite_schedule()
are being called within the right contexts:
chargingRateUnit
sent in the request is not configured.
Issue ticket number and link
Checklist before requesting a review