Reservation all connectors reserved (#958) #972

maaikez · 2024-11-27T14:04:26Z

Describe your changes

When there are as many reservations as evse's available, all available evse's must go to 'reserved'. And back to 'available' when there are more evse's than reservations.

Also, added unittests.

See other related pull requests:
EVerest/libocpp#878
EVerest/everest-utils#171

Issue ticket number and link

#958

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

Pietfried

lgtm, added a few suggestions

Pietfried · 2024-12-11T07:34:04Z

interfaces/evse_manager.yaml

+          The reservation id (should be added to the TransactionStarted event). Negative value if there was
+          no specific reservation id for this evse (if there are 'global' reservations for 'any' evse).


Suggested change

The reservation id (should be added to the TransactionStarted event). Negative value if there was

no specific reservation id for this evse (if there are 'global' reservations for 'any' evse).

The reservation id (should be added to the TransactionStarted event). Set tis to a negative value if there is

no specific reservation id for this evse but the evse should still move to a Reserved state because of total

global reservations.

Pietfried · 2024-12-11T07:38:03Z

types/reservation.yaml

@@ -54,11 +54,15 @@ types:
        Expired: When the reservation expired before the reserved token was used for a session
        Cancelled: When the reservation was cancelled manually
        UsedToStartCharging: When the reservation ended because the reserved token was used for a session
+        GlobalReservationConnectorFree: When the reservation ended because there is a connector free and there are less
+                                        reservations than available evse's (reservation is still there but it is not
+                                        occupying the EVSE anymore).


Suggested change

occupying the EVSE anymore).

occupying this EVSE anymore).

Pietfried · 2024-12-11T07:39:41Z

types/reservation.yaml

    type: string
    enum:
      - Expired
      - Cancelled
      - UsedToStartCharging
+      - GlobalReservationConnectorFree


Since I was a little confused by the name.. maybe GlobalReservationRequirementDropped ?

Pietfried · 2024-12-11T07:41:21Z

tests/ocpp_tests/test_sets/ocpp16/ocpp_compliance_tests.py

+    test_controller.clear_error(1)
+
+    await asyncio.sleep(1)
+


This doesnt look like its required at the end of the test case

Pietfried · 2024-12-11T07:42:01Z

tests/ocpp_tests/test_sets/ocpp16/ocpp_compliance_tests.py

-    test_controller.diode_fail()
+    test_controller.raise_error("MREC6UnderVoltage", 1)
+
+    await asyncio.sleep(1)


I dont think that we have to sleep here since wait_for_and_validate will wait for the error event already

Well I gave it a try and removed it again, but we have a race condition here. The statusnotification is sent earlier than the status is received by the auth handler so the reservation handler does not know of the state of the connector yet. :(

Pietfried · 2024-12-11T07:44:36Z

tests/ocpp_tests/test_sets/ocpp201/reservations.py

+    test_controller.raise_error("MREC6UnderVoltage", 1)
+
+    await asyncio.sleep(1)
+
+    t = datetime.utcnow() + timedelta(minutes=10)
+
+    # Send a reserve new request
+    await charge_point_v201.reserve_now_req(
+        id=0,
+        id_token=IdTokenType(id_token=test_config.authorization_info.valid_id_tag_1,
+                             type=IdTokenTypeEnum.iso14443),
+        expiry_date_time=t.isoformat(),
+        evse_id=1
+    )
+
+    # which should return 'faulted'
+    assert await wait_for_and_validate(
+        test_utility,
+        charge_point_v201,
+        "ReserveNow",
+        call_result201.ReserveNowPayload(ReserveNowStatusType.faulted),
+    )


also here you can do drop the sleep if you do

raise_error

wait and validate for StatusNotification.req(Faulted)

ReserveNow

wait and validate ReserveNow

Same here:
We have a race condition here. The statusnotification is sent earlier than the status is received by the auth handler so the reservation handler does not know of the state of the connector yet. :(

Pietfried · 2024-12-11T07:45:26Z

tests/ocpp_tests/test_sets/ocpp201/reservations.py

+        ),
+    )
+
+    await asyncio.sleep(1)


I think this can also be dropped.

Pietfried · 2024-12-11T07:55:44Z

modules/Auth/lib/ReservationHandler.cpp

@@ -400,6 +413,52 @@ bool ReservationHandler::has_reservation_parent_id(const std::optional<uint32_t>
    return false;
 }

+ReservationEvseStatus ReservationHandler::check_number_global_reservations_match_number_available_evses() {
+    ReservationEvseStatus evse_status;


Suggested change

ReservationEvseStatus evse_status;

This looks like it is not used

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

…s and notify evse manager. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…eserved if no more reservations can be made. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

…rvation was made for connector 0. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

… after a car stopped charging. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

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

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

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

…ndler, only cancel it internally. The EVSE Manager will cancel the reservation itself. If the ReservationHandler is calling the callback, there will be race conditions with the evse manager not knowing there is a transaction yet, but the reservation is already cancelled. And some other bug fixes. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

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

maaikez requested review from corneliusclaussen, SebaLukas, hikinggrass and Pietfried as code owners November 27, 2024 14:04

maaikez force-pushed the feature/958-reservation-all-connectors-reserved branch from 44f3b63 to ec79914 Compare November 27, 2024 14:37

maaikez marked this pull request as draft November 27, 2024 14:50

maaikez force-pushed the feature/958-reservation-all-connectors-reserved branch from e7b1ad9 to b359a9f Compare November 27, 2024 15:17

This was referenced Nov 27, 2024

Feature/958 reservation all connectors reserved EVerest/libocpp#878

Merged

Be able to raise and clear error in test controller EVerest/everest-utils#171

Merged

maaikez force-pushed the feature/958-reservation-all-connectors-reserved branch 2 times, most recently from aace320 to b1476a4 Compare November 28, 2024 14:38

maaikez marked this pull request as ready for review November 28, 2024 16:18

Pietfried self-assigned this Dec 3, 2024

maaikez force-pushed the feature/958-reservation-all-connectors-reserved branch 4 times, most recently from 19ae787 to 37f9f0d Compare December 10, 2024 13:39

Pietfried approved these changes Dec 11, 2024

View reviewed changes

maaikez force-pushed the feature/958-reservation-all-connectors-reserved branch 2 times, most recently from 9c9f185 to 0beb627 Compare December 16, 2024 09:40

maaikez added 10 commits December 16, 2024 12:27

Initial commit.

3635ad0

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

Check reservation status for 'global' reservation and number of evse'…

53e59f6

…s and notify evse manager. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Check number of evse's and number of reservations and set evse's to r…

b1dae5e

…eserved if no more reservations can be made. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Remove todo's and add mutex

e744b25

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

Add python tests for reservations, for 1.6 and 2.0.1

1d62f22

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

Remove changing connector state for connector id 0 in 1.6 when a rese…

6d30934

…rvation was made for connector 0. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Remove todo and change logging minor detail.

d0036c9

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

Some documentation and removing of logging to start the pull request.

3392776

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

Avoid race conditions. Add test that evse is going to available again…

45eb331

… after a car stopped charging. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Formatting...

d06a7e4

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

maaikez added 10 commits December 16, 2024 12:27

Fix bug where reserved callback was called without checking if it is …

a857b83

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

Fix some codacy issues, add documentation.

a5f42bb

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

Fix test.

b2eb2b0

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

Set everest utils to correct version.

5a393a0

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

Fix tests.

6f32be4

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

Fix ocpp 16 test as well for raise_error

b2a5665

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

Review comments.

a2bd096

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

Add test that covers (current existing) bug in evse manager. Skip tes…

5572a16

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

Update libocpp dependency

4314efc

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

maaikez force-pushed the feature/958-reservation-all-connectors-reserved branch from 47b7f5d to 4314efc Compare December 16, 2024 11:28

maaikez merged commit 4180de7 into main Dec 16, 2024
11 checks passed

maaikez deleted the feature/958-reservation-all-connectors-reserved branch December 16, 2024 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reservation all connectors reserved (#958) #972

Reservation all connectors reserved (#958) #972

maaikez commented Nov 27, 2024 •

edited

Loading

Pietfried left a comment

Pietfried Dec 11, 2024

Pietfried Dec 11, 2024

Pietfried Dec 11, 2024

Pietfried Dec 11, 2024

Pietfried Dec 11, 2024

maaikez Dec 11, 2024

Pietfried Dec 11, 2024

maaikez Dec 11, 2024

Pietfried Dec 11, 2024

Pietfried Dec 11, 2024

		The reservation id (should be added to the TransactionStarted event). Negative value if there was
		no specific reservation id for this evse (if there are 'global' reservations for 'any' evse).

-          The reservation id (should be added to the TransactionStarted event). Negative value if there was
-          no specific reservation id for this evse (if there are 'global' reservations for 'any' evse).
+          The reservation id (should be added to the TransactionStarted event). Set tis to a negative value if there is
+          no specific reservation id for this evse but the evse should still move to a Reserved state because of total
+          global reservations.

Reservation all connectors reserved (#958) #972

Reservation all connectors reserved (#958) #972

Conversation

maaikez commented Nov 27, 2024 • edited Loading

Describe your changes

Issue ticket number and link

Checklist before requesting a review

Pietfried 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

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 Nov 27, 2024 •

edited

Loading