Do not cancel the reservation when it is used from the reservation ha…

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

modules/Auth/lib/ReservationHandler.cpp

@@ -334,7 +334,7 @@ void ReservationHandler::register_reservation_cancelled_callback(
 
 void ReservationHandler::on_reservation_used(const int32_t reservation_id) {
     const std::pair<bool, std::optional<uint32_t>> cancelled =
-        this->cancel_reservation(reservation_id, true, types::reservation::ReservationEndReason::UsedToStartCharging);
+        this->cancel_reservation(reservation_id, false, types::reservation::ReservationEndReason::UsedToStartCharging);
     if (cancelled.first) {
         if (cancelled.second.has_value()) {
             EVLOG_info << "Reservation (" << reservation_id << ") for evse#" << cancelled.second.value()

modules/Auth/tests/reservation_tests.cpp

@@ -949,14 +949,14 @@ TEST_F(ReservationHandlerTest, reservation_timer) {
     Reservation reservation = create_reservation(types::evse_manager::ConnectorTypeEnum::cCCS2);
     reservation.expiry_time = Everest::Date::to_rfc3339(date::utc_clock::now() + std::chrono::seconds(1));
     EXPECT_EQ(r.make_reservation(std::nullopt, reservation), ReservationResult::Accepted);
-    sleep(1);
+    sleep(2);
     EXPECT_FALSE(evse_id.has_value());
 
     EXPECT_CALL(reservation_callback_mock, Call(_, 0, ReservationEndReason::Expired, true))
         .WillOnce(SaveArg<0>(&evse_id));
     reservation.expiry_time = Everest::Date::to_rfc3339(date::utc_clock::now() + std::chrono::seconds(1));
     EXPECT_EQ(r.make_reservation(0, reservation), ReservationResult::Accepted);
-    sleep(1);
+    sleep(2);
     ASSERT_TRUE(evse_id.has_value());
     EXPECT_EQ(evse_id.value(), 0);
 }
@@ -1153,7 +1153,7 @@ TEST_F(ReservationHandlerTest, on_reservation_used) {
 
     r.register_reservation_cancelled_callback(reservation_callback_mock.AsStdFunction());
 
-    EXPECT_CALL(reservation_callback_mock, Call(_, _, _, true)).Times(3);
+    EXPECT_CALL(reservation_callback_mock, Call(_, _, _, true)).Times(0);
 
     add_connector(0, 0, types::evse_manager::ConnectorTypeEnum::cCCS2, this->evses);
     add_connector(0, 1, types::evse_manager::ConnectorTypeEnum::cType2, this->evses);

modules/EvseManager/evse/evse_managerImpl.cpp

@@ -175,6 +175,9 @@ void evse_managerImpl::ready() {
             session_started.id_tag = provided_id_token;
             if (mod->is_reserved()) {
                 session_started.reservation_id = mod->get_reservation_id();
+                if (start_reason == types::evse_manager::StartSessionReason::Authorized) {
+                    this->mod->cancel_reservation(true);
+                }
             }
 
             session_started.logging_path = session_log.startSession(
@@ -206,7 +209,7 @@ void evse_managerImpl::ready() {
         transaction_started.meter_value = mod->get_latest_powermeter_data_billing();
         if (mod->is_reserved()) {
             transaction_started.reservation_id.emplace(mod->get_reservation_id());
-            mod->cancel_reservation(false);
+            mod->cancel_reservation(true);
         }
 
         transaction_started.id_tag = id_token;
@@ -292,7 +295,7 @@ void evse_managerImpl::ready() {
             // Cancel reservation, reservation might be stored when swiping rfid, but timed out, so we should not
             // set the reservation id here.
             if (mod->is_reserved()) {
-                mod->cancel_reservation(false);
+                mod->cancel_reservation(true);
             }
             session_log.stopSession();
             mod->telemetry.publish("session", "events",

modules/OCPP201/OCPP201.cpp

@@ -1114,7 +1114,9 @@ void OCPP201::process_transaction_started(const int32_t evse_id, const int32_t c
     auto tx_event = TxEvent::AUTHORIZED;
     auto trigger_reason = ocpp::v201::TriggerReasonEnum::Authorized;
     const auto transaction_started = session_event.transaction_started.value();
-    transaction_data->reservation_id = transaction_started.reservation_id;
+    if (transaction_started.reservation_id.has_value()) {
+        transaction_data->reservation_id = transaction_started.reservation_id;
+    }
     transaction_data->remote_start_id = transaction_started.id_tag.request_id;
     const auto id_token = conversions::to_ocpp_id_token(transaction_started.id_tag.id_token);
     transaction_data->id_token = id_token;

tests/ocpp_tests/test_sets/ocpp201/reservations.py

@@ -106,8 +106,85 @@ async def test_reservation_local_start_tx(
 
 
 @pytest.mark.asyncio
-@pytest.mark.skip(reason="There is a bug in the EVSE manager with setting an evse to occupied on plug in without "
-                         "checking if it is reserved. As soon as it is fixed, this test can be done")
+@pytest.mark.ocpp_version("ocpp2.0.1")
+async def test_reservation_local_start_tx_plugin_first(
+        test_config: OcppTestConfiguration,
+        charge_point_v201: ChargePoint201,
+        test_controller: TestController,
+        test_utility: TestUtility,
+):
+    """
+    Test making a reservation and start a transaction on the reserved evse id with the correct id token.
+    """
+    logging.info("######### test_reservation_local_start_tx #########")
+
+    t = datetime.utcnow() + timedelta(minutes=10)
+
+    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
+    )
+
+    # expect ReserveNow response with status accepted
+    assert await wait_for_and_validate(
+        test_utility,
+        charge_point_v201,
+        "ReserveNow",
+        call_result201.ReserveNowPayload(ReserveNowStatusType.accepted),
+    )
+
+    # expect StatusNotification with status reserved
+    assert await wait_for_and_validate(
+        test_utility,
+        charge_point_v201,
+        "StatusNotification",
+        call_201.StatusNotificationPayload(
+            ANY, ConnectorStatusType.reserved, 1, 1
+        ),
+    )
+
+    test_utility.messages.clear()
+
+    # start charging session
+    test_controller.plug_in()
+
+    await asyncio.sleep(2)
+
+    test_utility.messages.clear()
+
+    # swipe valid id tag that belongs to this reservation to authorize
+    test_controller.swipe(test_config.authorization_info.valid_id_tag_1)
+
+    # expect StatusNotification with status available (reservation is now used)
+    assert await wait_for_and_validate(
+        test_utility,
+        charge_point_v201,
+        "StatusNotification",
+        call_201.StatusNotificationPayload(
+            ANY, ConnectorStatusType.occupied, 1, 1
+        ),
+    )
+
+    # expect TransactionEvent with event type Started and the reservation id.
+    assert await wait_for_and_validate(
+        test_utility,
+        charge_point_v201,
+        "TransactionEvent",
+        {"eventType": "Started", "reservationId": 0}
+    )
+
+    assert await wait_for_and_validate(
+        test_utility,
+        charge_point_v201,
+        "TransactionEvent",
+        {"eventType": "Updated"}
+    )
+
+
+@pytest.mark.asyncio
 @pytest.mark.ocpp_version("ocpp2.0.1")
 async def test_reservation_plug_in_other_idtoken(
         test_config: OcppTestConfiguration,
@@ -167,7 +244,7 @@ async def test_reservation_plug_in_other_idtoken(
     test_utility.messages.clear()
 
     # swipe invalid id tag
-    test_controller.swipe(test_config.authorization_info.invalid_id_tag)
+    test_controller.swipe(test_config.authorization_info.valid_id_tag_2)
 
     assert not await wait_for_and_validate(
         test_utility,
@@ -184,19 +261,6 @@ async def test_reservation_plug_in_other_idtoken(
     # swipe valid id tag to authorize
     test_controller.swipe(test_config.authorization_info.valid_id_tag_1)
 
-    # expect StatusNotification with status available (reservation is now used)
-    assert await wait_for_and_validate(
-        test_utility,
-        charge_point_v201,
-        "StatusNotification",
-        call_201.StatusNotificationPayload(
-            ANY, ConnectorStatusType.available, 1, 1
-        ),
-    )
-
-    # start charging session
-    test_controller.plug_in()
-
     # expect TransactionEvent with event type Started and the reservation id.
     assert await wait_for_and_validate(
         test_utility,