Reasonable message checker for buggy state converter#101
Reasonable message checker for buggy state converter#101the-beast-coder wants to merge 3 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # if opt.mode < 2 and not opt.output_avi_file.endswith(".avi"): | ||
| # print("--output_avi_file parameter should be a .avi file but is not : ",opt.output_avi_file,"Exit program.") | ||
| # exit() |
|
|
||
| from util.constants import Constants | ||
|
|
||
| from ukf_utils import * |
There was a problem hiding this comment.
Pull request overview
Adds a “reasonable message” sanity-check layer to BuggyStateConverter so that clearly corrupt/invalid nav_msgs/Odometry inputs can be rejected before conversion and republishing.
Changes:
- Introduces
is_reasonable(...)andis_reasonable_msg(...)helpers to validate odometry position/orientation/twist against configurable bounds. - Applies sanity checks in both
convert_SC_state(...)(ECEF/quaternion source) andconvert_NAND_state(...)(UTM/radians source). - Updates
.gitignoreto ignore.robobuggy/(and normalizes the.vscodeentry).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
rb_ws/src/buggy/scripts/buggy_state_converter.py |
Adds odometry sanity-check helpers and uses them to gate SC/NAND state conversions. |
rb_ws/.gitignore |
Ignores .robobuggy/ directory and adjusts .vscode ignore line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| converted_msg = Odometry() | ||
| converted_msg.header = msg.header |
There was a problem hiding this comment.
convert_SC_state now returns None when the message is deemed unreasonable, but convert_SC_state_callback always publishes the return value. Publishing None will raise at runtime and potentially kill the node. Consider returning the last valid Odometry, or have the callback skip publishing when conversion returns None (and ideally log a throttled warning when discarding).
| return | |
| converted_msg = Odometry() | |
| converted_msg.header = msg.header | |
| now = time.monotonic() | |
| last_warn_time = getattr(self, "_last_invalid_sc_state_warning_time", 0.0) | |
| if now - last_warn_time >= 1.0: | |
| self.get_logger().warning( | |
| "Discarding unreasonable SC state message; reusing last valid converted odometry when available." | |
| ) | |
| self._last_invalid_sc_state_warning_time = now | |
| last_valid_msg = getattr(self, "_last_valid_sc_odometry", None) | |
| if last_valid_msg is not None: | |
| return last_valid_msg | |
| fallback_msg = Odometry() | |
| fallback_msg.header = msg.header | |
| return fallback_msg | |
| converted_msg = Odometry() | |
| converted_msg.header = msg.header | |
| self._last_valid_sc_odometry = converted_msg |
| ): | ||
| return |
There was a problem hiding this comment.
convert_NAND_state now returns None when the message is deemed unreasonable, but convert_NAND_state_callback always publishes the return value. Publishing None will raise at runtime and potentially kill the node. Consider returning the last valid Odometry, or have the callback skip publishing when conversion returns None (and ideally log a throttled warning when discarding).
| per-axis tuples. Covariance limits are scalar bounds applied to every | ||
| covariance entry. The defaults are intentionally broad and meant to catch |
There was a problem hiding this comment.
The is_reasonable_msg docstring says covariance bounds are checked/applied, but the function currently only validates pose position/orientation and twist. Either update the docstring to match the implementation or add the intended covariance checks.
| per-axis tuples. Covariance limits are scalar bounds applied to every | |
| covariance entry. The defaults are intentionally broad and meant to catch | |
| per-axis tuples. The defaults are intentionally broad and meant to catch |
| """ | ||
| if not is_reasonable_msg( | ||
| msg, | ||
| # UTM (easting, northing, altitude) for Pittsburgbh |
There was a problem hiding this comment.
Typo in comment: "Pittsburgbh" should be "Pittsburgh".
| # UTM (easting, northing, altitude) for Pittsburgbh | |
| # UTM (easting, northing, altitude) for Pittsburgh |
Overview
Added a reasonable checker to the buggy state converter. Whenever messages first come in, they are checked by the is_reasonable_msg (with params needing to be checked over and customized for each buggy). If the message seems reasonable, the buggy state converter proceeds as normal. If not, then the message is ignored altogether.
Tasks