Feature/327 use case h01 reservation #854

maaikez · 2024-10-31T17:03:17Z

Describe your changes

Changes to implement reservations for ocpp 2.0.1.

Issue ticket number and link

Checklist before requesting a review

I have performed a self-review of my code
I have made corresponding changes to the documentation
If OCPP 2.0.1: I have updated the OCPP 2.0.1 status document
I read the contribution documentation and made sure that my changes meet its requirements

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…ing reservations. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

marcemmers · 2024-11-04T13:21:34Z

include/ocpp/v201/charge_point.hpp

+    /// \param connector_type   The connector type.
+    /// \return std::nullopt if connector type is given, but does not exist. Otherwise the connector status.
+    ///
+    std::optional<ConnectorStatusEnum> get_available_connector_or_status(const uint32_t evse_id,


Purely looking at this function from the header it is a bit confusing. I think you are just checking if an evse has a connector available and optionally only check for a specific connector type?

If so, I would rename and change the documentation a bit. The connector_type argument is then a sort of optional filter.

Ok I changed it a bit, can you take a look if that's better (have to commit still).

I am not sure about the function name as it will just return 'Available' if possible and otherwise the status of one of the connectors. Which is hard to catch in a function name ;).

Moved it to the evse class by the way, as your other comment suggested.

This also looks a little bit confusing to me. Looking at how this function is used internally, it looks like bool is_connector_available(const int32_t evse_id, std::optional<ConnectorEnum> connector_type) provides everything you need. This would search for an available connector at the given evse_id using the given connector_type filter. Apart from that it looks like we need a function bool does_connector_exist(const int32_t evse_id, const ConnectorEnum connector_type) to check if the given connector_type belongs to any evse. I will mark the places in charge_point.cpp where I think these function shall be used. It looks like you never need the actual ConnectorStatus so a bool should be sufficient

Ok, can you (@marcemmers , @Pietfried ) take a look if this is what you mean and if it is more readable now?

Maybe get_connector_status can be a bit more explicit when it does not find an available connector. Instead of returning the state of one of the connectors I would return the first or the last connectorId or at least a defined connector status.

Other than that it looks fine for me.

It does return the status of the last found connector with the given connector type. But if no connector with that type is found, it returns a nullopt. Should that be changed? Because then there is a status returned of a connector that does not even exist?

I changed the function a bit by the way, if the connector type is unknown, it is also included in the search.

Ah sorry, I meant this part If there is no 'Available' connector, it will return the status of one of the connectors. which makes it seem like it just picks a connector to return the status for.

Ok changed to If there is no 'Available' connector, it will return the status of the last found connector with the given connector type.

marcemmers · 2024-11-04T13:23:24Z

include/ocpp/v201/charge_point_callbacks.hpp

@@ -151,5 +151,14 @@ struct Callbacks {
    std::optional<std::function<void(const RunningCost& running_cost, const uint32_t number_of_decimals,
                                     std::optional<std::string> currency_code)>>
        set_running_cost_callback;
+
+    /// \brief Callback function is called when a reservation request is received from the CSMS
+    std::optional<std::function<ReserveNowStatusEnum(


Any reason why you don't just forward the request like we do with the other callbacks? Saves a bunch of arguments ;)

Hmm I kept it quite the same as in OCPP 1.6. I don't know why the choice there was to do it this way. Better change it then?

I would prefer to keep the interface for the 2.0.1 implementation consistent over certain functionality shared between 1.6 and 2.0.1 so yeah I would change it. Just my 2 cents ofcourse.

Well it is consistent with 1.6.

@Pietfried do you have a preference?

Talked to Piet, I will change it to the Reservation type (give me some time :)).

marcemmers · 2024-11-04T13:26:06Z

lib/ocpp/v201/charge_point.cpp

+        return std::nullopt;
+    }
+
+    bool type_found = false;


I would move the implementation of this to the Evse class, minus the evse_id argument ofcourse.

marcemmers · 2024-11-04T13:27:33Z

lib/ocpp/v201/charge_point.cpp

+             .value_or(false)) {
+        // Reservation not available / implemented, return 'Rejected'.
+        // H01.FR.01
+        EVLOG_info << "Receiving a reservation request, but reservation is not implemented.";


if ReservationCtrlrEnabled is false it might be implemented, just not enabled.

Also, for most of these it would be nice if we can add the error to the call result as well for the CSMS to see what is actually happening.

marcemmers · 2024-11-04T13:28:48Z

lib/ocpp/v201/charge_point.cpp

+             .value_or(false)) {
+        // H01.FR.19
+        EVLOG_warning
+            << "Trying to make a reservation, but no evse id was given while it should be sent in the request.";


when ReservationCtrlrNonEvseSpecific is disabled

marcemmers · 2024-11-04T13:30:57Z

lib/ocpp/v201/charge_point.cpp

+    ConnectorStatusEnum connector_status = ConnectorStatusEnum::Unavailable;
+
+    if (evse_id.has_value()) {
+        if (evse_id.value() < 0 || !evse_manager->does_evse_exist(evse_id.value())) {


I see I missed some checks in the evse manager. The smaller than 0 check should definitely be there (also in the get_evse functions. Then evse_id.value() < 0 would not be necessary anymore

marcemmers · 2024-11-04T13:34:41Z

lib/ocpp/v201/charge_point.cpp

+
+    const std::optional<int32_t> evse_id = request.evseId;
+
+    ConnectorStatusEnum connector_status = ConnectorStatusEnum::Unavailable;


You don't need to store this since you never use it anywhere

It is just shorter and better readable in my opinion.

I'm not sure I follow. You set the connector_status value here and in other places, but you never read it except for in line 3407. Where you always set the value the line before so this variable could be limited to that if statement.

Ah thought it was about evse_id.

You're right.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

marcemmers · 2024-11-11T13:24:26Z

include/ocpp/v201/charge_point.hpp

+    // Function Block H: Reservations
+    void handle_reserve_now_request(Call<ReserveNowRequest> call);
+    void handle_cancel_reservation_callback(Call<CancelReservationRequest> call);
+    void send_reserve_now_rejected_response(const MessageId& unique_id, const std::string& status_info);


I see we haven't used this anywhere yet in ChargePoint but the status_info could be a string_view. You can change it if you want but it's fine either way.

Pietfried · 2024-11-11T16:39:09Z

lib/ocpp/v201/charge_point.cpp

+        }
+
+        // Check if there is a connector available for this evse id.
+        std::optional<ConnectorStatusEnum> status =


use does_connector_exist here.

Pietfried · 2024-11-11T16:39:27Z

lib/ocpp/v201/charge_point.cpp

+
+        bool status_found = false;
+        for (uint64_t i = 1; i <= number_of_evses; i++) {
+            std::optional<ConnectorStatusEnum> status = get_connector_status(i, request.connectorType);


use is_connector_available here

maaikez · 2024-11-13T12:01:38Z

lib/ocpp/v201/evse_manager.cpp

        throw EvseOutOfRangeException(id);
    }
    return *this->evses.at(id - 1);
 }

-bool EvseManager::does_evse_exist(int32_t id) const {


The name of this function is a bit strange. In principle, evse id 0 does not exist so I added that check. But composite schedule relies on this that evse id 0 does exist. So maybe we should change the name or add an extra parameter here? @Pietfried ?

The fact that with smart charging profiles they (mis)use evse 0 should be covered in that part of the code I think. Now you can ask the EvseManager if evse 0 exists and it will say yes. Then please give me evse 0 and it will throw 🤔

Exactly, that's strange isn't it?
So it's not for this pull request but shall I make a ticket maybe?

Well you could change this to be id > 0 && etc and then fix the issues in smart charging as well, should be a small fix. Or indeed in a different ticket if you prefer that.

Yes I created a bug report, as the function is also called from another function and I think to be sure, we have to test smart charging if everything is still fine.

#868

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…return the connector status. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Pietfried · 2024-11-22T13:55:09Z

lib/ocpp/v201/evse.cpp

+        return std::nullopt;
+    }
+
+    return conversions::string_to_connector_enum(connector_type.value());


Suggested change

return conversions::string_to_connector_enum(connector_type.value());

try {

return conversions::string_to_connector_enum(connector_type.value());

}

catch (const StringToEnumException& e) {

EVLOG_warning << "Could not convert to ConnectorEnum: " << connector_type.value();

return std::nullopt;

}

…n (needed for remote start). Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Pietfried · 2024-11-25T08:10:59Z

@maaikez we could also tick the Reservation functional block in the README.md as part of this PR :)

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez added 9 commits September 16, 2024 18:42

Start reservation implementation.

dfb964b

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Some changes to the reserve now callback.

92c75ac

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add todo's

218a1d5

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Merge branch 'main' into feature/327-use-case-h01-reservation

0f97209

Add todo.

d47b009

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Merge branch 'main' into feature/327-use-case-h01-reservation

c6b7242

Add reservation callbacks and handlers and fix some bugs.

d310f02

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Remove unnecessary todo's. Remove unnecessary loop to check for exist…

706f54c

…ing reservations. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Merge branch 'main' into feature/327-use-case-h01-reservation

dd6e6bb

maaikez linked an issue Oct 31, 2024 that may be closed by this pull request

Use Case H01: Reservation #327

Closed

maaikez added 2 commits November 4, 2024 13:10

Fix conversion of bool for connector zero support.

f454ce7

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add reservation status.

0eb8847

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez marked this pull request as ready for review November 4, 2024 12:54

maaikez requested review from Pietfried, hikinggrass and marcemmers as code owners November 4, 2024 12:54

marcemmers reviewed Nov 4, 2024

View reviewed changes

Review comments.

3823b1c

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/327-use-case-h01-reservation branch from 2828d56 to 3823b1c Compare November 5, 2024 17:26

Remove unused variable.

27bcebb

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

marcemmers self-assigned this Nov 6, 2024

maaikez force-pushed the feature/327-use-case-h01-reservation branch 2 times, most recently from 4259dc0 to dbdf07a Compare November 8, 2024 17:28

Review comment: change parameters of reserve now callback.

045bdaf

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/327-use-case-h01-reservation branch from dbdf07a to 045bdaf Compare November 8, 2024 17:44

marcemmers approved these changes Nov 11, 2024

View reviewed changes

Pietfried reviewed Nov 11, 2024

View reviewed changes

Merge branch 'main' into feature/327-use-case-h01-reservation

04b2c15

maaikez commented Nov 13, 2024

View reviewed changes

maaikez added 9 commits November 13, 2024 13:04

Fix bug for composite schedule

ae1d21a

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Review comments. Some changes in functions to make it more readable.

d7c7792

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add new method to mock and change mock method function.

c019173

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Bump version

b6e8bb9

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

get_connector_status: When connector type is unknown, it should also …

4a93e69

…return the connector status. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Merge branch 'main' into feature/327-use-case-h01-reservation

17fd4b4

Change comment.

8568af1

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Merge branch 'main' into feature/327-use-case-h01-reservation

8145980

Fix build after merge.

6c3a3ec

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Pietfried approved these changes Nov 22, 2024

View reviewed changes

Some changes to check if there is a reservation for an evse / id toke…

7840131

…n (needed for remote start). Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/327-use-case-h01-reservation branch from c70ecec to 7840131 Compare November 22, 2024 15:32

maaikez added 6 commits November 25, 2024 12:03

Review comments.

d9b3372

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Merge branch 'main' into feature/327-use-case-h01-reservation

41a8ee7

Fix build after merge.

0bc3c95

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Review comments

c682827

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Formatting

a1eb987

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Bump version

8e9a5d7

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez merged commit 173af8d into main Nov 26, 2024
8 checks passed

maaikez deleted the feature/327-use-case-h01-reservation branch November 26, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/327 use case h01 reservation #854

Feature/327 use case h01 reservation #854

maaikez commented Oct 31, 2024 •

edited

Loading

marcemmers Nov 4, 2024

maaikez Nov 5, 2024 •

edited

Loading

Pietfried Nov 11, 2024 •

edited

Loading

maaikez Nov 13, 2024

marcemmers Nov 14, 2024 •

edited

Loading

maaikez Nov 14, 2024

marcemmers Nov 14, 2024 •

edited

Loading

maaikez Nov 15, 2024

marcemmers Nov 4, 2024

maaikez Nov 5, 2024

marcemmers Nov 5, 2024

maaikez Nov 5, 2024

maaikez Nov 7, 2024

maaikez Nov 8, 2024

marcemmers Nov 4, 2024

marcemmers Nov 4, 2024

marcemmers Nov 4, 2024

marcemmers Nov 4, 2024

marcemmers Nov 4, 2024

maaikez Nov 5, 2024

marcemmers Nov 5, 2024

maaikez Nov 6, 2024

marcemmers Nov 11, 2024

Pietfried Nov 11, 2024

Pietfried Nov 11, 2024

maaikez Nov 13, 2024

marcemmers Nov 14, 2024

maaikez Nov 14, 2024

marcemmers Nov 14, 2024

maaikez Nov 15, 2024

Pietfried Nov 22, 2024

Pietfried commented Nov 25, 2024


		const std::optional<int32_t> evse_id = request.evseId;

		ConnectorStatusEnum connector_status = ConnectorStatusEnum::Unavailable;

Feature/327 use case h01 reservation #854

Feature/327 use case h01 reservation #854

Conversation

maaikez commented Oct 31, 2024 • edited Loading

Describe your changes

Issue ticket number and link

Checklist before requesting a review

Choose a reason for hiding this comment

maaikez Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Pietfried Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcemmers Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcemmers Nov 14, 2024 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Pietfried commented Nov 25, 2024

maaikez commented Oct 31, 2024 •

edited

Loading

maaikez Nov 5, 2024 •

edited

Loading

Pietfried Nov 11, 2024 •

edited

Loading

marcemmers Nov 14, 2024 •

edited

Loading

marcemmers Nov 14, 2024 •

edited

Loading