-
Notifications
You must be signed in to change notification settings - Fork 67
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
Routable metadata #545
Routable metadata #545
Changes from 4 commits
711080c
cf28cae
63ae453
32b1c05
43c8739
a705342
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 |
---|---|---|
|
@@ -29,9 +29,9 @@ | |
"description":"Port for the incoming message network connection." | ||
}, | ||
{ | ||
"key":"RouteDSRC", | ||
"key":"RouteMessage", | ||
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 renaming this boolean? 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. Only changing the user-facing name. Idea is to remove "DSRC" to avoid confusion with communication hardware used 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 this boolean only for enable route for J2735Message? Some other internal v2xhub message like State, log on the UI are not controlled by this boolean? Maybe narrow down the Message to a specific 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. Nope, just here. It's specific to some logic for broadcasting messages received by this plugin. 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. make sense to me |
||
"default":"false", | ||
"description":"Set the flag to route a received J2735 message." | ||
"description":"Set the flag to route/broadcast a received J2735 message to TMX Core." | ||
}, | ||
{ | ||
"key":"EnableSimulatedBSM", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,7 @@ void MessageReceiverPlugin::OnMessageReceived(routeable_message &msg) | |
BsmEncodedMessage encodedBsm; | ||
SrmEncodedMessage encodedSrm; | ||
|
||
|
||
uint16_t msgPSID = NULL; | ||
|
||
if (msg.get_type() == "Unknown" && msg.get_subtype() == "Unknown") | ||
{ | ||
|
@@ -263,6 +263,7 @@ void MessageReceiverPlugin::OnMessageReceived(routeable_message &msg) | |
} | ||
|
||
sendMsg = encode(encodedBsm, bsm); | ||
msgPSID = 0x20; | ||
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 different messages have different msgPSID? Does a PSID ever change for a message type? I am not sure if we want to hardcode these values for different msgPSID? 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 two I added were under the cases for BSMs and SRMs that are detected by the plugin. Others are kept at null. 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 a enum or struct to consolidate all the PSID? Then use it here in this class? It might be easy to read. 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 don't, but we could make one. What do you think about adding it here? https://github.com/usdot-fhwa-OPS/V2X-Hub/blob/develop/src/tmx/TmxApi/tmx/TmxApiMessages.h 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. yep, I think that is a good location 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. @dan-du-car Changes made |
||
if (!simBSM) return; | ||
} | ||
break; | ||
|
@@ -280,6 +281,7 @@ void MessageReceiverPlugin::OnMessageReceived(routeable_message &msg) | |
ntohl(*((uint32_t*)&(bytes.data()[24]))), | ||
ntohl(*((uint32_t*)&(bytes.data()[28])))); | ||
sendMsg = encode(encodedSrm, srm); | ||
msgPSID = 0xE0000016; | ||
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. There is a code smell on this line about an implicit conversion. Please check this out in the sonar scanner report. 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 |
||
|
||
} | ||
break; | ||
|
@@ -330,6 +332,7 @@ void MessageReceiverPlugin::OnMessageReceived(routeable_message &msg) | |
if (routeDsrc) | ||
{ | ||
sendMsg->set_flags(IvpMsgFlags_RouteDSRC); | ||
sendMsg->addDsrcMetadata(msgPSID); | ||
} | ||
else | ||
{ | ||
|
@@ -345,7 +348,7 @@ void MessageReceiverPlugin::UpdateConfigSettings() | |
lock_guard<mutex> lock(syncLock); | ||
|
||
// Atomic flags | ||
GetConfigValue("RouteDSRC", routeDsrc); | ||
GetConfigValue("RouteMessage", routeDsrc); | ||
GetConfigValue("EnableSimulatedBSM", simBSM); | ||
GetConfigValue("EnableSimulatedSRM", simSRM); | ||
GetConfigValue("EnableSimulatedLocation", simLoc); | ||
|
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 do we need this ${numCPU}? Whese is this env variable from?
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 env is for using the number of cores available on the local machine. This allows for building in parallel. Since we're using Ubuntu as the docker image, this command to get the env variable will always work.
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.
Do you have a link explain how this command "make -j${numCPU}" allow building in parallel. Sorry, I have not seen this before and am curious how this works. Also if we are running this inside a base image ubuntu, how does that get the host machine hardware specs, like num of CPUs?
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.
Yep! Section 5.4 here: https://www.gnu.org/software/make/manual/make.html#Parallel
or run: make --help and you'll see a short description there
Yeah, it's still linked to the host machine, so it's able to find the number of cores available. Then this would run one job per core.
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 see, in section 5.4:
If there is nothing looking like an integer after the ‘-j’ option, there is no limit on the number of job slots.
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.
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.
Oh, numCPU set by
numproc