Reservation changes for 2.0.1 #943

maaikez · 2024-10-31T17:01:36Z

Describe your changes

Implement reservation changes for ocpp 2.0.1 (connector type, reservation for not specific evse)

OCTT tests:

OCPP 2.0.1

Passed:
TC_H_01_CS
TC_H_02_CS
TC_H_03_CS
TC_H_04_CS
TC_H_06_CS
TC_H_07_CS
TC_H_09_CS
TC_H_10_CS
TC_H_12_CS
TC_H_13_CS
TC_H_16_CS
TC_H_17_CS
TC_H_18_CS
TC_H_19_CS
TC_H_22_CS
TC_H_23_CS

Failed:
TC_B_24_CS: After reset, BootNotification should have reason 'RemoteReset', but is 'PowerUp'.
TC_H_08_CS: Did not receive expected status change to 'Available'.
TC_H_14_CS: Two 'global' reservations and 2 evse's, should set them both to 'reserved'.
TC_H_15_CS: Did not receive expected status change to 'Available'. ??? -> Reservation is 'Cancelled', there is no reservation status update sent.
TC_H_21_CS: When evse is set to Unavailable and a reservation is cancelled, evse should go to 'Unavailable' directly, but it first goes to 'Available'.
TC_H_24_CS: Received unexpected request message: ReservationStatusUpdateRequest (Waiting for StatusNotificationRequest; NotifyEventRequest) ????

OCPP 1.6

Passed:
TC_046_1_CS
TC_046_2_CS
TC_047_CS
TC_048_2_CS
TC_048_3_CS
TC_048_4_CS
TC_050_2_CS
TC_050_3_CS
TC_051_CS
TC_052_CS
TC_053_CS

Failed:
TC_049_CS: Test makes a reservation for connector id 0. Expects a StatusNotification.req with 'Reserved'. Is the test wrong or should we send a StatusNotification.req with connector id 0 and status reserved????

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
I read the contribution documentation and made sure that my changes meet its requirements

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

…d added a request to check if there is a reservation for a specific token. Reservations are done on evse level, not on connector level. Further implementation of reservations. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…th tests. Some changes in connector availability. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… connector type and evse's with different connector types. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

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

…ed. If a reservation is made and there is already an existing one with the same reservation id, cancel that reservation. Add reservation timer. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

…on. Fix bug with expired reservation. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

…se state after submitting an event. Change set_evse_available to set_evse_state and the same for connector state. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…s with Auth tests. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… evse index. Fix start reserved session with parent id for 'global' reservation. Change return value of cancel reservation callback so it returns true or false if a reservation could or could not be cancelled. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…ce. Add tests for storing and loading reservations. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

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

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

Signed-off-by: Kai-Uwe Hermann <[email protected]>

types/reservation.yaml

Pietfried · 2024-11-11T16:54:48Z

modules/Auth/include/AuthHandler.hpp

     */
-    void init_connector(const int connector_id, const int evse_index);
+    void init_evse(const int evse_id, const int evse_index, const std::vector<Connector>& connectors);


It looks like you dont need the evse_index anymore in this function and also not in the Connector.hpp/cpp, which is good. So this can simply be

void init_evse(const int evse_id, const std::vector<Connector>& connectors);

Is evse index always evse id - 1?
Or can evse id also be 5 while evse index is 1?

yes you can make this assumption and add this requirement to the documentation in the index.rst with something like

The module connections of the evse_manager requirement must be connected in the correct order in the EVerest config file, i.e. the module representing the EVSE with evse id 1 must listed first, EVSE with evse id 2 second and so on.

Added this to the Auth index.rst

modules/Auth/include/AuthHandler.hpp

modules/Auth/lib/AuthHandler.cpp

modules/Auth/include/ReservationHandler.hpp

Pietfried · 2024-11-11T18:07:25Z

modules/Auth/include/ReservationHandler.hpp

+    /// \param evse_id          The EVSE id the connector belongs to.
+    /// \param connector_id     The connector id.
+    ///
+    void on_connector_state_changed(const ConnectorState connector_state, const uint32_t evse_id,


You don't need to pass the connector_state here since the ReservationManager has a reference the the evse map and can check the status internally

Well I did that because if that connector is reserved, the reservation is just cancelled and we don't have to loop through all reservations to check if they can still be done. Do you think it's more readable when I remove it?

Pietfried · 2024-11-11T18:22:07Z

modules/Auth/Auth.cpp

+            if (evse_id.has_value()) {
+                EVLOG_info << "Call reserved callback for evse id " << evse_id.value();
+
+                this->r_evse_manager.at(evse_id.value() - 1)->call_reserve(reservation_id);


The EvseManager shall stay autonoumus with the decision if a reservation can be made, so we should be able to handle the response at some point

Pietfried · 2024-11-11T18:26:18Z

modules/Auth/include/ReservationHandler.hpp

+    types::reservation::ReservationResult
+    get_evse_state(const uint32_t evse_id,
+                   const std::map<uint32_t, types::reservation::Reservation>& evse_specific_reservations);
+
+    ///
+    /// \brief Helper function to check if the connector of a specific EVSE is available.
+    /// \param evse_id          The evse id the connector belongs to.
+    /// \param connector_type   The connector type to check.
+    /// \return The `ReservationResult` to return fot his specific connector.
+    ///
+    types::reservation::ReservationResult
+    is_connector_available(const uint32_t evse_id, const types::evse_manager::ConnectorTypeEnum connector_type);


These function signatures look confusing. get_evse_state and is_connector_available return a ReservationResult. I see that you are using the responses to return the value in make_reservation, but I think something more readable is possible here

I gave the function a slightly different name, is it more clear like this?

I also thought of a helper function converting a ConnectorState to a ReservationResult, maybe that makes it more clear? What do you think?

it looks better now 👍

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

SirVer

Looks good for Bazel.

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

…king a reservation if the evse manager rejects the reservation. Make sure the evse manager can make the reservation if it is overwritten (same reservation id). Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

…in timed out (swipe of rfid but never a plugin of a connector) Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez · 2024-11-13T17:32:47Z

modules/OCPP201/manifest.yaml

@@ -98,6 +98,10 @@ requires:
    interface: display_message
    min_connections: 0
    max_connections: 1
+  reservation:
+    interface: reservation
+    min_connections: 1


@Pietfried Is it ok that this is mandatory?

I have discussed this with Cornelius and Kai , lets make it optional to allow compatability of older configs

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

Pietfried · 2024-11-14T10:20:10Z

modules/Auth/lib/ReservationHandler.cpp

+    if (evse_reservations.count(evse_id) != 0 && evse_reservations[evse_id].connector_type.has_value() &&
+        (connector_it->type == evse_reservations[evse_id].connector_type.value() ||
+         connector_it->type == types::evse_manager::ConnectorTypeEnum::Unknown ||
+         evse_reservations[evse_id].connector_type.value() == types::evse_manager::ConnectorTypeEnum::Unknown)) {
+        cancel_reservation(evse_reservations[evse_id].reservation_id, true,
+                           types::reservation::ReservationEndReason::Cancelled);
+        return;
+    }


nit: I find that quite hard to read. Id prefer something like:

bool reservation_exists = evse_reservations.count(evse_id) != 0; bool reserved_connector_specified = evse_reservations[evse_id].connector_type.has_value(); bool connector_type_matches = connector_it->type == evse_reservations[evse_id].connector_type.value(); bool connector_type_unknown = connector_it->type == types::evse_manager::ConnectorTypeEnum::Unknown || evse_reservations[evse_id].connector_type.value() == types::evse_manager::ConnectorTypeEnum::Unknown; if (reservation_exists && reserved_connector_specified && (connector_type_matches || connector_type_unknown)) { cancel_reservation(evse_reservations[evse_id].reservation_id, true, types::reservation::ReservationEndReason::Cancelled); return; }

Hmm I did this, but the order is important because of the optionals. I tried what happens if I keep those bools but it's a lot extra checks and I don't know if it will be more readable.

Pietfried · 2024-11-14T10:30:59Z

modules/Auth/include/AuthHandler.hpp

     */
-    void init_connector(const int connector_id, const int evse_index);
+    void init_evse(const int evse_id, const int evse_index, const std::vector<Connector>& connectors);


yes you can make this assumption and add this requirement to the documentation in the index.rst with something like

The module connections of the evse_manager requirement must be connected in the correct order in the EVerest config file, i.e. the module representing the EVSE with evse id 1 must listed first, EVSE with evse id 2 second and so on.

Pietfried · 2024-11-14T10:37:52Z

modules/Auth/include/ReservationHandler.hpp

+    types::reservation::ReservationResult
+    get_evse_state(const uint32_t evse_id,
+                   const std::map<uint32_t, types::reservation::Reservation>& evse_specific_reservations);
+
+    ///
+    /// \brief Helper function to check if the connector of a specific EVSE is available.
+    /// \param evse_id          The evse id the connector belongs to.
+    /// \param connector_type   The connector type to check.
+    /// \return The `ReservationResult` to return fot his specific connector.
+    ///
+    types::reservation::ReservationResult
+    is_connector_available(const uint32_t evse_id, const types::evse_manager::ConnectorTypeEnum connector_type);


it looks better now 👍

Pietfried · 2024-11-14T10:43:42Z

Failed:
TC_B_24_CS: After reset, BootNotification should have reason 'RemoteReset', but is 'PowerUp'.

That is just a SIL issue, the system module doesnt remember the reset reason.

TC_H_08_CS: Did not receive expected status change to 'Available'.

Probably unrelated to this PR

TC_H_14_CS: Two 'global' reservations and 2 evse's, should set them both to 'reserved'.

TC_H_15_CS: Did not receive expected status change to 'Available'. ??? -> Reservation is 'Cancelled', there is no reservation status update sent.

We need to investigate.

TC_H_21_CS: When evse is set to Unavailable and a reservation is cancelled, evse should go to 'Unavailable' directly, but it first goes to 'Available'.

Probably unrelated to this PR

TC_H_24_CS: Received unexpected request message: ReservationStatusUpdateRequest (Waiting for StatusNotificationRequest; NotifyEventRequest) ????

Failed:
TC_049_CS: Test makes a reservation for connector id 0. Expects a StatusNotification.req with 'Reserved'. Is the test wrong or should we send a StatusNotification.req with connector id 0 and status reserved????

Great that all OCPP1.6 PASS ! TC_049_CS can fail in my view since we can configure ReserveConnectorZeroSupported to false, right?

maaikez · 2024-11-14T14:58:18Z

Failed:
TC_B_24_CS: After reset, BootNotification should have reason 'RemoteReset', but is 'PowerUp'.

That is just a SIL issue, the system module doesnt remember the reset reason.

Yes all the identified issues were not for this pull request, only some we indeed need to investigate.

TC_H_08_CS: Did not receive expected status change to 'Available'.

Probably unrelated to this PR

I wonder if it is our bug of a bug in OCTT.

TC_H_15_CS: Did not receive expected status change to 'Available'. ???

We need to investigate.

Yep I also wonder here if it isn't an OCTT bug. Or some feature I don't understand (yet).

TC_H_21_CS: When evse is set to Unavailable and a reservation is cancelled, evse should go to 'Unavailable' directly, but it first goes to 'Available'.

Probably unrelated to this PR

Yes you'd already taken a look.

TC_H_24_CS: Received unexpected request message: ReservationStatusUpdateRequest (Waiting for StatusNotificationRequest; NotifyEventRequest) ????

We need to investigate this one as well.

Failed:
TC_049_CS: Test makes a reservation for connector id 0. Expects a StatusNotification.req with 'Reserved'. Is the test wrong or should we send a StatusNotification.req with connector id 0 and status reserved????

Great that all OCPP1.6 PASS ! TC_049_CS can fail in my view since we can configure ReserveConnectorZeroSupported to false, right?

All other tests pass except this one indeed.
Also for this one: I am not sure if this is indeed expected behaviour to have a status notification for connector 0.
We can indeed configure that to pass the test.

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

…P cancels the reservation. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

maaikez added 28 commits September 17, 2024 18:17

Start implementation of reservation.

1abe69e

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

Add todo's to support evse 0 and start making changes for that.

606af4f

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

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

dc3b0cf

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

c8f44cc

Continue working on reservations.

c40fe98

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

Add connector type when initializing connector in auth module. Fix au…

63ca3de

…th tests. Some changes in connector availability. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Try to make an algorithm for reservations with no evse but a specific…

b56eca9

… connector type and evse's with different connector types. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Reservations algorithm for multiple cars, no evse id and a given conn…

bc9b476

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

Tests now work (but I don't understand completely why yet).

3b1170f

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

The algorithm for reservations now seems to work and all tests pass.

81f407d

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

Add possibility to add reservation for specific evse id.

2abd635

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

Check if charging is possible on a specific evse id. Add more tests.

35a306f

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

Cancel reservation. Return the reason why a reservation did not succe…

84f7208

…ed. If a reservation is made and there is already an existing one with the same reservation id, cancel that reservation. Add reservation timer. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Set evse unavailable / faulted and cancel reservations.

b46b503

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

Test cancel reservation. Test cancelling because of expired reservati…

8172cd0

…on. Fix bug with expired reservation. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add some new tests.

fd13f08

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

Add some tests and fix bugs found with the tests.

bfe4ee5

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

Refactor AuthHandler so it can handle multiple connectors per evse.

f4eaf0d

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

Add documentation to ReservationHandler and reservation tests. Set ev…

3437975

…se state after submitting an event. Change set_evse_available to set_evse_state and the same for connector state. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Send reservation update when a reservation is cancelled. Fix some bug…

4e6d076

…s with Auth tests. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

7d03a7a

Load and store reservations from the kvs. Create stub for kvs interfa…

f4d10a8

…ce. Add tests for storing and loading reservations. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Fix / remove todo's. Add debug logging. Remove unnecessary logging.

315b62a

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

Change libocpp dependency to get the correct version for the reservat…

4a0ac28

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

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

c1994aa

Fix some small issues after self reviewing the code.

c2d2b65

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

maaikez changed the title ~~Feature/327 use case h01 reservation~~ Reservation changes for 2.0.1 Oct 31, 2024

Add connector type to config sil ocpp201 yaml.

843fae4

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

maaikez and others added 4 commits November 11, 2024 15:44

Formatting.

cf54aaa

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

Remove todo's (tickets are made if necessary).

78eacfb

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

Types in config must be primitive

cc6c6a9

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

Add missing bazel dependency to interfaces lib

8ab8859

Signed-off-by: Kai-Uwe Hermann <[email protected]>

hikinggrass requested review from SirVer and dorezyuk as code owners November 11, 2024 17:57

Pietfried reviewed Nov 11, 2024

View reviewed changes

Pietfried requested changes Nov 11, 2024

View reviewed changes

Pietfried self-assigned this Nov 12, 2024

maaikez added 5 commits November 12, 2024 12:12

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

3cc70d4

Review comments.

6df07ce

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

Review comments

df45d6c

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

Formatting.

42d5461

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

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

c671e80

SirVer approved these changes Nov 12, 2024

View reviewed changes

maaikez added 5 commits November 12, 2024 19:06

Fix bug after test.

925e08f

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

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

f205906

Set evse manager connector type default to unknown

9ae31b5

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

Fix bug where reservation was not removed from evse manager when plug…

53aa3a5

…in timed out (swipe of rfid but never a plugin of a connector) Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez commented Nov 13, 2024

View reviewed changes

maaikez and others added 2 commits November 13, 2024 18:34

Add reservation to config 201 pnc.

b1e1040

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

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

e6d2374

Pietfried reviewed Nov 14, 2024

View reviewed changes

maaikez added 4 commits November 14, 2024 16:17

Reservation is now optional for OCPP201 module. Review comments.

ad9d05a

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

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

3b19bd0

Reservation Update can also not been sent, which is the case when OCP…

08282f2

…P cancels the reservation. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add comment

cf690d0

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reservation changes for 2.0.1 #943

Reservation changes for 2.0.1 #943

maaikez commented Oct 31, 2024 •

edited

Loading

Pietfried Nov 11, 2024

maaikez Nov 12, 2024

Pietfried Nov 14, 2024

maaikez Nov 14, 2024

Pietfried Nov 11, 2024

maaikez Nov 12, 2024 •

edited

Loading

Pietfried Nov 11, 2024

Pietfried Nov 11, 2024

maaikez Nov 12, 2024

Pietfried Nov 14, 2024

SirVer left a comment

maaikez Nov 13, 2024 •

edited

Loading

Pietfried Nov 14, 2024

Pietfried Nov 14, 2024

maaikez Nov 14, 2024

Pietfried Nov 14, 2024

Pietfried Nov 14, 2024

Pietfried commented Nov 14, 2024

maaikez commented Nov 14, 2024

Reservation changes for 2.0.1 #943

Are you sure you want to change the base?

Reservation changes for 2.0.1 #943

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

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

maaikez Nov 12, 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

SirVer left a comment

Choose a reason for hiding this comment

maaikez Nov 13, 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

Pietfried commented Nov 14, 2024

maaikez commented Nov 14, 2024

maaikez commented Oct 31, 2024 •

edited

Loading

maaikez Nov 12, 2024 •

edited

Loading

maaikez Nov 13, 2024 •

edited

Loading