Telematic bridge nats #567

dan-du-car · 2023-11-22T19:00:09Z

PR Details

Description

This is the implementation story for the telematic bridge to stream J2735 data from V2xHub into the cloud at near real time. The module design refers to https://usdot-carma.atlassian.net/wiki/spaces/WFD2/pages/2640379977/V2xHub+WFD+Bridge+Design The following functionalities should be implemented for this module:

Able to forward the JSON message to telematic cloud when the message type is requested by end users through telematic UI.

Able to register itself with WFD cloud services.

Able to map each TMX message type to each individual topic.

Able to provide available topics to telematic cloud.

Able to stream the requested TMX message in JSON format to telematic cloud.

Related Issue

#591
WDF-447

Motivation and Context

Telematic tool data collection

How Has This Been Tested?

Unit test and local integration testing

Types of changes

Defect fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that cause existing functionality to change)

Checklist:

I have added any new packages to the sonar-scanner.properties file
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
V2XHUB Contributing Guide
I have added tests to cover my changes.
All new and existing tests passed.

paulbourelly999 · 2023-11-28T18:44:23Z

src/v2i-hub/TelematicBridgePlugin/manifest.json

+        },
+        {
+            "key": "UnitId",
+            "default": "v2xhub_id",


We are currently, in CDASim pulling and infrastructure ID from an environment variable. You may want to do the same here. See CDASimAdapter

This is not the same concept as the infrastructure id. It is a unique identifier we give to a telematic unit to distinguish it from other units like streets and vehicle. Is there any rule when to use the UI configuration parameter and when to use the environment variables? All these parameters make sense when they are together.

Hmm, so part of the UC for this plugin is to disable health of different RSUs, and therefore infrastructure instances. Coming up with and ID that is completely different from the J2735 intersection ID and our infrastructure ID I think is bad practice. Then we will have 3 different IDs for the same instance, all which are completely indepedent.

I think this unit id value can be anything that makes sense to end user during their testing with v2xhub as long as it is unique in the telematic tool.

So then why don't we just re-use INFRASTRUCTURE ID here? The same environment variable we use for CDASimAdapter.

What is the value of the infrastructure id? We also display this ID in the UI. Does it make sense to end users to use infrastructure id like 1079? Or use a more descriptive unit id, like east_intersection_1079 or v2xhub_1079?

Something like <rsu_21312>. We have not standardized this yet but I think if we say it has to be rsu_ then maybe we can minimize different ID

The unit id is under the telematic tool context. They may share the same value. For debugging and ease of usability purpose, it might be more obvious if we keep both unit name and unit id in the same place like here on the V2xHub UI.

paulbourelly999 · 2023-11-28T18:45:07Z

src/v2i-hub/TelematicBridgePlugin/manifest.json

+        },
+        {
+            "key": "UnitType",
+            "default": "Infrastructure",


Not sure about this field.

Addressed e867876

UnitType does not make sense as a configurable parameter if it is sent to infrastructre. Every V2X-Hub falls under infrastructure. Maybe if we have types like intersection or highway it would make sense but like this is does not make sense.

This is a category defined by telematic tool. It only has two categories platform and infrastructure. Carma-streets, carma-cloud, and v2xhub belong to infrastructure.

So then lets make this a constant and not configurable since there is no scenario in which we would have V2X-Hub have any other type.

Addressed d5998cf

paulbourelly999 · 2023-11-28T18:45:30Z

src/v2i-hub/TelematicBridgePlugin/manifest.json

+            "description": "The log level for this plugin"            
+        },
+        {
+            "key": "TopicExclusionList",


These are not topics but messages

The topic name consist of type_subtype_source. We create a topic for each message because v2xhub does not have a topic. For telematic users to select topics, we come up with this combination for each type of message.

So topics is not a familiar term for users of V2X-HUb. If we are making a connection between topics and message in telematics, I would recommend we use "message" to refer to the messages we are listening to in V2X-Hub and "topic" as the term of the topics we will create on telematics.

I suggest we call this messages here and the term topics is only used to send information to telematics about topics to create and in the telematics UI/Software.

Addressed 71ac033

paulbourelly999 · 2023-11-28T18:45:56Z

src/v2i-hub/TelematicBridgePlugin/readme.md

@@ -0,0 +1,94 @@
+## NATS Publisher/Subscriber


Can we make this file called README.md instead of readme.md

paulbourelly999 · 2023-11-28T18:46:43Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicBridgePlugin.cpp

@@ -4,6 +4,8 @@ namespace TelematicBridge
 {
    TelematicBridgePlugin::TelematicBridgePlugin(const string &name) : PluginClient(name)
    {
+        _telematicUnitPtr = make_shared<TelematicUnit>();


Can this be a unique pt?

Is there a reason for this change?

Yes, it is best practice to limit pointer scope or reference scope where possible to avoid other users possibly unintentionally impacting the lifecycle of this pointer by holding a reference.

This point is shared with this plugin. I cannot find a case where users will accidentally change the lifecycle of this pointer. This pointer should be sharing the same lifecycle of the plugin itself.

If the plugin has a reference to this object but the telematics bridge is no longer in scope, the telematics bridge will still not get deleted because the plugin holds a reference to this pointer which will prevent the pointer from getting deleted

Why does the plugin need a reference to this pointer?

So that the plugin can use this pointer to send data into the telematic tool. It does not make sense for this plugin to subscribe messages but not forward them into the telematic tool.

@paulbourelly999 Addressed 040a99b

paulbourelly999 · 2023-11-28T19:04:01Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicUnit.h

+        static CONSTEXPR const char *TOPIC_NAME = "topic_name";               // topic_name key used to find topic_name value from JSON
+        static CONSTEXPR const char *TIMESTAMP = "timestamp";                 // timestamp key used to find timestamp value from JSON
+        static CONSTEXPR const char *PAYLOAD = "payload";                     // payload key used to find payload value from JSON
+        static CONSTEXPR const char *TOPICS = "topics";                       // topics key used to find topics value from JSON


These are not topics, I would refrain from using this language unless telematics requires it.

I can change the variable name "TOPICS". We do need the value topics to find JSON value that match this topics key.

paulbourelly999 · 2023-11-29T14:41:58Z

src/v2i-hub/TelematicBridgePlugin/REAADME.md

@@ -0,0 +1,94 @@
+## NATS Publisher/Subscriber
+### NATS Connections and registration


REAME file is misspelled

Addressed 71ac033

paulbourelly999 · 2023-11-29T14:42:23Z

src/v2i-hub/TelematicBridgePlugin/REAADME.md

@@ -0,0 +1,94 @@
+## NATS Publisher/Subscriber
+### NATS Connections and registration
+#### Telematic plugin sends registration request to telematic server


Please update to include an overall plugin functional/usage description

Addressed 71ac033

paulbourelly999 · 2023-11-29T14:50:23Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicBridgePlugin.h

@@ -3,21 +3,30 @@

 #include "PluginClient.h"
 #include "TelematicBridgeMsgWorker.h"
+#include "TelematicUnit.h"

 using namespace tmx::utils;


Using namespace is bad practice especially in header files.

Addressed 71ac033

paulbourelly999 · 2023-11-29T14:53:41Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicUnit.cpp

+            }
+            else
+            {
+                throw TelematicBridgeException(natsStatus_GetText(s));


Why do this while loop and throw and exception. If the registration fails you throw an exception so the loop is exited. If you are successful the loop is exited. Is there any condition in which you will retry. Only if NAT_OK and the validateRegisterStatus fails? In which situation would this occur.
\

Only if we get an invalid registration reply json from telematics?

The server can return response without an event information. In this case, this bridge continues to attempt to register. It throw error when NATS_OK check fails. This usually because the server is down.

Is the response without event information also a bug or error behavior or would reattempts ever fix that. Also do we want to limit connection attempts on invalid responses?

It is neither of them. It requires user attention to double check the event and its assigned unit.

I think we want to wait for the valid response, otherwise there is no point keeping this plugin running.

So if user input is required to make a valid response, then we should indicate to the user that something is wrong instead of silently indefinitely and continuously retrying a connection that has an issue that a user must resolve.

To me either we do a single register attempt and fail or succeed and notify the user on fails, or we have a loop that attempts the connection some finite amount of times before failing. To me reattempts on connections should ideally be used for connection issues that do not require user input to be resolved, like if the server is restarting or if there are network temporary issues.

If the concern here is uptime of the resulting plugin, maybe this registration logic needs to be more complex and have some kind of failure recovery logic but this does not seem to me to satisfy that requirement

@paulbourelly999 Addressed 040a99b

@paulbourelly999 Addressed 040a99b

paulbourelly999 · 2023-11-29T14:56:54Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicUnit.cpp

+        message[LOCATION_KEY] = eventLocation;
+        message[TESTING_TYPE_KEY] = testingType;
+        message[EVENT_NAME_KEY] = eventName;
+        message[TIMESTAMP_KEY] = duration_cast<milliseconds>(system_clock::now().time_since_epoch()).count();


If you do not extend PluginClientClockAware and use the carma-clock object this plugin will not support simulation.

That is a different case. We will evaluate and address this later when we integrate this with simulation.

I think that is fine for now, but for future reference, every new plugin created should extend PluginClientClockAware. It is no additional work and it avoids having to integrated with simulation in the future.

paulbourelly999 · 2023-11-29T15:25:58Z

src/v2i-hub/TelematicBridgePlugin/manifest.json

+        },
+        {
+            "key": "UnitName",
+            "default": "East Intersection",


Can this match J2735 intersection name or does it need to be separate. If it can match I suggest we just add in the description to set this to intersection name

Addressed 1c59a3e

Please specify J2735 MAP in the description. So should be set to MAP J2735 intersection name if available.

This is not specific about MAP message nor intersection. If we deploy this v2xhub in Cav-in-a-box at roadside, there is no concept of intersection. This east intersection is just a default value I give cause I test it with east intersection.

I know this is not MAP Message specific from an implementation stand point but we should not be creating unnecessary new IDs in our different pieces of functionality. For example INFRASTRUCTURE_ID is a unique string used for registration for simulation but if we can reuse the ID for say telematics or if we could relate that ID to the unique J2735 Intersection ID we decrease the number of separately managed ID parameters. This way they all relate to each other.

Can we table this for now? The unit id is something we used in the telematic tool context. The same design for this unit id is applied to carma-streets, carma-platform (vehicle), and carma-cloud. If this has some conflicts with IDs in a simulated environment, we probably want to decide and make the changes to all units to better fit the simulated environment.

Do we have unique IDs for carma streets, carma-cloud , carma-platform as well to register for the simulation?

paulbourelly999

Left some more comments do be address

paulbourelly999 · 2023-11-29T18:18:22Z

src/v2i-hub/TelematicBridgePlugin/manifest.json

+        },
+        {
+            "key": "UnitName",
+            "default": "East Intersection",


Please specify J2735 MAP in the description. So should be set to MAP J2735 intersection name if available.

paulbourelly999 · 2023-11-29T18:20:02Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicBridgePlugin.cpp

@@ -4,6 +4,8 @@ namespace TelematicBridge
 {
    TelematicBridgePlugin::TelematicBridgePlugin(const string &name) : PluginClient(name)
    {
+        _telematicUnitPtr = make_shared<TelematicUnit>();


If the plugin has a reference to this object but the telematics bridge is no longer in scope, the telematics bridge will still not get deleted because the plugin holds a reference to this pointer which will prevent the pointer from getting deleted

paulbourelly999 · 2023-11-29T18:20:22Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicBridgePlugin.cpp

@@ -4,6 +4,8 @@ namespace TelematicBridge
 {
    TelematicBridgePlugin::TelematicBridgePlugin(const string &name) : PluginClient(name)
    {
+        _telematicUnitPtr = make_shared<TelematicUnit>();


Why does the plugin need a reference to this pointer?

paulbourelly999 · 2023-11-29T18:24:53Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicUnit.cpp

+            }
+            else
+            {
+                throw TelematicBridgeException(natsStatus_GetText(s));


So if user input is required to make a valid response, then we should indicate to the user that something is wrong instead of silently indefinitely and continuously retrying a connection that has an issue that a user must resolve.

To me either we do a single register attempt and fail or succeed and notify the user on fails, or we have a loop that attempts the connection some finite amount of times before failing. To me reattempts on connections should ideally be used for connection issues that do not require user input to be resolved, like if the server is restarting or if there are network temporary issues.

paulbourelly999 · 2023-11-29T18:29:05Z

src/v2i-hub/TelematicBridgePlugin/src/TelematicUnit.cpp

+            }
+            else
+            {
+                throw TelematicBridgeException(natsStatus_GetText(s));


If the concern here is uptime of the resulting plugin, maybe this registration logic needs to be more complex and have some kind of failure recovery logic but this does not seem to me to satisfy that requirement

@paulbourelly999 Addressed 040a99b

dan-du-car added 30 commits November 16, 2023 19:15

init

2cb49ca

update sonar cloud

9b685ae

fix unit test failure

d9dddf9

update code coverage

614941b

update code coverage

4b4a8b8

address code smell

f21faab

update coverage

0fac3f8

update

47de4e6

update

30d8528

flat json

eece9af

update unit test

b9d209e

code smell

8b4d24d

address comments

5ca81e0

address comments

a3b68aa

unnecessary log

2f951d4

init

3c6143c

address comments

9e63a6a

address comments

31f0be3

Merge branch 'telematic_bridge' into telematic_bridge_nats

bebc386

update

dccd1be

fix register unit

7107404

add exclusion list

f445eeb

clean code

e38ff04

resolve conflict

7c050bd

resolve conflict

c72ecc9

add unit test

383bafd

update unit test

758a8ea

code coverage

6175f1f

code coverage

d3936df

code coverage

4370816

paulbourelly999 reviewed Nov 28, 2023

View reviewed changes

dan-du-car added 3 commits November 28, 2023 19:30

address comments

e867876

address comments

986b979

address comments

4278be8

paulbourelly999 reviewed Nov 29, 2023

View reviewed changes

address comments

71ac033

paulbourelly999 reviewed Nov 29, 2023

View reviewed changes

dan-du-car added 2 commits November 29, 2023 15:29

address comments

1c59a3e

address comments

d5998cf

paulbourelly999 requested changes Nov 29, 2023

View reviewed changes

dan-du-car added 7 commits November 29, 2023 19:30

address comments

040a99b

address comments

bf290f5

address comments

e1d7315

address comments

ebaa421

address comments

1fee071

address comments

691178f

address comments

9d89744

paulbourelly999 approved these changes Nov 30, 2023

View reviewed changes

paulbourelly999 merged commit 6b604b7 into develop Nov 30, 2023
9 checks passed

paulbourelly999 deleted the telematic_bridge_nats branch November 30, 2023 19:36

		@@ -0,0 +1,94 @@
		## NATS Publisher/Subscriber
		### NATS Connections and registration

Telematic bridge nats #567

Telematic bridge nats #567

Conversation

dan-du-car commented Nov 22, 2023 • edited by paulbourelly999 Loading

PR Details

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

dan-du-car Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbourelly999 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

dan-du-car Nov 29, 2023 • edited Loading

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

dan-du-car Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

paulbourelly999 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-du-car Nov 28, 2023 • edited Loading

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

dan-du-car Nov 29, 2023 • edited Loading

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

paulbourelly999 Nov 29, 2023 • edited Loading

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

dan-du-car Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-du-car Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbourelly999 Nov 29, 2023 • edited by dan-du-car Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-du-car Nov 29, 2023 • edited Loading

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

dan-du-car Nov 29, 2023 • edited Loading

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

paulbourelly999 left a comment

dan-du-car commented Nov 22, 2023 •

edited by paulbourelly999

Loading

dan-du-car Nov 28, 2023 •

edited

Loading

paulbourelly999 Nov 29, 2023 •

edited

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

paulbourelly999 Nov 29, 2023 •

edited

Loading

dan-du-car Nov 28, 2023 •

edited

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

paulbourelly999 Nov 29, 2023 •

edited

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

paulbourelly999 Nov 29, 2023 •

edited by dan-du-car

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

dan-du-car Nov 29, 2023 •

edited

Loading

paulbourelly999 Nov 29, 2023 •

edited by dan-du-car

Loading