Rsu health monitor #564
Rsu health monitor #564
Conversation
src/v2i-hub/RSUHealthMonitorPlugin/src/RSUHealthMonitorPlugin.cpp
Outdated
Show resolved
Hide resolved
| } | ||
| catch (nmea::NMEAParseError &e) | ||
| { | ||
| fprintf(stderr, "Error:%s\n", e.message.c_str()); |
There was a problem hiding this comment.
Why are we using fprintf instead of PLOG
There was a problem hiding this comment.
Addressed a49366e
| result.insert({std::stod(latitude_str), std::stod(longitude_str)}); | ||
| PLOG(logDEBUG) << "Parse GPS NMEA string: " << gps_nmea_data << ". Result (Latitude, Longitude): (" << latitude_str << "," << longitude_str << ")"; | ||
| } | ||
| catch (nmea::NMEAParseError &e) |
There was a problem hiding this comment.
Should be constant
There was a problem hiding this comment.
Addressed a49366e
| if (rsuStatusConfigTbl.size() == 0) | ||
| { | ||
| PLOG(logERROR) << "RSU status update call failed due to the RSU status config table is empty!"; | ||
| return Json::nullValue; |
There was a problem hiding this comment.
Why are we printing error and returning null instead of throwing an exception
There was a problem hiding this comment.
We encourage users to update the RSU config table by updating the mib version with the supported version, but it is not a fatal program issue.
There was a problem hiding this comment.
Exceptions do not need to be fatal but this is fine for now.
src/tmx/TmxUtils/src/SNMPClient.cpp
Outdated
| snmp_pdu *response; | ||
|
|
||
| std::string result = ""; | ||
| auto pdu = snmp_pdu_create(SNMP_MSG_GET); |
There was a problem hiding this comment.
There is a member variable with the same name and overlapping scope. We should probably reconsider naming here
There was a problem hiding this comment.
Addressed a49366e
| }; | ||
|
|
||
| /** | ||
| * RSUStatusTable is custome defined RSU status information. |
There was a problem hiding this comment.
Addressed a49366e
|
|
||
| /** | ||
| * RSUStatusTable is custome defined RSU status information. | ||
| * The fields are the subset of fields from RSU MIB definition https://github.com/certificationoperatingcouncil/COC_TestSpecs/blob/master/AppNotes/RSU/RSU-MIB.txt |
There was a problem hiding this comment.
The fields are a subset of the fields from the RSU MIB definition used to quantify the health of the RSU.
There was a problem hiding this comment.
Addressed a49366e
| { | ||
| private: | ||
| // A map of RSU MIB version used and RSUStatusTable | ||
| shared_ptr<map<RSUMibVersion, RSUStatusConfigTable>> _RSUSTATUSConfigMapPtr; |
There was a problem hiding this comment.
Why do we have a map between mib version and RSU Status Config table? Will we populate this map with all versions regardless of the configured version and then just use the configured version?
There was a problem hiding this comment.
yes, we will populate the map with all versions if exist like NTCIP1218 and RSU4.1. But the users can choose which version to use by specifying the mib version through the UI.
| * @param vector<string> Input RSU fields to verify | ||
| * @return True if all required fields found. Otherwise, false. | ||
| */ | ||
| bool isAllRequiredFieldsPresent(const RSUMibVersion &mibVersion, const vector<string> &fields) const; |
There was a problem hiding this comment.
Can we rename this method to validate. Also I am not sure I understand this method. Is it meant to validate a RSU status message or is it meant to confirm all the required OIDs are obtainable from SNMP client?
There was a problem hiding this comment.
sure, update to validate. Yes, it is to make sure we obtain all the required fields.
There was a problem hiding this comment.
Addressed a49366e
| return rsuStatusTbl; | ||
| } | ||
|
|
||
| bool RSUHealthMonitorWorker::isAllRequiredFieldsPresent(const RSUMibVersion &mibVersion, const vector<string> &fields) const |
There was a problem hiding this comment.
If this is a validation method I suggest for clarity sake you pass in the config directly instead of passing in the mib version and calling Get config. I think it is cleaner for the validate method to take parameters ( message to validate, list of required fields)
There was a problem hiding this comment.
Addressed a49366e
| _timerThId = _rsuStatusTimer->AddPeriodicTick([this]() | ||
| { | ||
| // Periodic SNMP call to get RSU status based on RSU MIB version 4.1 | ||
| auto rsuStatusJson = _rsuWorker->getRSUStatus(_rsuMibVersion, _rsuIp, _snmpPort, _securityUser, _authPassPhrase, _securityLevel, SEC_TO_MICRO); |
There was a problem hiding this comment.
This is confusing to me. So you get the RSU status here by querying the SNMP server and you return a JSON. Then you wait until the broadcast method to check that the required information is present? You do not have to change this, it just seems weird to me. I would imagine that the method that gets the RSU status either directly turns it into the tmx message (including validation) or the next method turns it into a tmx message.
There was a problem hiding this comment.
If the JSON is empty due to some SNMP timeout or other issues, we stop any further logic like creating TMX message, broadcasting message.
PR Details
Description
RSU health monitor plugin is a V2xHub plugin that interface with its connected RSU directly via SNMP protocol. The plugin is responsible for monitoring the connected RSU status by constantly ping RSU device. For detailed design of this plugin, refer to https://usdot-carma.atlassian.net/wiki/spaces/WFD2/pages/2640740360/RSU+Health+Monitor+Plugin+Design .
Related Issue
WFD-435
#591
Motivation and Context
In support of WFD telematics testing (Data visualization)
How Has This Been Tested?
Integration test
Types of changes
Checklist:
V2XHUB Contributing Guide