Multiplexer module for ISO communication #776

corneliusclaussen · 2024-07-12T10:38:50Z

This module starts and SDP server and waits
for incoming V2G connections (TCP, and optionally TLS). Upon connection, it reads the SupportedAppProtocol.req message to determine if the EV supports ISO-20 or not.

If ISO-20 is supported, it transparently bridges the connection to the ISO-20 module (using TLS->TCP).

If the EV does not support ISO-20, it bridges to the ISO-2 module instead.

Both the required iso-2 and iso-20 modules only need TCP enabled, but they require to accept e.g. PnC on TCP sessions (as the actual communication to the EV may be TLS).
Also the need to listen on localhost (::1) instead of the actual PLC device.

Describe your changes

Issue ticket number and link

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

corneliusclaussen · 2024-07-12T10:49:20Z

What's missing: D20 module needs support for ::1 TCP connections via option, both modules need to allow PnC via option on TCP connection. Config files to be updated to actually use D20 module once it is merged.

a-w50

Stopping reviewing here as most of the code base is copied from the EvseV2G module (and therefore this would be more like a review for that code base). Nice to see that the proxying can be implemented that easy via the connection_proxy functions.
I would recommend for the next time, when copying source files, that they are checked in with their initial/original content in a first commit - so it would be possible to track changes.

a-w50 · 2024-07-29T07:44:06Z

modules/IsoMux/IsoMux.cpp

+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2022-2023 chargebyte GmbH
+// Copyright (C) 2022-2023 Contributors to EVerest
+#include "IsoMux.hpp"


blank line after main header file

a-w50 · 2024-07-29T07:44:49Z

modules/IsoMux/IsoMux.cpp

+
+void IsoMux::init() {
+    /* create v2g context */
+    v2g_ctx = v2g_ctx_create(&(*r_security));


r_security.get()

a-w50 · 2024-07-29T07:49:31Z

modules/IsoMux/CMakeLists.txt

+)
+target_sources(${MODULE_NAME}
+    PRIVATE
+        "connection/tls_connection.cpp"


move to upper target_sources

a-w50 · 2024-07-29T07:52:19Z

modules/IsoMux/CMakeLists.txt

+find_package(PkgConfig REQUIRED)
+
+target_include_directories(${MODULE_NAME} PRIVATE
+    crypto


where is this crypto directory?

wouldn't include connection here, #include "connection/connection.hpp" should be more explicit

a-w50 · 2024-07-29T07:54:05Z

modules/IsoMux/CMakeLists.txt

+    connection
+)
+
+target_link_libraries(${MODULE_NAME} PUBLIC -lpthread)


would prefer cmakes Threads::Threads if its enough

a-w50 · 2024-07-29T08:11:45Z

modules/IsoMux/charger/ISO15118_chargerImpl.cpp

+    mod->r_iso2->subscribe_Require_Auth_EIM([this]() {
+        if (not mod->selected_iso20()) {
+            publish_Require_Auth_EIM(nullptr);
+        }
+    });
+    mod->r_iso20->subscribe_Require_Auth_EIM([this]() {
+        if (mod->selected_iso20()) {
+            publish_Require_Auth_EIM(nullptr);
+        }
+    });
+
+    mod->r_iso2->subscribe_Require_Auth_PnC([this](const auto value) {
+        if (not mod->selected_iso20()) {
+            publish_Require_Auth_PnC(value);
+        }
+    });
+    mod->r_iso20->subscribe_Require_Auth_PnC([this](const auto value) {
+        if (mod->selected_iso20()) {
+            publish_Require_Auth_PnC(value);
+        }
+    });


Would sort all these subscriptions by mod->selected_iso20() and not mod->selected_iso20() for better overview. Furthermore, currently as the subscribers don't do anything else it would be probably enough to have one if mod->selected_iso2() block within which you do the subscriptions and the same for the other case

a-w50 · 2024-07-29T08:12:13Z

modules/IsoMux/charger/ISO15118_chargerImpl.cpp

+        }
+    });
+
+    mod->r_iso2->subscribe_Require_Auth_PnC([this](const auto value) {


Probably const auto& value

a-w50 · 2024-07-29T08:31:07Z

modules/IsoMux/charger/ISO15118_chargerImpl.cpp

+void ISO15118_chargerImpl::handle_set_charging_parameters(
+    types::iso15118_charger::SetupPhysicalValues& physical_values) {
+    mod->r_iso20->call_set_charging_parameters(physical_values);
+    mod->r_iso2->call_set_charging_parameters(physical_values);
+}


It is not easy to reason here, when information needs to be forwarded to both modules and when not.
For the cases where only one module gets informed, it might be easier to have a pointer like active_iso_module which points either to r_iso2 or r_iso20 instead of checking selected_iso20 all the time and making this variable probably obsolete

a-w50 · 2024-07-29T08:54:30Z

modules/IsoMux/connection/tls_connection.cpp

+namespace {
+
+void process_connection_thread(std::shared_ptr<tls::ServerConnection> con, struct v2g_context* ctx) {
+    assert(con != nullptr);


might be question of style, but asserting pointers can also be done simply by assert(con)

a-w50 · 2024-07-31T05:54:59Z

modules/IsoMux/v2g_server.cpp

+             proto_ns, app_proto->VersionNumberMajor, app_proto->VersionNumberMinor, app_proto->SchemaID,
+             app_proto->Priority);
+
+        free(proto_ns);


This needs to be done after strncmp.

corneliusclaussen · 2024-08-02T09:51:59Z

Stopping reviewing here as most of the code base is copied from the EvseV2G module (and therefore this would be more like a review for that code base). Nice to see that the proxying can be implemented that easy via the connection_proxy functions. I would recommend for the next time, when copying source files, that they are checked in with their initial/original content in a first commit - so it would be possible to track changes.

Thanks for your review, I'm not sure if it makes sense to fix the issues that are copy past from the EvseV2G module as it will make it harder to merge fixes done to EvseV2G into this module. This module is also just an intermediate solution until the D20 module supports DIN and ISO-2 as well.

This module starts and SDP server and waits for incoming V2G connections (TCP, and optionally TLS). Upon connection, it reads the SupportedAppProtocol.req message to determine if the EV supports ISO-20 or not. If ISO-20 is supported, it transparently bridges the connection to the ISO-20 module (using TLS->TCP). If the EV does not support ISO-20, it bridges to the ISO-2 module instead. Both the required iso-2 and iso-20 modules only need TCP enabled, but they require to accept e.g. PnC on TCP sessions (as the actual communication to the EV may be TLS). Also the need to listen on localhost (::1) instead of the actual PLC device. Signed-off-by: Cornelius Claussen <[email protected]>

corneliusclaussen force-pushed the feature/iso-mux branch from e4803a2 to 02ea455 Compare July 12, 2024 10:48

Pietfried assigned SebaLukas Jul 25, 2024

a-w50 reviewed Jul 31, 2024

View reviewed changes

corneliusclaussen force-pushed the feature/iso-mux branch from 02ea455 to 5af54a7 Compare August 2, 2024 09:52

SebaLukas mentioned this pull request Aug 2, 2024

Feat: Adding initial iso mux support EVerest/libiso15118#28

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiplexer module for ISO communication #776

Multiplexer module for ISO communication #776

corneliusclaussen commented Jul 12, 2024

corneliusclaussen commented Jul 12, 2024

a-w50 left a comment

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 29, 2024

a-w50 Jul 31, 2024

corneliusclaussen commented Aug 2, 2024

Multiplexer module for ISO communication #776

Are you sure you want to change the base?

Multiplexer module for ISO communication #776

Conversation

corneliusclaussen commented Jul 12, 2024

Describe your changes

Issue ticket number and link

Checklist before requesting a review

corneliusclaussen commented Jul 12, 2024

a-w50 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

corneliusclaussen commented Aug 2, 2024