Fix: OCPP 2.0.1 CS, EVSE, and Connector availability tracking #292

valentin-dimov · 2023-12-06T16:37:00Z

Companion PR from everest-core: EVerest/everest-core#459

valentin-dimov · 2023-12-07T09:53:48Z

Confirmed that OCTT Availability test cases pass with this and EVerest/everest-core#459
Should be ready for review now.

rinzevdwalAlfen · 2023-12-11T08:58:20Z

When running OCTT test change availability charging station - inoperative to operative we get unexpected state changes from available to unavailable and back again multiple times. Can you please check on your end if this might be caused by these changes?

rinzevdwalAlfen · 2023-12-11T14:37:36Z

Testing the code we see that the callbacks persist_availability_setting_callback and change_effective_availability_callback are called multiple times, while we expect it to be called once. This is causing problems (race conditions) on our end.

In the following logging I added where the callbacks are called.

2023-Dec-11 13:59:55.410560 BBOC  (BackOfficeCommu:225)  [comm    ] [in ][2, "1112611f-ce33-42a7-9270-502bc90f3049", "ChangeAvailability", {"operationalStatus":"Operative"}]
2023-Dec-11 13:59:55.410685 BBOC  (BackOfficeCommu:222)  [comm    ] [out][3,"1112611f-ce33-42a7-9270-502bc90f3049",{"status":"Accepted"}]
2023-Dec-11 13:59:55.410765 BBOC  (AvailabilityHan:292)  [info    ] status: Operative persist: false **<-- (change_effective_availability_callback)**
2023-Dec-11 13:59:55.437280 BBOC  (TransactionHand:299)  [info    ] Connector #1: : Change: 160 ---> 1
2023-Dec-11 13:59:55.437376 BBOC  (AvailabilityHan:403)  [info    ] connectorStatus: Available
                                                                        OperationalStatusEnum: Operative
2023-Dec-11 13:59:55.437572 BBOC  (AvailabilityHan:292)  [info    ] status: Operative persist: true **<-- (persist_availability_setting_callback)**
2023-Dec-11 13:59:55.437645 BBOC  (BackOfficeCommu:222)  [comm    ] [out][2,"ad3dcfa5-0695-488e-a71b-8006db2ca3c6","StatusNotification",{"connectorId":1,"connectorStatus":"Available","evseId":1,"timestamp":"2023-12-11T13:59:55.437Z"}]
2023-Dec-11 13:59:55.442844 BBOC  (AvailabilityHan:292)  [info    ] status: Operative persist: false **<-- (change_effective_availability_callback)**

valentin-dimov · 2023-12-11T14:48:45Z

libocpp will call the change_effective_availability_callback once for every component whose effective state changed - for example, if an EVSE and all of its connectors are initially enabled, and we change the EVSE's availability to Inoperative, the callback will be called once for the EVSE, then once for each connector.

The persist_availability_setting_callback isn't supposed to trigger multiple times per request - if that does happen, that would be a bug. However, I only see one invocation of that callback in the log you attached.

klemmpnx

Technically I think this works fine; however, I am afraid it is still too complex (might be hard to maintain in the future)

A "simpler" thing I would suggest is to make the public interfaces more explicit (instead of having a lot of optional values, split the public interface into dedicated methods that clearly express the effect in the name (set_evse_operational_status, set_connector_operational_status; drop this "is_boot" parameter but instead create a update method to recompute the effective status)

But an actual thought I have: To me it seems a lot of the code complexity stems from keeping the statuses in sync. So as first step I would suggest to extract this responsibility of computing the effective status (since it needs to have a "global" view on the cs, evses, connectors ) into a dedicated class that can compute the effective status for each level. And then indeed I would rather just compute the effective state each time it is requested (since that is cheap) instead of having is stored redundantly.

But let's talk about this rather in a call ;)

klemmpnx · 2023-12-13T20:11:14Z

lib/ocpp/v201/database_handler.cpp

+    insert_stmt.bind_int("@evse_id", evse_id.value_or(0));
+    insert_stmt.bind_int("@connector_id", connector_id.value_or(0));


Do I understand correctly that for connector_id = 0 you refer to the evse, and for both 0 the whole CS?
And evse_id = 0 is invalid for any connector_id != 0 , right?

If so, I would suggest to split up the public interface of this handler into three dedicated methods for each use case ( insert_cs_availability (no ids as inputs), insert_evse_availability (evse id as input), insert_connector_availability (evse and connector ids as inputs) which then can privately call exactly this implementation (would be clearer to me and avoid possible misusage)

Refactored!

klemmpnx · 2023-12-13T20:11:33Z

lib/ocpp/v201/database_handler.cpp

+OperationalStatusEnum DatabaseHandler::get_availability(const std::optional<int32_t> evse_id,
+                                                        std::optional<int32_t> connector_id) {


same as for insert

klemmpnx · 2023-12-13T20:12:11Z

config/v201/__pycache__/init_device_model_db.cpython-310.pyc

commited by accident

config/v201/init_core.sql

include/ocpp/v201/charge_point.hpp

klemmpnx · 2023-12-13T20:26:26Z

lib/ocpp/v201/charge_point.cpp

+void ChargePoint::set_operative_status(std::optional<int32_t> evse_id, std::optional<int32_t> connector_id,
+                                       OperationalStatusEnum new_status, bool persist, bool is_boot) {
+    OperationalStatusEnum old_op_status = this->operative_status;
+    if (!evse_id.has_value()) {


I understand this works correctly, but I would assume in the future this can get confusing again for someone not that deep into how this works ("why don't I care about the connector_id?")

Refactored - there are now separate methods for setting CS, EVSE, and connector statuses.

klemmpnx

Put a bunch of comments into the code; not fully conclusive, but I would suggest best way to deal with it is just have a pairing session (in the end I think this is all really just about readability and maintainability of the code, which is also a matter of style & taste of course)

include/ocpp/v201/charge_point.hpp

lib/ocpp/v201/charge_point.cpp

include/ocpp/v201/component_state_manager.hpp

include/ocpp/v201/charge_point.hpp

include/ocpp/v201/component_state_manager.hpp

valentin-dimov · 2023-12-19T13:38:31Z

lib/ocpp/v201/charge_point.cpp

    if (!transaction_active) {
-        // execute change availability if possible
-        this->callbacks.change_availability_callback(msg, true);
+        // No transactions - execute the change now
+        this->execute_change_availability_request(msg, true);


Note: This differs from the original behaviour - in this current state, libocpp will NOT call the callback if the effective state of the components did not change.

valentin-dimov · 2023-12-19T13:46:43Z

lib/ocpp/v201/charge_point.cpp

            for (auto const& [evse_id, evse] : this->evses) {
                if (!evse->has_active_transaction()) {
-                    this->set_evse_connectors_unavailable(evse, false);
+                    // FIXME: This will linger after the update too! We probably need another mechanism...


This is probably a bug. Need to write an issue/ticket about this.

klemmpnx · 2023-12-19T14:16:02Z

@hikinggrass : I reviewed this together with @valentin-dimov ; besides the two comments by @valentin-dimov (those we can discuss; probably those are out of scope for this PR), for me this looks good - but I think you (or someone as involved as you) should approve/re-check first

rinzevdwalAlfen · 2024-01-09T15:50:25Z

Just tested the branch.

There seems to be bug where if you change the chargingStation to inoperative and then back of operative, to connector is not updated to operative. You have to send a operative for the connector specifically to fix it afterwards.

See our logging where I added the callback as logging

2024-Jan-09 15:46:37.896884 BBOC  (BackOfficeCommu:219)  [comm    ] [in ][3,"4528285b-466a-4e01-bcbb-57c0c3e59aa7",{}]
2024-Jan-09 15:47:31.443887 BBOC  (BackOfficeCommu:219)  [comm    ] [in ][2,"1d81f457-3454-4fcc-a6c9-88a848001b52","ChangeAvailability",{"operationalStatus":"Inoperative"}]
2024-Jan-09 15:47:31.444615 BBOC  (BackOfficeCommu:216)  [comm    ] [out][3,"1d81f457-3454-4fcc-a6c9-88a848001b52",{"status":"Accepted"}]
2024-Jan-09 15:47:31.462177 BBOC  (BackOfficeCommu:164)  [info    ] connector_effective_operative_status_changed_callback: evse_id: 1 connector_id: 1 new_status: Inoperative
2024-Jan-09 15:47:31.464886 BBOC  (BackOfficeCommu:216)  [comm    ] [out][2,"998d38b3-8595-4955-840d-f8cbc46f6d9b","StatusNotification",{"connectorId":1,"connectorStatus":"Unavailable","evseId":1,"timestamp":"2024-01-09T15:47:31.464Z"}]
2024-Jan-09 15:47:31.502163 BBOC  (BackOfficeCommu:219)  [comm    ] [in ][3,"998d38b3-8595-4955-840d-f8cbc46f6d9b",{}]
2024-Jan-09 15:47:31.510147 BBOC  (TransactionHand:299)  [info    ] Connector #1: : Change: 1 ---> 160
2024-Jan-09 15:47:37.812339 BBOC  (device_model.cp:112)  [warning ] [libocpp]  unknown component in ISO15118Ctrlr.V2GCertificateInstallationEnabled
2024-Jan-09 15:47:37.812580 BBOC  (device_model.cp:112)  [warning ] [libocpp]  unknown component in ISO15118Ctrlr.PnCEnabled
2024-Jan-09 15:47:37.812644 BBOC  (device_model.cp:120)  [warning ] [libocpp]  unknown variable in InternalCtrlr.V2GCertificateExpireCheckIntervalSeconds
2024-Jan-09 15:47:47.007601 BBOC  (BackOfficeCommu:219)  [comm    ] [in ][2,"f0859086-7e87-48dd-a662-7923e6c0453c","ChangeAvailability",{"operationalStatus":"Operative"}]
2024-Jan-09 15:47:47.007769 BBOC  (BackOfficeCommu:216)  [comm    ] [out][3,"f0859086-7e87-48dd-a662-7923e6c0453c",{"status":"Accepted"}]
2024-Jan-09 15:47:54.758691 BBOC  (BackOfficeCommu:219)  [comm    ] [in ][2,"689c5392-9336-41ba-bb26-b55470e629fd","ChangeAvailability",{"operationalStatus":"Operative","evse":{"id":1,"connectorId":1}}]
2024-Jan-09 15:47:54.758992 BBOC  (BackOfficeCommu:216)  [comm    ] [out][3,"689c5392-9336-41ba-bb26-b55470e629fd",{"status":"Accepted"}]
2024-Jan-09 15:47:54.775972 BBOC  (BackOfficeCommu:164)  [info    ] connector_effective_operative_status_changed_callback: evse_id: 1 connector_id: 1 new_status: Operative
2024-Jan-09 15:47:54.778600 BBOC  (BackOfficeCommu:216)  [comm    ] [out][2,"55b4aab5-c698-48fc-b35f-824b34975b50","StatusNotification",{"connectorId":1,"connectorStatus":"Available","evseId":1,"timestamp":"2024-01-09T15:47:54.778Z"}]
2024-Jan-09 15:47:54.815825 BBOC  (BackOfficeCommu:219)  [comm    ] [in ][3,"55b4aab5-c698-48fc-b35f-824b34975b50",{}]
2024-Jan-09 15:47:54.826826 BBOC  (TransactionHand:299)  [info    ] Connector #1: : Change: 160 ---> 1

hikinggrass · 2024-01-17T22:15:44Z

include/ocpp/v201/component_state_manager.hpp

+    /// Note: It is expected that ComponentStateManager::trigger_all_effective_availability_changed_callbacks is called
+    /// on boot, and ComponentStateManager::send_status_notification_all_connectors is called when first connected to
+    /// the CSMS. \param evse_connector_structure Maps each EVSE ID to the number of connectors the EVSE has \param
+    /// db_handler A shared reference to the persistent database \param send_connector_status_notification_callback The


Formatting of these \param entries is quite hard to read

* Introduced component_state_managager which stores and monitors operational/effective states of the CS, EVSEs, and connectors * Introduced three callbacks cs_effective_operative_status_changed_callback, evse_effective_operative_status_changed_callback, connector_effective_operative_status_changed_callback that are called in case of operative state changes for each component * extended ocpp201 database_handler to store operational states persistently * libocpp user is required to call on_unavailable and on_enabled for connectors as a result of operative_status_changed_callback(s). Signed-off-by: pietfried <[email protected]> Signed-off-by: Valentin DImov <[email protected]>

…_status in send_status_notification_single_connector_internal * applied same change in initialize_reported_state_cache * removed unused function connector_status_to_operational_status * fixed CMakeLists.txt in test dir * removed obsolete test cases Signed-off-by: pietfried <[email protected]>

valentin-dimov force-pushed the vd-availability-tracking-refactor branch from 7e790a9 to 8d7b0c4 Compare December 7, 2023 08:44

valentin-dimov mentioned this pull request Dec 7, 2023

OCPP201: Fix availability tracking for CS, EVSEs, and Connectors EVerest/everest-core#459

Merged

valentin-dimov marked this pull request as ready for review December 7, 2023 09:53

valentin-dimov requested review from hikinggrass, marcemmers and maaikez December 7, 2023 09:53

valentin-dimov force-pushed the vd-availability-tracking-refactor branch from 94337ef to 6f315ab Compare December 13, 2023 18:15

klemmpnx reviewed Dec 13, 2023

View reviewed changes

valentin-dimov force-pushed the vd-availability-tracking-refactor branch from fe11475 to d1e02ac Compare December 14, 2023 13:21

klemmpnx reviewed Dec 14, 2023

View reviewed changes

valentin-dimov force-pushed the vd-availability-tracking-refactor branch from 8c74f78 to ff9ca60 Compare December 15, 2023 15:02

valentin-dimov requested a review from klemmpnx December 18, 2023 09:28

valentin-dimov commented Dec 19, 2023

View reviewed changes

valentin-dimov assigned hikinggrass Dec 19, 2023

Pietfried requested a review from rinzevdwalAlfen January 8, 2024 10:11

Pietfried mentioned this pull request Jan 13, 2024

Sync Operative state to connector states. #400

Closed

hikinggrass reviewed Jan 17, 2024

View reviewed changes

hikinggrass approved these changes Jan 17, 2024

View reviewed changes

Pietfried force-pushed the vd-availability-tracking-refactor branch from 5ebf6a7 to 94edef1 Compare January 18, 2024 09:14

Pietfried force-pushed the vd-availability-tracking-refactor branch 2 times, most recently from 3cbb6f5 to 5fface8 Compare January 18, 2024 20:38

Pietfried force-pushed the vd-availability-tracking-refactor branch from 5fface8 to 468bedb Compare January 18, 2024 21:08

Pietfried merged commit 536ec6e into main Jan 18, 2024
3 of 4 checks passed

Pietfried deleted the vd-availability-tracking-refactor branch January 18, 2024 21:19

christopher-davis-afs mentioned this pull request Jan 26, 2024

Evse constructor is improperly documented #422

Closed

hikinggrass mentioned this pull request Jan 31, 2024

Add EnergyTransfer to StartSessionReason, and set Charging state EVerest/everest-core#494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: OCPP 2.0.1 CS, EVSE, and Connector availability tracking #292

Fix: OCPP 2.0.1 CS, EVSE, and Connector availability tracking #292

valentin-dimov commented Dec 6, 2023 •

edited

Loading

valentin-dimov commented Dec 7, 2023

rinzevdwalAlfen commented Dec 11, 2023

rinzevdwalAlfen commented Dec 11, 2023

valentin-dimov commented Dec 11, 2023

klemmpnx left a comment

klemmpnx Dec 13, 2023

valentin-dimov Dec 14, 2023

klemmpnx Dec 13, 2023

valentin-dimov Dec 14, 2023

klemmpnx Dec 13, 2023

valentin-dimov Dec 14, 2023

klemmpnx Dec 13, 2023

valentin-dimov Dec 15, 2023

klemmpnx left a comment

valentin-dimov Dec 19, 2023

valentin-dimov Dec 19, 2023

klemmpnx commented Dec 19, 2023

rinzevdwalAlfen commented Jan 9, 2024

hikinggrass Jan 17, 2024

		insert_stmt.bind_int("@evse_id", evse_id.value_or(0));
		insert_stmt.bind_int("@connector_id", connector_id.value_or(0));

		OperationalStatusEnum DatabaseHandler::get_availability(const std::optional<int32_t> evse_id,
		std::optional<int32_t> connector_id) {

Fix: OCPP 2.0.1 CS, EVSE, and Connector availability tracking #292

Fix: OCPP 2.0.1 CS, EVSE, and Connector availability tracking #292

Conversation

valentin-dimov commented Dec 6, 2023 • edited Loading

valentin-dimov commented Dec 7, 2023

rinzevdwalAlfen commented Dec 11, 2023

rinzevdwalAlfen commented Dec 11, 2023

valentin-dimov commented Dec 11, 2023

klemmpnx 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

klemmpnx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klemmpnx commented Dec 19, 2023

rinzevdwalAlfen commented Jan 9, 2024

Choose a reason for hiding this comment

valentin-dimov commented Dec 6, 2023 •

edited

Loading