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]>

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]>

…that id to -1. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

…d_for_connector_callback for OCPP module. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Pietfried · 2024-11-25T07:22:33Z

interfaces/reservation.yaml

+        Returns true if there is a reservation for the given evse id and token or group id token. If there is an
+        evse id given, it will also return true if a reservation with this token was made for evse id 0.


This description is still based on the bool return type

Pietfried · 2024-11-25T07:27:13Z

modules/Auth/include/ReservationHandler.hpp

+    std::map<int, std::unique_ptr<module::EVSEContext>>& evses;
+    /// \brief Recursive mutex for the evse map (shared with AuthHandler).
+    std::recursive_mutex& evse_mutex;
+    // std::map<uint32_t, Evse> evses;


Suggested change

// std::map<uint32_t, Evse> evses;

Pietfried · 2024-11-25T07:31:04Z

modules/Auth/lib/Connector.cpp

+            occupied = true;
+        }
+        if (connector.get_state() != ConnectorState::UNAVAILABLE && connector.get_state() != ConnectorState::FAULTED) {
+


Suggested change

Pietfried · 2024-11-25T07:36:25Z

modules/Auth/reservation/reservationImpl.cpp

    if (reservation_result == ReservationResult::Accepted) {
-        this->mod->auth_handler->call_reserved(connector_id, reservation.reservation_id);
+        if (!this->mod->auth_handler->call_reserved(request.reservation_id, request.evse_id)) {
+            return ReservationResult::Rejected;


Do we need to cancel the reservation in this case, since it was placed before?

Well that was because you said the evse manager always has the last saying about the reservation. So if the reservation is made but the evse manager sais it is not possible, it is cancelled again.

I don't like that behaviour as well.

Yes but I can not see where the reservation is canceled if the evse manager rejects it

Pietfried · 2024-11-25T07:59:24Z

modules/Auth/CMakeLists.txt

@@ -21,6 +21,7 @@ target_link_libraries(${MODULE_NAME}
        date::date
        date::date-tz
        everest::timer
+        nlohmann_json::nlohmann_json


I think this can be removed?

Suggested change

nlohmann_json::nlohmann_json

I removed it (and will check if it still builds). But it is auto generated so it will be put back in again later probably?

This part is not autogenerated

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 added 2 commits November 12, 2024 15:48

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 7 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]>

Reservation id 0 is a valid reservation id, so set initialization of …

8a3e188

…that id to -1. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Some changes to check if a reservation exists for a specific evse id …

d89cfb0

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

Function to convert ReservationCheckStatus. Register is_token_reserve…

36fbc1d

…d_for_connector_callback for OCPP module. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Pietfried reviewed Nov 25, 2024

View reviewed changes

maaikez added 4 commits November 25, 2024 13:02

Review comments.

b858dba

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

Review comments.

1423237

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

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

4b87c25

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

f89aa47

Pietfried approved these changes Nov 26, 2024

View reviewed changes

Bump libocpp version

60d7ac1

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

maaikez merged commit fcc2088 into main Nov 26, 2024
11 of 12 checks passed

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

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

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

Pietfried Nov 25, 2024

Pietfried Nov 25, 2024

Pietfried Nov 25, 2024

Pietfried Nov 25, 2024

maaikez Nov 25, 2024

Pietfried Nov 25, 2024

Pietfried Nov 25, 2024

maaikez Nov 25, 2024

Pietfried Nov 25, 2024

		Returns true if there is a reservation for the given evse id and token or group id token. If there is an
		evse id given, it will also return true if a reservation with this token was made for evse id 0.

Reservation changes for 2.0.1 #943

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

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

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

maaikez commented Oct 31, 2024 •

edited

Loading

maaikez Nov 13, 2024 •

edited

Loading