Anbit/guidance#698
Conversation
for more information, see https://pre-commit.ci
* ci(pre-commit): add CI workflow for checking pre-commit hooks * ci(pre-commit): add hooks for cpp and CMake * refactor(pre-commit.yml): now uses the workflow on main * docs(README.md): update status badge for pre-commit * refactor(ci): move local hooks into a different config file * docs(README.md): update status badge for pre-commit * refactor(ci): pre-commit.yml now only runs on pull_request and workflow_dispatch * ci(pre-commit-local): ignore build/include_order in ament_cpplint since conflict with clang-format * refactor: fix ament_cpplint and cppcheck warnings * ci(pre-commit-config-local): increase line length to 120 for ament_cpplint due to some lines being longer * refactor: fix ament_cpplint and cppcheck warnings * chore(pre-commit): exclude test dirs from ament_cppcheck to avoid false positives * ci(pre-commit): update workflow to run on push to main and pull_request
* waypoint mode handling * update tests with new message * initial package setup * ros interface function declaration * node setup * working prototype * reentrant cb, multithreaded * single threaded impl * conv threshold action goal * default thresholdref conv value * removed switching logic * removed timer execution * sim test utils * waypoint_manager_test setup * no rendering test arg * waypoint tests, timeout error * test refactor * format * rename utils package * test suite and description * first waypoint test * removed unused function * renamed service field to priority. Added simple tests * waypoint manager readme * uniform attitude naming convention * fix pr requests * update tests with new service fields * four corner test * update util func name * update with new action def * removed failing build type * test dependencies * ignore failing yasmin package * remove __init__ files * quat_to_euler in make_pose helper * added __init__ file * removed sim deps for test packages * added action shutdown handling * removed waypoint manager set setup * added waypoint manager node tests * waypoint manager 4 corner sim test * added missing launch testing test dependency * add sleep for topic discovery * fix action member field name * removed unnecessary if here
* waypoint mode handling * update tests with new message * initial package setup * ros interface function declaration * node setup * working prototype * reentrant cb, multithreaded * single threaded impl * conv threshold action goal * default thresholdref conv value * removed switching logic * removed timer execution * sim test utils * waypoint_manager_test setup * no rendering test arg * waypoint tests, timeout error * test refactor * format * rename utils package * test suite and description * first waypoint test * removed unused function * renamed service field to priority. Added simple tests * waypoint manager readme * uniform attitude naming convention * fix pr requests * update tests with new service fields * four corner test * update util func name * update with new action def * removed failing build type * test dependencies * ignore failing yasmin package * remove __init__ files * quat_to_euler in make_pose helper * added __init__ file * removed sim deps for test packages * added action shutdown handling * removed waypoint manager set setup * added waypoint manager node tests * waypoint manager 4 corner sim test * added missing launch testing test dependency * add sleep for topic discovery * fix action member field name * updated to new utils type names * renamed variables to match types * update function arg to reflect vortex type * update variable name in tests * renamed function arg names
* refactor to new utils ros packages * added utils dep for velocity controller * fix typo
vortexuser
left a comment
There was a problem hiding this comment.
first of round of reviews
6a2a511 to
55131a9
Compare
jorgenfj
left a comment
There was a problem hiding this comment.
I think it makes sense to move most of the state management out of the ros node. Could follow a similar setup to the "waypoint_follower.cpp" file in the reference filter
| double time_step_s = 0.1; | ||
| time_step_ = | ||
| std::chrono::milliseconds(static_cast<int>(time_step_s * 1000)); |
There was a problem hiding this comment.
Maybe make this a config param?
Also should make the member variable name more explicit by naming it time_step_ms_
| const auto& v = odom_copy->twist.twist.linear; | ||
| double surge = std::sqrt(v.x * v.x + v.y * v.y + v.z * v.z); | ||
|
|
||
| vortex_msgs::msg::LOSGuidance state_debug_msg; | ||
| Eigen::Vector3d euler = vortex::utils::math::quat_to_euler( | ||
| Eigen::Quaterniond(odom_copy->pose.pose.orientation.w, | ||
| odom_copy->pose.pose.orientation.x, | ||
| odom_copy->pose.pose.orientation.y, | ||
| odom_copy->pose.pose.orientation.z)); | ||
|
|
||
| state_debug_msg.pitch = euler.y(); | ||
| state_debug_msg.yaw = euler.z(); | ||
| state_debug_msg.surge = surge; | ||
|
|
There was a problem hiding this comment.
This should be a util function
There was a problem hiding this comment.
Could also consider wether we should move this into vortex_utility_nodes in vortex_utils to avoid creating a dependency on velocity/odom in this node. Either way, the odom sub creating should be guarded by a debug flag so we dont subscribe uncessary
jorgenfj
left a comment
There was a problem hiding this comment.
Suggestion: replace the four unique_ptr members + ActiveLosMethod enum with std::variant
Currently LosGuidanceStateManager holds all four LOS implementations alive simultaneously and uses
a separate method_ enum + switch statement to dispatch:
types::ActiveLosMethod method_{};
std::unique_ptr adaptive_los_{};
std::unique_ptr integral_los_{};
std::unique_ptr proportional_los_{};
std::unique_ptr vector_field_los_{};
Since all four types share the same calculate_outputs(const types::Inputs&) → types::Outputs
interface, this could be collapsed into a single:
std::variant<ProportionalLOSGuidance, IntegralLOSGuidance,
AdaptiveLOSGuidance, VectorFieldLOSGuidance> los_method_;
The dispatch switch in calculate_outputs() then becomes:
std::visit([&](auto& los) { outputs = los.calculate_outputs(inputs_copy); }, los_method_);
The main complication with variant is that set_los_method() can no longer just set an enum —
switching types requires constructing the new object with its parameters. This is resolved by tying
method selection to goal initialization: initialize_goal() re-parses the YAML config, reads the
active method, and emplaces the right alternative with fresh parameters. This also means config
changes (lookahead distances, gains, etc.) take effect at the start of each new segment without a
restart.
A nice side effect: constructing a fresh instance implicitly resets stateful methods
(AdaptiveLOSGuidance, IntegralLOSGuidance), so the unconditional adaptive_los_->reset() call in
initialize_goal() — which currently fires regardless of the active method — goes away naturally.
^^from mr. claude.
If you go for variant you should store the yaml file path as a member. And the 4 los setters in the constructor should be replaced by a single function set correct los method based on the active los config param
| struct Outputs { | ||
| double psi_d{}; | ||
| double theta_d{}; | ||
| }; |
There was a problem hiding this comment.
Rename to GuidanceOutputs or something more explicit
| struct Inputs { | ||
| Point prev_point{}; | ||
| Point next_point{}; | ||
| Point current_position{}; | ||
| }; |
There was a problem hiding this comment.
Same here. Maybe make the name more explicit
| #include <mutex> | ||
| #include <string> | ||
|
|
||
| #include <vortex_msgs/action/guidance_waypoint.hpp> |
| state_manager_->set_los_method( | ||
| static_cast<types::ActiveLosMethod>(request->mode)); | ||
|
|
There was a problem hiding this comment.
Same comment as previously with the ActiveLosMethod initialization. This can cast to values outside the valid ENUM range. Also there is no guarantee that the vortexmsg value matches the enum values. Should use an explicit conversion function
This PR adds the los_guidance package and integrates it into the autopilot setup for waypoint-based guidance of the AUV. The node computes guidance references from the current vehicle state and a target waypoint, and sends the desired yaw, pitch, and surge to the downstream controller.
The package also has overwritten the old simulator test with need changes for it to work with the new guidance. Most of the implementation details, usage, and configuration are described in the README.