Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telematic bridge nats #567

Merged
merged 50 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
2cb49ca
init
dan-du-car Nov 16, 2023
9b685ae
update sonar cloud
dan-du-car Nov 16, 2023
d9dddf9
fix unit test failure
dan-du-car Nov 16, 2023
614941b
update code coverage
dan-du-car Nov 16, 2023
4b4a8b8
update code coverage
dan-du-car Nov 16, 2023
f21faab
address code smell
dan-du-car Nov 16, 2023
0fac3f8
update coverage
dan-du-car Nov 17, 2023
47de4e6
update
dan-du-car Nov 17, 2023
30d8528
update
dan-du-car Nov 17, 2023
eece9af
flat json
dan-du-car Nov 17, 2023
b9d209e
update unit test
dan-du-car Nov 17, 2023
8b4d24d
code smell
dan-du-car Nov 17, 2023
5ca81e0
address comments
dan-du-car Nov 20, 2023
a3b68aa
address comments
dan-du-car Nov 20, 2023
2f951d4
unnecessary log
dan-du-car Nov 20, 2023
3c6143c
init
dan-du-car Nov 21, 2023
9e63a6a
address comments
dan-du-car Nov 21, 2023
31f0be3
address comments
dan-du-car Nov 21, 2023
bebc386
Merge branch 'telematic_bridge' into telematic_bridge_nats
dan-du-car Nov 21, 2023
dccd1be
update
dan-du-car Nov 21, 2023
7107404
fix register unit
dan-du-car Nov 21, 2023
f445eeb
add exclusion list
dan-du-car Nov 21, 2023
e38ff04
clean code
dan-du-car Nov 21, 2023
7c050bd
resolve conflict
dan-du-car Nov 22, 2023
c72ecc9
resolve conflict
dan-du-car Nov 22, 2023
383bafd
add unit test
dan-du-car Nov 22, 2023
758a8ea
update unit test
dan-du-car Nov 22, 2023
6175f1f
code coverage
dan-du-car Nov 22, 2023
d3936df
code coverage
dan-du-car Nov 22, 2023
4370816
code coverage
dan-du-car Nov 22, 2023
2d6364a
code coverage
dan-du-car Nov 22, 2023
8b01468
code coverage
dan-du-car Nov 22, 2023
466db6b
code coverage
dan-du-car Nov 22, 2023
9ce48c0
add more log
dan-du-car Nov 27, 2023
3e58116
convert timestamp from mill to micro
dan-du-car Nov 27, 2023
9b4e478
update unit test
dan-du-car Nov 27, 2023
fbe124c
update unit test
dan-du-car Nov 27, 2023
e867876
address comments
dan-du-car Nov 28, 2023
986b979
address comments
dan-du-car Nov 29, 2023
4278be8
address comments
dan-du-car Nov 29, 2023
71ac033
address comments
dan-du-car Nov 29, 2023
1c59a3e
address comments
dan-du-car Nov 29, 2023
d5998cf
address comments
dan-du-car Nov 29, 2023
040a99b
address comments
dan-du-car Nov 29, 2023
bf290f5
address comments
dan-du-car Nov 30, 2023
e1d7315
address comments
dan-du-car Nov 30, 2023
ebaa421
address comments
dan-du-car Nov 30, 2023
1fee071
address comments
dan-du-car Nov 30, 2023
691178f
address comments
dan-du-car Nov 30, 2023
9d89744
address comments
dan-du-car Nov 30, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/v2i-hub/TelematicBridgePlugin/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ PROJECT (TelematicBridgePlugin VERSION 7.5.1 LANGUAGES CXX)
set (TMX_PLUGIN_NAME "Telematic Bridge")

BuildTmxPlugin()
TARGET_LINK_LIBRARIES ( ${PROJECT_NAME} tmxutils jsoncpp)
TARGET_LINK_LIBRARIES ( ${PROJECT_NAME} tmxutils jsoncpp nats)

####################################################
################## Testing #######################
####################################################
add_library(${PROJECT_NAME}_lib src/TelematicUnit.cpp)
target_link_libraries(${PROJECT_NAME}_lib PUBLIC tmxutils jsoncpp nats)
enable_testing()
include_directories(${PROJECT_SOURCE_DIR}/src)
file(GLOB_RECURSE TEST_SOURCES LIST_DIRECTORIES false test/*.h test/*.cpp)
add_executable(${PROJECT_NAME}_test ${TEST_SOURCES})
target_link_libraries(${PROJECT_NAME}_test PRIVATE gtest tmxutils jsoncpp)
target_link_libraries(${PROJECT_NAME}_test PRIVATE gtest ${PROJECT_NAME}_lib)
94 changes: 94 additions & 0 deletions src/v2i-hub/TelematicBridgePlugin/REAADME.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
## NATS Publisher/Subscriber
### NATS Connections and registration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REAME file is misspelled

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed 71ac033

#### Telematic plugin sends registration request to telematic server
Copy link
Contributor

@paulbourelly999 paulbourelly999 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update to include an overall plugin functional/usage description

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed 71ac033

##### Subject
NATS subject: *.register_unit
##### Request
```
{
"unit_id": "<unit_id>"
}
```
##### Response
```
{
"unit_id": "<unit_id>",
"unit_type": "infrastructure",
"unit_name": "East Intersection",
"timestamp": "1678998191815233965",
"event_name": "wfd_integration_testing",
"location": "TFHRC",
"testing_type": "Integration"
}
```

### Available topics
#### Telematic plugin receives request from telematic UI
##### Subject
NATS subject: <unit_id>.available_topics
##### Request
```
{
"unit_id": "<unit_id>"
}
```
##### Reply

```
{
"unit_id": "<unit_id>",
"unit_type": "infrastructure",
"unit_name": "East Intersection",
"timestamp": "1678998191815233965",
"event_name": "wfd_integration_testing",
"location": "TFHRC",
"testing_type": "Integration",
"topics": [
{
"name": "J2735_TMSG03-P_CARMAStreetsPlugin"
},
{
"name": "<topic_name_2>"
}
]
}
```

### Selected topics
#### Telematic plugin receives selected topics from telematic UI
##### Subject
NATS subject: <unit_id>.publish_topics
#### Request
```
{
"unit_id": "<unit_id>",
"unit_type": "infrastructure",
"timestamp": 1663084528513000400,
"event_name": "wfd_integration_testing",
"location": "TFHRC",
"testing_type": "Integration",
"topics": [
"<topic_name_1>",
"<topic_name_2>"
]
}
```
##### Reply
```
"request received!"
```

## Check status
#### Telematic plugin receives live status request from telematic server
##### Subject
NATS subject: <unit_id>.check_status
##### Request
```
{
"unit_id": "<unit_id>"
}
```
##### Reponse
```
"OK"
```
25 changes: 25 additions & 0 deletions src/v2i-hub/TelematicBridgePlugin/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,31 @@
"key": "LogLevel",
"default": "INFO",
"description": "The log level for this plugin"
},
{
"key": "NATSUrl",
"default": "nats://127.0.0.1:4222",
"description": "The NATS connection URL"
},
{
"key": "UnitId",
"default": "v2xhub_id",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@dan-du-car dan-du-car Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@paulbourelly999 paulbourelly999 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@dan-du-car dan-du-car Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"description": "The unique identifier for a telematic unit"
},
{
"key": "UnitName",
"default": "East Intersection",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed 1c59a3e

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@dan-du-car dan-du-car Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"description": "A descriptive name for each telematic unit"
},
{
"key": "UnitType",
"default": "Infrastructure",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed e867876

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@dan-du-car dan-du-car Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@paulbourelly999 paulbourelly999 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed d5998cf

"description": "The type of telematic unit. V2xhub belongs to infrastructure category."
},
{
"key": "TopicExclusionList",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not topics but messages

Copy link
Collaborator Author

@dan-du-car dan-du-car Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed 71ac033

"default": "System_KeepAlive_CommandPlugin,System_KeepAlive_CARMAStreetsPlugin,System_KeepAlive_CDASimAdapter,System_KeepAlive_MessageReceiver",
"description": "The list of topics are excluded from the available topic list. Topic name is a combination of message type, subtype and source separated by underscore. E.G: type_subtype_source"
}
]
}
52 changes: 48 additions & 4 deletions src/v2i-hub/TelematicBridgePlugin/src/TelematicBridgePlugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ namespace TelematicBridge
{
TelematicBridgePlugin::TelematicBridgePlugin(const string &name) : PluginClient(name)
{
_telematicUnitPtr = make_shared<TelematicUnit>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a unique pt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@dan-du-car dan-du-car Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the plugin need a reference to this pointer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateConfigSettings();
AddMessageFilter("*", "*", IvpMsgFlags_None);
AddMessageFilter("J2735", "*", IvpMsgFlags_RouteDSRC);
SubscribeToMessages();
Expand All @@ -20,14 +22,56 @@ namespace TelematicBridge
auto messageFm = (MessageFrame_t *)calloc(1, sizeof(MessageFrame_t));
DecodeJ2735Msg(msg->payload->valuestring, messageFm);
string xml_payload_str = ConvertJ2735FrameToXML(messageFm);
ASN_STRUCT_FREE(asn_DEF_MessageFrame, messageFm);
json["payload"] = StringToJson(xml2Json(xml_payload_str));
ASN_STRUCT_FREE(asn_DEF_MessageFrame, messageFm);
string json_payload_str = xml2Json(xml_payload_str.c_str());
json["payload"] = StringToJson(json_payload_str);
}

auto jsonStr = JsonToString(json);
PLOG(logINFO) << jsonStr;
stringstream topic;
topic << (msg->type ? msg->type : "") << "_" << (msg->subtype ? msg->subtype : "") << "_" << (msg->source ? msg->source : "");
auto topicStr = topic.str();
_telematicUnitPtr->updateAvailableTopics(topicStr);
if (_telematicUnitPtr->inSelectedTopics(topicStr))
{
_telematicUnitPtr->publishMessage(topicStr, json);
}
}
}

void TelematicBridgePlugin::UpdateConfigSettings()
{
lock_guard<mutex> lock(_configMutex);
GetConfigValue<string>("NATSUrl", _natsURL);
GetConfigValue<string>("UnitId", _unitId);
GetConfigValue<string>("UnitName", _unitName);
GetConfigValue<string>("UnitType", _unitType);
GetConfigValue<string>("TopicExclusionList", _excludedTopics);
unit_st unit = {_unitId, _unitName, _unitType};
if (_telematicUnitPtr)
{
_telematicUnitPtr->setUnit(unit);
_telematicUnitPtr->updateExcludedTopics(_excludedTopics);
}
}

void TelematicBridgePlugin::OnStateChange(IvpPluginState state)
{
PluginClient::OnStateChange(state);
if (state == IvpPluginState_registered)
{
UpdateConfigSettings();
if (_telematicUnitPtr)
{
_telematicUnitPtr->connect(_natsURL);
}
}
}

void TelematicBridgePlugin::OnConfigChanged(const char *key, const char *value)
{
PluginClient::OnConfigChanged(key, value);
UpdateConfigSettings();
}
}

// The main entry point for this application.
Expand Down
15 changes: 12 additions & 3 deletions src/v2i-hub/TelematicBridgePlugin/src/TelematicBridgePlugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,30 @@

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

using namespace tmx::utils;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using namespace is bad practice especially in header files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed 71ac033

using namespace std;
namespace TelematicBridge
{


class TelematicBridgePlugin : public tmx::utils::PluginClient
{
private:
static CONSTEXPR const char *Telematic_MSGTYPE_J2735_STRING = "J2735";
shared_ptr<TelematicUnit> _telematicUnitPtr;
string _unitId;
string _unitType;
string _unitName;
string _natsURL;
string _excludedTopics;
mutex _configMutex;
void OnMessageReceived(IvpMessage *msg);

public:
explicit TelematicBridgePlugin(const string& name);
explicit TelematicBridgePlugin(const string &name);
void OnConfigChanged(const char *key, const char *value) override;
void OnStateChange(IvpPluginState state) override;
void UpdateConfigSettings();
~TelematicBridgePlugin() override = default;
};

Expand Down
Loading