-
Notifications
You must be signed in to change notification settings - Fork 68
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
Changes from 37 commits
2cb49ca
9b685ae
d9dddf9
614941b
4b4a8b8
f21faab
0fac3f8
47de4e6
30d8528
eece9af
b9d209e
8b4d24d
5ca81e0
a3b68aa
2f951d4
3c6143c
9e63a6a
31f0be3
bebc386
dccd1be
7107404
f445eeb
e38ff04
7c050bd
c72ecc9
383bafd
758a8ea
6175f1f
d3936df
4370816
2d6364a
8b01468
466db6b
9ce48c0
3e58116
9b4e478
fbe124c
e867876
986b979
4278be8
71ac033
1c59a3e
d5998cf
040a99b
bf290f5
e1d7315
ebaa421
1fee071
691178f
9d89744
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 log level for this plugin" | ||
}, | ||
{ | ||
"key": "UnitId", | ||
"default": "v2xhub_id", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 log level for this plugin" | ||
}, | ||
{ | ||
"key": "UnitName", | ||
"default": "East Intersection", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed 1c59a3e There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": "The log level for this plugin" | ||
}, | ||
{ | ||
"key": "UnitType", | ||
"default": "Infrastructure", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed e867876 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed d5998cf |
||
"description": "The log level for this plugin" | ||
}, | ||
{ | ||
"key": "TopicExclusionList", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are not topics but messages There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
## NATS Publisher/Subscriber | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this file called README.md instead of readme.md There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
### NATS Connections and registration | ||
#### Telematic plugin sends registration request to telematic server | ||
##### 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" | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ namespace TelematicBridge | |
{ | ||
TelematicBridgePlugin::TelematicBridgePlugin(const string &name) : PluginClient(name) | ||
{ | ||
_telematicUnitPtr = make_shared<TelematicUnit>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be a unique pt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does the plugin need a reference to this pointer? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paulbourelly999 Addressed 040a99b |
||
UpdateConfigSettings(); | ||
AddMessageFilter("*", "*", IvpMsgFlags_None); | ||
AddMessageFilter("J2735", "*", IvpMsgFlags_RouteDSRC); | ||
SubscribeToMessages(); | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,21 +3,30 @@ | |
|
||
#include "PluginClient.h" | ||
#include "TelematicBridgeMsgWorker.h" | ||
#include "TelematicUnit.h" | ||
|
||
using namespace tmx::utils; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using namespace is bad practice especially in header files. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect descriptions for all configuration parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed e867876