-
Notifications
You must be signed in to change notification settings - Fork 23
Implement system hydrodynamics #7
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dawsonc
reviewed
Apr 27, 2023
dawsonc
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.
Overall, nice work! I have some high-level comments:
- When I see a struct/class called
Dynamics, my assumption is that it will be able to compute the state derivatives (dx/dt) for me, but I don't see this in your code. Is that a feature that you plan to add to theVehicleDynamicsstruct? If not, then I would think of this struct as more of aVehicleParameters. - I think that the intended way to interact with
VehicleDynamicsis not clear. All of the members are public, so am I supposed to just use them? But it seems like that could lead to easy mistakes (e.g. usingadded_mass.toMatrix()since you've defined those convenient helper methods rather thancalculateAddedMassMatrix). Consider making it clear (in documentation and through use of private members) what the intended way to access these data is. - I left a few scattered comments about how it feels inconsistent that some
VehicleDynamicsmethods require you to manually pass in vehicle parameters, while others use parameters directly from the struct. Thinking about the intended use case: would you ever want to use these methods without a fullVehicleDynamicsstruct? Would some of these methods be better off as private methods? I guess then they become harder to test, but there's some discussion of that online. - You asked about stamped vs non-stamped message types. My feeling is that if you accept the stamped message type, you're making an implicit promise to respect the header and transform the type appropriately before using. It seems like your code is not doing this, which could lead to bugs when someone tries passing in a stamped type in a frame other than the inertial frame. It would probably be a pain to make
VehicleDynamicsrely on having a TF tree connection, since that would make testing a pain and break some isolation, so I would lean towards using non-stamped types and specifying in the function documentation that they must be in a certain frame.
evan-palmer
commented
May 9, 2023
evan-palmer
added a commit
that referenced
this pull request
Jun 29, 2023
* started implementation of thruster dynamics * Finished initial implementation of bridge * Finished initial implementation of bridge * forgot to save * removed unused configs directory * Implement an interface to manage control of the thrusters (#9) * started porting bridge to base controller * continued base controller implementation * bedtime * Added parameter to control publish rate * Continued controller implementation and fixed docker errors * save point * reincluded bridge to separate concerns * continued base controller implementation * added linear velocity calculation * cleaned up lambdas and continued base controller * Started implementing vehicle dynamics * Continued vehicle dynamics implementation * Finished initial untested version of vehicle dynamics * Resolve initial bugs * Started working on unit testing the dynamics * i dont remember * Started implementing test cases and squashed some bugs * Finished vehicle dynamics test cases and added current effects * Added TCM and started thruster dynamics * Removed unrelevant files * Fixed comments * started implementation of manager * Removed unused packages and cleaned up manager implementation * Removed dynamics package as not in pr scope * Implemented and tested parameter backup procedure, started on passthrough mode enabling * Cleaned up implementation and started param backup service * Time for some video games baby * Removed unnecessary methods * Removed more unused methods * Added interface to stop thrusters and improve success rate of mode change command * Refactored to use multithreadedexecutor * Save point for golf * Finalized initial version * Resolved PR comments * Implement system hydrodynamics (#7) * started porting bridge to base controller * continued base controller implementation * bedtime * Added parameter to control publish rate * Continued controller implementation and fixed docker errors * save point * reincluded bridge to separate concerns * continued base controller implementation * added linear velocity calculation * cleaned up lambdas and continued base controller * Started implementing vehicle dynamics * Continued vehicle dynamics implementation * Finished initial untested version of vehicle dynamics * Resolve initial bugs * Started working on unit testing the dynamics * i dont remember * Started implementing test cases and squashed some bugs * Finished vehicle dynamics test cases and added current effects * Added TCM and started thruster dynamics * Removed unrelevant files * Fixed comments * Started refactoring to resolve pr comments * Continued PR refactor * Missing semicolon * found a pattern that makes sense * updated coriolis implementation * Finished refactor * Added docstrings * Adding back negative * Updated test cases * Removed unused dependencies * Removed irrelevant packages from dockerignore * fixed comment * Fixed comment spacing * Removed unused time derivatives * cleaned up documentation * Added deadzone formula * Implemented initial version of thrust surface * Fixed thruster model tests * Added docstrings * Resolved remaining PR comments * Implement a base controller and integral sliding mode controller (#10) * Implemented initial version of base controller * Fixed deps * Started implementation of ismc * Continued ismc implementation * Added service to arm and disarm controller * Fixed custom controllers build * use modernize bind * Resolved PR comments * Implement launch files (#16) * Started integration of launch files and updated dockerfile to properly install mavros * precommit * Added message interval client * Added vision pose plugin * Added docstrings * Fixed pr comments * Removed controller arming from manager (#17) * Port angler_localization to blue_localization (#18) * Ported angler_localization * Added localization package to dockerignore * Resolved controller bug and fixed dockerfile * Removed redundant install * Resolved PR comments * Integrate support for simulation with Gazebo and ArduSub (#23) * Started integrating ardusub and gazebo into dockerfile * Added Gazebo Garden installation to dockerfile * Added workspace.repos for models * added vcs import to devcontainer * Started blue description * Added ability to launch gz with models * Started to add ardusub process into launch * Added ros_gz and ardupilot bridge to enable sitl * Cleaned up for PR * Fixed codespell and ran linters * Resolved PR comments * Use tf2_eigen for msg conversions (#26) * Added tf2_eigen and switched dynamic matrices to fixed-size matrices * Resolved PR comments * Added thruster dynamics without battery state model (#27) * Started implementing thruster dynamics without battery state * Added thruster models without battery state * Tune ISMC and resolve coordinate frame issues (#32) * Started debugging controller * Added proportional gain and fixed missing initial conditions * Added gazebo localizer and fixed some more bugs * IT WORKS * Cleaned up comments and re-added heavy launch file * More bug fixes * Started cleanup * Continued cleanup and updated readme * Added support to run foxglove from launch * Renamed reference to twistaccelcmd * Started to resolve pr comments * WIP: tuning the controller for the bluerov2 base configuration * WIP: Im tired of this * WIP: resolved bugs in transforms and tcm * Final fixes for pr and added keyboard teleop * Resolve blue_control build errors (#38) * Resolved build issues and cleaned up utils * Resolved bugs in build and bluerov base config tcm * Removed negatives from zeros in the config files * Moved tf2 import to correct file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
Changes Made
This PR introduces a library which should be used by a controller to calculate the dynamics of the vehicle and its respective thrusters. Entrypoints have been implemented for the vehicle dynamics and the thruster dynamics. The current effects and thruster configuration matrices have also been added.
Associated Issues
Testing
Google Test has been used to create a collection of test cases to evaluate the correctness of the implementation.
Additional Notes for Reviewers
The main points that I am hoping to elicit some feedback on are as follows:
blue_dynamics/include/blue_dynamics/current_effects.hpp/cppVehicleDynamicsstruct? I made the decision to implement a collection of structs to use for defining collections of variables to improve readability as opposed to listing all parameters. However, it may be painful to require users to instantiate each of these just to create the vehicle dynamicsblue_dynamics/src/vehicle_dynamics.hpp/cppblue_dynamics/include/blue_dynamics/hydrodynamics.hpp/cppblue_dynamics/test/test_vehicle_dynamics.cpp