Reset with ongoing transaction #128

maaikez · 2023-07-24T13:57:50Z

Reset with ongoing transaction.

Because we have to resend a transaction message if there is no 'ack' returned before rebooting, the database is also changed. I made this a draft review because I can not test it yet and I am a bit unsure about:

Are the database changes ok and is it possible for someone to test this?
I added the part about OnIdle and ImmediateReset (currently line 865 - 903 in chargepoint.cpp). Is it possible to test this? Will the submit_event work and will it work that the charging station will reset as soon as a transaction ends?

Thanks for looking into it!

hikinggrass · 2023-08-16T12:53:33Z

include/ocpp/common/message_queue.hpp

@@ -289,12 +314,13 @@ template <typename M> class MessageQueue {
    }

    MessageQueue(const std::function<bool(json message)>& send_callback, const int transaction_message_attempts,
-                 const int transaction_message_retry_interval) :
-        MessageQueue(send_callback, transaction_message_attempts, transaction_message_retry_interval, {}){};
+                 const int transaction_message_retry_interval, common::DatabaseHandlerBase& databaseHandler) :


Would it be possible to pass the databaseHandler as a std::shared_ptr here? It's already a sharedptr in the charge point

No in the chargepoint it is a unique ptr. I can make it a shared ptr if you want, then I can pass it as shared ptr of course. But actually the charge point is the 'owner' of the pointer

Oh I totally forgot about that, then we'll have to check if the access to the database handler with a shared ptr is a good idea. unpacking the unique ptr here is definitely not recommended

hikinggrass · 2023-08-16T12:54:00Z

lib/ocpp/v16/charge_point_impl.cpp

@@ -47,7 +47,7 @@ ChargePointImpl::ChargePointImpl(const std::string& config, const std::filesyste
    this->message_queue = std::make_unique<ocpp::MessageQueue<v16::MessageType>>(
        [this](json message) -> bool { return this->websocket->send(message.dump()); },
        this->configuration->getTransactionMessageAttempts(), this->configuration->getTransactionMessageRetryInterval(),
-        this->external_notify);
+        this->external_notify, *this->database_handler.get());


If you would use the sharedptr here you would not have to unpack it to pass it

Pietfried · 2023-08-16T13:31:54Z

lib/ocpp/v201/charge_point.cpp

            // B12.FR.04
+            for (const auto &[evse_id, evse] : this->evses) {
+                if (evse->has_active_transaction()) {
+                    callbacks.stop_transaction_callback(


This leads to a race condition with a TransactionEvent(Ended) because this TransactionEvent(Ended) could be queued faster than the response to Reset.req . This could be solved either by responding to this message before executing the stop_transaction_callback(s) or have a look at how this was solved for OCPP1.6: https://github.com/EVerest/libocpp/blob/main/lib/ocpp/v16/charge_point_impl.cpp#L1519 . A seperate threads waits for all transactions to be stopped before the reset_callback is called. There could be a (configurable) timeout for waiting for the responses (other than in implemented 1.6)

Changed it, but not with the 5s because that's up to the charging station how long that can take. Can you take a look if this is better?

Also worked on the other comment about the message queue that is sending things before the accepted bootnotification. Is it better with this change?

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

…m the queue in the database, so it can be sent after reboot if it is not sended yet. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

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

…queue can be stored in both versions (16 and 201). Put transaction in database as soon as it is received. Remove transaction message from database when reply is received. Some small improvements in for loop with copying / reference. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

…n be performed as soon as the charging has stopped. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

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

…lability handler on reset. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

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

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

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

…ccepted' Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

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

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

* Start with 'reset with ongoing transaction'. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Start with database related things to store a transaction message from the queue in the database, so it can be sent after reboot if it is not sended yet. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Implement database insert / remove / get functions. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Add database base class in common library so the transactions in the queue can be stored in both versions (16 and 201). Put transaction in database as soon as it is received. Remove transaction message from database when reply is received. Some small improvements in for loop with copying / reference. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Add evse id to reset callback. Store scheduled 'OnIdle' reset so it an be performed as soon as the charging has stopped. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Add evseid to is_reset_allowed_callback Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Add documentation. Reset when all evse id's stopped charging. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * 'initialize' sqlite db member in the constructor. Add documentation Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Make it possible to call the reset multiple times. Also call the availability handler on reset. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Add two templates for conversion from message type to string. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Change start() function and add optional boot reason. Signed-off-by: Maaike <[email protected]> * Add documentation about start function Signed-off-by: Maaike <[email protected]> * sqlite bind text fixes. Signed-off-by: Maaike <[email protected]> * Set flag to persist availability after reboot / reset Signed-off-by: Maaike <[email protected]> * Add persist to as well Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Review comments about race conditions Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * Fill transaction database after bootnotification is receveived and 'accepted' Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * change database handler to shared ptr instead of unique ptr Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> * clang-format Signed-off-by: Kai-Uwe Hermann <[email protected]> --------- Signed-off-by: Maaike Zijderveld, Alfen <[email protected]> Signed-off-by: Maaike <[email protected]> Signed-off-by: Kai-Uwe Hermann <[email protected]> Co-authored-by: Kai-Uwe Hermann <[email protected]>

maaikez requested review from hikinggrass and Pietfried and removed request for hikinggrass July 24, 2023 13:57

maaikez changed the title ~~Mz reset with ongoing transaction~~ Reset with ongoing transaction Jul 24, 2023

maaikez force-pushed the mz-reset-with-ongoing-transaction branch 2 times, most recently from 7726349 to 5cac860 Compare August 7, 2023 12:48

maaikez force-pushed the mz-reset-with-ongoing-transaction branch from a1ff7bb to 3320a1c Compare August 11, 2023 07:20

maaikez marked this pull request as ready for review August 11, 2023 07:21

hikinggrass requested changes Aug 16, 2023

View reviewed changes

Pietfried reviewed Aug 16, 2023

View reviewed changes

maaikez added 18 commits August 21, 2023 17:56

Start with 'reset with ongoing transaction'.

e19fd7c

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

Start with database related things to store a transaction message fro…

cfdb19a

…m the queue in the database, so it can be sent after reboot if it is not sended yet. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

Implement database insert / remove / get functions.

78b00ce

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

Add evse id to reset callback. Store scheduled 'OnIdle' reset so it a…

08f8ada

…n be performed as soon as the charging has stopped. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

Add evseid to is_reset_allowed_callback

db26920

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

Add documentation. Reset when all evse id's stopped charging.

39fd483

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

'initialize' sqlite db member in the constructor. Add documentation

e1ecc24

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

Make it possible to call the reset multiple times. Also call the avai…

5e46f40

…lability handler on reset. Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

Add two templates for conversion from message type to string.

20e2466

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

Change start() function and add optional boot reason.

c0c2afd

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

Add documentation about start function

aa70197

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

sqlite bind text fixes.

8e5715e

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

Set flag to persist availability after reboot / reset

2afacbe

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

Add persist to as well

45c08ba

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

Review comments about race conditions

d2dd5ec

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

Fill transaction database after bootnotification is receveived and 'a…

d6d1831

…ccepted' Signed-off-by: Maaike Zijderveld, Alfen <[email protected]>

change database handler to shared ptr instead of unique ptr

32b9209

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

hikinggrass force-pushed the mz-reset-with-ongoing-transaction branch from b0d3aa9 to 32b9209 Compare August 21, 2023 16:26

clang-format

979cdb2

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

hikinggrass mentioned this pull request Aug 21, 2023

Change OCPP2.0.1 module callback signatures EVerest/everest-core#322

Merged

maaikez requested review from hikinggrass and Pietfried August 22, 2023 09:21

hikinggrass approved these changes Aug 22, 2023

View reviewed changes

hikinggrass merged commit c365fe4 into main Aug 22, 2023

hikinggrass deleted the mz-reset-with-ongoing-transaction branch August 22, 2023 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset with ongoing transaction #128

Reset with ongoing transaction #128

maaikez commented Jul 24, 2023

hikinggrass Aug 16, 2023

maaikez Aug 17, 2023

hikinggrass Aug 17, 2023

hikinggrass Aug 16, 2023

Pietfried Aug 16, 2023 •

edited

Loading

maaikez Aug 17, 2023

Reset with ongoing transaction #128

Reset with ongoing transaction #128

Conversation

maaikez commented Jul 24, 2023

hikinggrass Aug 16, 2023

Choose a reason for hiding this comment

maaikez Aug 17, 2023

Choose a reason for hiding this comment

hikinggrass Aug 17, 2023

Choose a reason for hiding this comment

hikinggrass Aug 16, 2023

Choose a reason for hiding this comment

Pietfried Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

maaikez Aug 17, 2023

Choose a reason for hiding this comment

Pietfried Aug 16, 2023 •

edited

Loading