Validate a TxDefaultProfile. #524

gberardi-pillar · 2024-03-11T19:35:49Z

These changes apply to K01.FR.52 and K01.FR.53.

Given a TxDefaultProfile on an EVSE or on the charging station as a whole, we can validate whether a new TxDefaultProfile is allowed.

gberardi-pillar · 2024-03-11T19:36:31Z

@shankari Please review these changes.

gberardi-pillar · 2024-03-13T17:08:49Z

@shankari In case you missed my previous comment, can you please review this PR?

shankari

I think that some additional documentation would be helpful
there is a static code analysis error in here

include/ocpp/v201/smart_charging.hpp

include/ocpp/v201/evse.hpp

Pietfried

It is not intended that an Evse instance is initialized with ID 0. Ids of Evses must start from 1 upwards as required by 7.1 EVSE Numbering in OCPP2.0.1 Part 1 - Architecture & Topology. This requires a redesign of the validate_tx_default_profile function signature, where I think the Evse argument could simply be replaced with int32_t evse_id.

The Evse class does there not require a member function is_station_wide.

Additionally, a more general note: Im not sure if the member std::vector<EvseProfile> charging_profiles of the SmartChargingHandler is complex enough to represent more complex combinations of profiles. The OCPP1.6 implementation makes use of two different std::map per EVSE (in 1.6 Evse and Connector is equivilant):
https://github.com/EVerest/libocpp/blob/main/include/ocpp/v16/connector.hpp#L20

Pietfried

lgtm, except for some minor function doc updates and the argument for validate_tx_default_profile.
Please do not squash and force push after a review, so that the changes made afterwards can be reviewed easier. We will squash the commits when we merge into main

Pietfried · 2024-03-27T16:58:48Z

include/ocpp/v201/smart_charging.hpp

+    /// \brief validates the given \p profile according to the specification
+    ///
+    ProfileValidationResultEnum validate_tx_default_profile(const ChargingProfile& profile,
+                                                            std::optional<Evse*> evse_opt) const;


Couldnt we use a const int32_t evse_id as an argumen instead of std::optional<Evse*> evse_opt. Would be more simple and easier to read imo.

Please also update the function docs according to the added param

We've been treating the existence of an Evse as a specific instance and the lack of one as indicating ID = 0. I did this work to follow the pattern we have established elsewhere.

But I can use an ID and check if it is 0 instead.

Pietfried · 2024-03-27T16:59:13Z

include/ocpp/v201/smart_charging.hpp

    /// \brief Adds a given \p profile to our stored list of profiles
-    void add_profile(const ChargingProfile& profile);
+    ///
+    void add_profile(int32_t evse_id, ChargingProfile& profile);


Please update the function docs according to the added param

Pietfried

👍
This needs a rebase since
#534 has been merged

marcemmers

Sorry I am a bit late to the party here but I do have some suggestions/questions.

marcemmers · 2024-03-28T11:03:09Z

include/ocpp/v201/smart_charging.hpp

@@ -38,7 +44,8 @@ class SmartChargingHandler {

    std::shared_ptr<ocpp::v201::DatabaseHandler> database_handler;
    // cppcheck-suppress unusedStructMember
-    std::vector<ChargingProfile> charging_profiles;
+    std::vector<EvseProfile> charging_profiles;


Would an std::map with the evse_id and a vector of charging profiles make sense here? That way we don't need the specific type for EvseProfile and we can search a bit quicker too.

I have changed it into a map.

Should the second type not be a vector since you can now only store one profile per EVSE? Or are you storing them by ChargingProfile id now?

Ah, yes, you're correct.

lib/ocpp/v201/smart_charging.cpp

These changes apply to K01FR52 and K01FR53. Signed-off-by: Christoph <367712+folkengine@users.noreply.github.com> Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

gberardi-pillar · 2024-03-28T14:58:15Z

Put PR back into draft due to still waiting for @shankari to re-review.

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

shankari

@gberardi-pillar the changes look fine to me. However, I would suggest that, in the future, you have more detailed commit messages than "Review fix". A short description of what part of the review is being addressed by the commit would be helpful, along with a summary of the changes. You could even add a link to the commit to make it easier for future maintainers to easily find the review without having to search through all the comments in the PR

gberardi-pillar · 2024-04-10T12:40:09Z

@Pietfried @marcemmers Is this PR ready to be merged?

marcemmers · 2024-04-10T12:53:30Z

@Pietfried @marcemmers Is this PR ready to be merged?

Yes, go for it 👍
Might be good to do a rebase before merging. Don't expect any issues there.

gberardi-pillar · 2024-04-10T12:56:35Z

@Pietfried @marcemmers Is this PR ready to be merged?

Yes, go for it 👍 Might be good to do a rebase before merging. Don't expect any issues there.

Ah, but I don't have permission to merge the PR. B-)

* Validate a TxDefaultProfile. These changes apply to K01FR52 and K01FR53. Signed-off-by: Christoph <367712+folkengine@users.noreply.github.com> Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com> * Review fixes. Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com> * Review fix. Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com> * Add test case. Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com> * Review fix. Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com> * Review fix. Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com> --------- Signed-off-by: Christoph <367712+folkengine@users.noreply.github.com> Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

drmrd · 2024-05-02T18:02:11Z

Contributes to #361

gberardi-pillar requested review from Pietfried, hikinggrass, marcemmers and maaikez as code owners March 11, 2024 19:35

gberardi-pillar force-pushed the gberardi/v201-validate-txdefaultprofile branch from 9313586 to 6327074 Compare March 11, 2024 19:44

shankari suggested changes Mar 15, 2024

View reviewed changes

include/ocpp/v201/smart_charging.hpp Outdated Show resolved Hide resolved

include/ocpp/v201/evse.hpp Outdated Show resolved Hide resolved

gberardi-pillar force-pushed the gberardi/v201-validate-txdefaultprofile branch 3 times, most recently from 67e95ae to 537fab6 Compare March 18, 2024 16:31

gberardi-pillar requested a review from shankari March 20, 2024 12:31

Pietfried requested changes Mar 21, 2024

View reviewed changes

gberardi-pillar force-pushed the gberardi/v201-validate-txdefaultprofile branch from 537fab6 to 8b67c08 Compare March 22, 2024 20:13

Pietfried assigned Pietfried and marcemmers Mar 25, 2024

gberardi-pillar force-pushed the gberardi/v201-validate-txdefaultprofile branch 2 times, most recently from fa19bc3 to 8b04483 Compare March 26, 2024 14:32

gberardi-pillar requested a review from Pietfried March 26, 2024 17:35

Pietfried requested changes Mar 27, 2024

View reviewed changes

Pietfried approved these changes Mar 28, 2024

View reviewed changes

marcemmers requested changes Mar 28, 2024

View reviewed changes

gberardi-pillar added 3 commits March 28, 2024 06:40

Validate a TxDefaultProfile.

24b5b44

These changes apply to K01FR52 and K01FR53. Signed-off-by: Christoph <367712+folkengine@users.noreply.github.com> Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

Review fixes.

085331e

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

Review fix.

962c43a

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

gberardi-pillar force-pushed the gberardi/v201-validate-txdefaultprofile branch from 3dac9b0 to 962c43a Compare March 28, 2024 12:04

gberardi-pillar marked this pull request as draft March 28, 2024 14:57

gberardi-pillar added 2 commits March 28, 2024 10:37

Add test case.

24abcc8

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

Review fix.

bfcd2e9

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

Review fix.

99ef95e

Signed-off-by: Gianfranco Berardi <54074967+gberardi-pillar@users.noreply.github.com>

shankari approved these changes Apr 9, 2024

View reviewed changes

gberardi-pillar marked this pull request as ready for review April 9, 2024 12:59

gberardi-pillar requested a review from marcemmers April 9, 2024 12:59

marcemmers approved these changes Apr 9, 2024

View reviewed changes

marcemmers merged commit d47fd2d into EVerest:main Apr 10, 2024
3 of 4 checks passed

This was referenced May 2, 2024

OCPP 2.0.1 Client Smart Charging Functional Block Implementation #308

Open

Use Case K01: SetChargingProfile #361

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate a TxDefaultProfile. #524

Validate a TxDefaultProfile. #524

gberardi-pillar commented Mar 11, 2024

gberardi-pillar commented Mar 11, 2024

gberardi-pillar commented Mar 13, 2024

shankari left a comment

Pietfried left a comment •

edited

Pietfried left a comment •

edited

Pietfried Mar 27, 2024

Pietfried Mar 27, 2024

gberardi-pillar Mar 27, 2024

Pietfried Mar 27, 2024

gberardi-pillar Mar 27, 2024

Pietfried left a comment

marcemmers left a comment

marcemmers Mar 28, 2024

gberardi-pillar Mar 28, 2024

marcemmers Mar 28, 2024

gberardi-pillar Mar 29, 2024

gberardi-pillar commented Mar 28, 2024

shankari left a comment

gberardi-pillar commented Apr 10, 2024

marcemmers commented Apr 10, 2024

gberardi-pillar commented Apr 10, 2024

drmrd commented May 2, 2024

Validate a TxDefaultProfile. #524

Validate a TxDefaultProfile. #524

Conversation

gberardi-pillar commented Mar 11, 2024

gberardi-pillar commented Mar 11, 2024

gberardi-pillar commented Mar 13, 2024

shankari left a comment

Choose a reason for hiding this comment

Pietfried left a comment • edited

Choose a reason for hiding this comment

Pietfried left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pietfried left a comment

Choose a reason for hiding this comment

marcemmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gberardi-pillar commented Mar 28, 2024

shankari left a comment

Choose a reason for hiding this comment

gberardi-pillar commented Apr 10, 2024

marcemmers commented Apr 10, 2024

gberardi-pillar commented Apr 10, 2024

drmrd commented May 2, 2024

Pietfried left a comment •

edited

Pietfried left a comment •

edited