355 navigation loss of gps signal or weak gps signal#396
355 navigation loss of gps signal or weak gps signal#396rosalievanark wants to merge 11 commits intomainfrom
Conversation
8009eaf to
e1bd350
Compare
Signed-off-by: Rosalie <[email protected]>
…uration is requested Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
… central diagnostics node), add simulation parameter to check whether simulation or realtime should be used, and final linting fix Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
113734c to
c2c96bb
Compare
…t now also reacts during FollowPath Signed-off-by: Rosalie <[email protected]>
Signed-off-by: Rosalie <[email protected]>
|
|
||
| protected: | ||
| /// Current diagnostic status message. | ||
| diagnostic_msgs::msg::DiagnosticStatus status_; |
There was a problem hiding this comment.
Perhaps the no_data_received_limit you defined in gps_diagnostics.hpp can be moved to this, as we can expect the same timeout behavior in other future diagnostics classes? Maybe in the form of three variables (warning_timeout / error_timeout / stale_timeout) as rclcpp::Durations? That way an absence of (GPS) data will escalate slowly, giving the operator or BT time to take appropriate action
| std::bind(&DiagnosticsNode::diagnostics_cb, this)); | ||
| } | ||
|
|
||
| void DiagnosticsNode::diagnostics_cb() { |
| gps_msg_received = true; | ||
|
|
||
| latest_msg_time = msg->header.stamp; | ||
| latest_covariance = msg->position_covariance[0]; |
There was a problem hiding this comment.
This only takes into account latitude, not longitude. Maybe a max(msg->position_covariance[0], msg->position_covariance[4]) to take whichever is longer out of lat/lon?
|
|
||
| rclcpp::Duration since_last = now - latest_msg_time; | ||
|
|
||
| if (since_last.seconds() > no_data_received_limit) { |
There was a problem hiding this comment.
If multiple things are wrong, only one fault will be returned. Is this intentional?
|
|
||
| /// The number of seconds of having received no data until the GPS signal is | ||
| /// deemed to be unstable | ||
| double no_data_received_limit = 5.0; |
There was a problem hiding this comment.
Could be more semantically meaningful if this was a rclcpp::Duration instance (as with the other double/float members representing time instances.
Also, no need for doubles here: a float already gives you a 0.00001 second precision. I find doubles are usually only needed when making very exact representations, e.g. lat/lon.
Also also, could be nice to put these into variables that are declared/gotten in the nodes. That way we keep the configuration in the launch files or even predefined_configurations.py.
|
|
||
| # C++ executables: | ||
| include_directories(include) | ||
| add_executable(diagnostics |
There was a problem hiding this comment.
Nitpick: I would vote in favor of naming our nodes ${PROJECT_NAME}_node if there is only one node for this package.
|
|
||
| install( | ||
| TARGETS diagnostics | ||
| DESTINATION lib/${PROJECT_NAME} |
There was a problem hiding this comment.
Any specific reason for putting this executable in lib/?
| @@ -0,0 +1,129 @@ | |||
| // # SPDX-FileCopyrightText: Alliander N. V. | |||
There was a problem hiding this comment.
How do you envision this when we expand the diagnostics in the future? Would each diagnostic (GPS, IMU, LiDAR, costmap, network, etc.) have its own node? Or would it all be in this node, which will then be something like diagnostics_health_check.cpp?
Description
Adjusted the Nav2 behaviour tree to include a custom GPS health check condition, and added a diagnostics package (set up in a modular manner such that it can easily be expanded on). Once the GPS is too unstable or even not received, the behaviour tree will stay in waiting mode until the GPS signal is restored, or until a stop command is send.
Fixes: #355
Testing
Simulation:
topic_toolsfor this purpose which is now by default added to thealliander_coreimage. Specifically tested like so:Hardware:
Used
uv run start.py gpsfor purely testing the diagnostics package.Used
uv run start.py panther_gps_navigationto test the waiting behaviour.Documentation
TODO:
Additional Notes
Any relevant screenshots, logs, or context.