-
Notifications
You must be signed in to change notification settings - Fork 69
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
v2x hub Integration: Implement telematic plugin to subscribe to all TMX messages from V2xHub #565
Conversation
@@ -73,7 +73,7 @@ TEST(PedestrianDetectionForSPAT, updateEncodedSpat) | |||
EXPECT_EQ(spatPtr->intersections.list.array[0]->states.list.array[0]->signalGroup, 1); | |||
EXPECT_EQ(spatPtr->intersections.list.array[0]->states.list.array[1]->signalGroup, 2); | |||
ASSERT_NE(spatPtr->intersections.list.array[0]->maneuverAssistList, nullptr); | |||
EXPECT_EQ(spatPtr->intersections.list.array[0]->maneuverAssistList->list.count, 1); | |||
EXPECT_NE(spatPtr->intersections.list.array[0]->maneuverAssistList->list.count, 0); |
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.
Why this change?
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.
This unit test sometimes failed. https://github.com/usdot-fhwa-OPS/V2X-Hub/actions/runs/6895436333/job/18759375988
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.
I ran the unit test in local and the list count sometimes equals to 1, sometimes equals to 2.
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.
This is a issue. Can we create a bug for this is we do not plan to address it in this PR and leave a TODO comment here outlining what you found.
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.
cmake . -DNATS_BUILD_NO_SPIN=ON | ||
make -j${numCPU} | ||
make install |
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.
After we install this dependency, we should delete the source code.
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 a3b68aa
@@ -31,6 +31,9 @@ DEPENDENCIES="build-essential \ | |||
wget \ | |||
zip \ | |||
zlib1g \ | |||
rapidjson-dev \ |
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.
Is there a reason we need to use rapidjson instead of any of the json libraries we currently have installed in V2X-Hub
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.
I think the xml to json library use the rapidjson lib
@@ -145,7 +145,11 @@ namespace RSUHealthMonitor | |||
} | |||
else if (success) | |||
{ | |||
rsuStatuJson.append(populateJson(config.field, responseVal)); | |||
auto json = populateJson(config.field, responseVal); |
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.
What changed here for the RSU Health Monitor Plugin
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.
The JSON was an array. My intention is to create a JSON object.
find_package(RapidJSON REQUIRED) | ||
|
||
BuildTmxPlugin() | ||
TARGET_LINK_LIBRARIES ( ${PROJECT_NAME} tmxutils jsoncpp) |
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.
if we are not linking rapidjson, what is the point of finding the package?
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.
|
||
namespace TelematicBridge | ||
{ | ||
bool TelematicBridgeMsgWorker::HexToBytes(const string &hexPaylod, vector<char> &byteBuffer) |
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.
What is this method for?
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.
Added comments
|
||
if (decode_rval.code != RC_OK) | ||
{ | ||
erroross.str(""); |
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.
What is this erroross object?
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.
ostringstream
size_t allocated_size; // this is the total size of the buffer. | ||
}; | ||
|
||
class TelematicBridgeMsgWorker |
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.
Does this object have any state information? Is there a reason we need a class for this or could we just define functions here without a class
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 5ca81e0
target_link_libraries(${PROJECT_NAME}_lib PUBLIC tmxutils jsoncpp) | ||
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 ${PROJECT_NAME}_lib gtest) |
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.
If you link the following you do not need to create a main.cpp
file for testing.
target_link_libraries(${PROJECT_NAME}_test PRIVATE ${PROJECT_NAME}_lib GTest::Main
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.
"exeLocation": "/bin/TelematicBridgePlugin", | ||
"coreIpAddr": "127.0.0.1", | ||
"corePort": 24601, | ||
"messageTypes": [], |
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.
Update Message Types here if we want status counts for the messages this plugin handles.
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.
This plugin intents to subscribe to all messages. what exact messageTypes should be list here?
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.
The same as Immediate Forward plugin, messageTypes is empty array.
{ | ||
TelematicBridgePlugin::TelematicBridgePlugin(const string &name) : PluginClient(name) | ||
{ | ||
AddMessageFilter("*", "*", IvpMsgFlags_None); |
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.
Will this only pickup messages with no IvpMsgFlags? I imagine this filter should look essentially identical to the ImmediateForward filter, except it should not require a DSRC flag.
The immediate forward filer looks like this :
AddMessageFilter("J2735", "*", IvpMsgFlags_RouteDSRC);
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.
In our case we probably do not want to subscribe to all tmx messages by default but build in some configuration (similar to Immediate Forward) where we can specify a TMX message PSID that we want to subscribe to.
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.
How do we map the PSID to each TMX message? eg: time sync TMX message, do we have a PSID for that? Which messages use the IvpMsgFlags_RouteDSRC flag? When I test the J2735 message with Message receiver plugin, I can subcribe to the J2735 without IvpMsgFlags_RouteDSRC flag. The intention for this plugin is to discover all messages. So that the plugin can tell telematic system what topics/message available in V2xHub. V2xHub UI does not control what to subscribe to. Instead, the telematic UI will receive a list of all available topics/messages and decide which messages to send to cloud. Note: we will create a mapping between messages and topics as the plugin discover messages with the AddMessageFilter func.
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.
Immediate Forward plugin subscribe to all J2735 message and use IVPMessage rather than routable message.
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.
Although I think it is unnecessary to pull in a new JSON dependency for the Telematics bridge when we already have jsoncpp included, I think it probably not worth the effort now to try and change the logic for the existing code. Some changes that I think are worth while:
Removing any unnecessary classes. If something is not stateful, meaning it does not have a state associated with it, I think we can just have free functions instead of creating an object and having static methods associated with it.
Additionally, I think its worth while reworking the configuration surrounding the messages we want to forward to telematics. I think this should be very similar to how Immediate Forward works in that we should be able to configure what J2735 messages, as well as what TMX messages get forwarded to Telematics.
Lastly, I am not sure I understand why the processing of J2735 messages for this plugin requires so many custom methods, like converting hex string to bytes (etc). I am unsure how different this plugin is from Message Receiver and Immediate forward. Why do we need these methods there, but not here. Considering using different Handle methods. Maybe instead of ivp message we should be dealing with routeable message.
@paulbourelly999 This plugin is very similar to Immediate Forward plugin. The only differences from immediate Forward is that it decodes the J2735 and also subscribes to any other TMX messages other than J2735.
PR Details
Description
In order to publish data from V2xHub to cloud a telematics bridge is needed. This story is the implementation story for the telematic module to collect J2735 data from V2xHub. The design refers to https://usdot-carma.atlassian.net/wiki/spaces/WFD2/pages/2640379977/V2xHub+WFD+Bridge+Design
Connect to V2xHub
Subscribe all TMX messages from V2xhub.
If there are J2735 message payloads in the TMX messages, decode the J2735 into human readable message.
Convert TMX messages into messages in JSON format.
Related Issue
#591
WFD-392
Motivation and Context
CDA telematic
How Has This Been Tested?
Unit test
Integration test
Types of changes
Checklist:
V2XHUB Contributing Guide