From 0beb62768f5225b0f3bbd5c2ff0602ae5819332c Mon Sep 17 00:00:00 2001 From: "Maaike Zijderveld, iolar" Date: Fri, 13 Dec 2024 15:57:51 +0100 Subject: [PATCH] Do not cancel the reservation when it is used from the reservation handler, 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 --- modules/Auth/lib/ReservationHandler.cpp | 2 +- modules/Auth/tests/reservation_tests.cpp | 6 +- modules/EvseManager/evse/evse_managerImpl.cpp | 7 +- modules/OCPP201/OCPP201.cpp | 4 +- .../test_sets/ocpp201/reservations.py | 96 +++++++++++++++---- 5 files changed, 92 insertions(+), 23 deletions(-) diff --git a/modules/Auth/lib/ReservationHandler.cpp b/modules/Auth/lib/ReservationHandler.cpp index ae566a19a..6e4a10571 100644 --- a/modules/Auth/lib/ReservationHandler.cpp +++ b/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> 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() diff --git a/modules/Auth/tests/reservation_tests.cpp b/modules/Auth/tests/reservation_tests.cpp index 53c5a07f0..433ed3408 100644 --- a/modules/Auth/tests/reservation_tests.cpp +++ b/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); diff --git a/modules/EvseManager/evse/evse_managerImpl.cpp b/modules/EvseManager/evse/evse_managerImpl.cpp index 4b8f424d8..e1ba78a83 100644 --- a/modules/EvseManager/evse/evse_managerImpl.cpp +++ b/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", diff --git a/modules/OCPP201/OCPP201.cpp b/modules/OCPP201/OCPP201.cpp index 3e7b03427..0f63b9f65 100644 --- a/modules/OCPP201/OCPP201.cpp +++ b/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; diff --git a/tests/ocpp_tests/test_sets/ocpp201/reservations.py b/tests/ocpp_tests/test_sets/ocpp201/reservations.py index 25d918f0a..9a83eddfe 100644 --- a/tests/ocpp_tests/test_sets/ocpp201/reservations.py +++ b/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,