-
Notifications
You must be signed in to change notification settings - Fork 24
Anbit/guidance #642
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
base: main
Are you sure you want to change the base?
Anbit/guidance #642
Conversation
for more information, see https://pre-commit.ci
|
just a control question, what's the use-case that requires one node to ad-hoc switch the LOS-flavor to use? I'm not too too familiar with the full system / intended use here |
Andeshog
left a comment
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.
Good job so far!
| services: # Maybe not the right place for this? | ||
| los_mode: "set_los_mode" | ||
|
|
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.
This is fine! Although if I am going to be picky I think it should be between topics and actions hehe
| if(BUILD_TESTING) | ||
| include(CTest) |
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.
Is the CTest include needed here?
| // again i dont know if i should have them here or just in the functions | ||
| double pi_h_{}; | ||
| double pi_v_{}; | ||
| Eigen::AngleAxisd rotation_y_{0.0, Eigen::Vector3d::UnitY()}; | ||
| Eigen::AngleAxisd rotation_z_{0.0, Eigen::Vector3d::UnitZ()}; |
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.
It is a fair point, and it makes sense to have just have them as input/output for the calculations
| PROPORTIONAL, // 0 | ||
| INTEGRAL, // 1 | ||
| ADAPTIVE // 2 |
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.
You could also explicitly set the integers
enum class ActiveLosMethod : uint8_t {
PROPORTIONAL = 0,
INTEGRAL = 1,
ADAPTIVE = 2
};| const types::CrossTrackError& e) { | ||
| const double denom_h = std::sqrt(m_params.lookahead_distance_h * | ||
| m_params.lookahead_distance_h + | ||
| e.y_e * e.y_e); | ||
| const double denom_v = std::sqrt(m_params.lookahead_distance_v * | ||
| m_params.lookahead_distance_v + | ||
| e.z_e * e.z_e); | ||
|
|
||
| const double beta_dot = | ||
| m_params.gamma_h * (m_params.lookahead_distance_h / denom_h) * e.y_e; | ||
| const double alpha_dot = | ||
| m_params.gamma_v * (m_params.lookahead_distance_v / denom_v) * e.z_e; | ||
|
|
||
| beta_c_hat_ += beta_dot * m_params.time_step; | ||
| alpha_c_hat_ += alpha_dot * m_params.time_step; | ||
| } |
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.
It's not immediately clear what e represents without looking at the function input. cross_track_error is a perfectly fine name, and it is very clear what it is 👍
|
|
||
| int_h += e.y_e * m_params.time_step; | ||
| int_v += e.z_e * m_params.time_step; | ||
|
|
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.
Sidetracking a bit, but would be cool to have implementations of some common integrators like RK4 and forward euler in vortex-utils that could be used in cases like this👀
| const double k_p_h = 1.0 / std::max(m_params.lookahead_distance_h, 1e-9); | ||
| const double k_p_v = 1.0 / std::max(m_params.lookahead_distance_v, 1e-9); |
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.
This could be avoided by having a parameter validation in the constructor
| auto adaptive_los_config = config["adaptive_los"]; | ||
| spdlog::info("A0"); | ||
| auto params = AdaptiveLosParams{}; | ||
| params.lookahead_distance_h = | ||
| adaptive_los_config["lookahead_distance_h"].as<double>(); | ||
| spdlog::info("A1"); | ||
| params.lookahead_distance_v = | ||
| adaptive_los_config["lookahead_distance_v"].as<double>(); | ||
| spdlog::info("A2"); | ||
| params.gamma_h = adaptive_los_config["gamma_h"].as<double>(); | ||
| spdlog::info("A3"); | ||
| params.gamma_v = adaptive_los_config["gamma_v"].as<double>(); | ||
| spdlog::info("A4"); | ||
| params.time_step = static_cast<double>(time_step_.count()) / 1000.0; | ||
| spdlog::info("A5"); |
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 assume these prints are not supposed to be in the finished product 😆
|
|
||
| m_adaptive_los = std::make_unique<AdaptiveLOSGuidance>(params); |
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.
You are currently running two styles on member naming. I suggest you choose one of them 😄 FYI, the rest of the codebase use trailing underscore
| u_desired_ = common_config["u_desired"].as<double>(); | ||
| spdlog::info("C1"); | ||
| goal_reached_tol_ = common_config["goal_reached_tol"].as<double>(); | ||
| spdlog::info("C2"); | ||
| m_method = static_cast<types::ActiveLosMethod>( | ||
| common_config["active_los_method"].as<int>()); | ||
| spdlog::info("C3"); | ||
| } |
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.
Would probably be wise to lock with the mutex here, as you are writing to data that is accessed in the while loop, to not run into any race conditions?
|
To fix the build errors you could try to add |
|
Thank you @Andeshog for the comments, i will look into all of them after i am done with my exmas. replying to @chrstrom : This explanation is from my understanding, so please do correct me if something doesn't feal right! |
Addition:
From last year, this package was only for Adaptive LOS guidance. To make a LOS library that supports multiple types of LOS-based guidance and is robust enough to allow adding other guidance systems as well, my primary focus has been to implement more LOS-based methods first. As of now, I have added Proportional LOS and Integral LOS, in addition to the Adaptive LOS from last year.
Restructure:
I have also worked on restructuring the existing Adaptive ROS node and YAML file.
ros_node:
To create a system capable of changing the LOS method in the middle of a mission or segment, I needed to restructure the file named los_guidance_ros.cpp. Previously, this file only ran Adaptive LOS, but now it has been modified so that it can receive a method selection from a service called SetLosMode.srv and switch between all available LOS methods within a single running node. Together with our mentor Anders, we planned how the new structure should look and implemented it accordingly.
A new header file named types.hpp has been added, which holds all the shared data structures used in the code. All parts of the code are now within the same namespace, vortex::guidance::los, making it easier to reference and maintain consistency across different components.
YAML:
The parameter file has been restructured to use the default YAML format. This was done to clearly separate ROS parameters from code-based parameters. The remaining files such as CMakeLists.txt, package.xml, and the launch file have also been updated accordingly.
Test:
There was a test file for Adaptive LOS from last year. Following the same concept, I created equivalent tests for all the new LOS methods. Since there are now multiple test files, I also added a main test file that contains the main function to run all tests together.
For now, if we want to run a single test, it must be done using ROS2 commands in the terminal, but this can be improved later to make it simpler.
All test invocation:
colcon test --packages-select los_guidance --event-handlers console_direct+colcon test-result --verboseExample single-test invocation:
colcon test --packages-select los_guidance --ctest-args -R ProportionalLosTestPlan further:
I plan to run more tests to ensure that all LOS methods work as expected. I also want to make further adjustments so that the ROS node can either remember or reset the Adaptive and Integral values. It would be useful to retain these values within the same segment but reset them when starting a new one.
Lastly, I plan to add more LOS methods, and if time permits, explore creating a library for other types of guidance methods as well.
Summary:
This PR completes the first stage of the LOS guidance library as planned. It introduces two new LOS methods (Proportional LOS and Integral LOS) and provides a clear separation between different methods. Additionally, the ROS2 node has been restructured to support runtime method switching, with updated configuration and the addition of unit tests for each method.
The node is ready to run, it builds and launches, but I still need to perform more testing to confirm that all methods work correctly and continue developing new LOS variants in the next phase.