Ackermann: Refactor #24285
Open
Ackermann: Refactor #24285
+2,409
−1,328
Conversation
This file contains 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
chfriedrich98
force-pushed
the
pr-rover_refactor
branch
from
8411c45 to
c91cfa7
Compare
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: 344 byte (0.02 %)]px4_fmu-v6x [Total VM Diff: 352 byte (0.02 %)]Updated: 2025-02-07T15:06:33 |
chfriedrich98
force-pushed
the
pr-rover_refactor
branch
from
b3b20c0 to
dc7162a
Compare
sfuhrer
reviewed
Feb 5, 2025
There was a problem hiding this comment.
Thanks for putting in the effort for a clean and shared interface and architecture between all the rover types!
General remarks:
- as handle both position and velocity in the "Position" controller - how about renaming it to PositionVelocityControl?
- in the diagram you show that also attitude and rate controllers subscribe to TrajectorySetpoint.msg - is this only for Differential/Mecanum?
|
|
||
| void AckermannAttControl::generateAttitudeSetpoint() | ||
| { | ||
| bool stab_mode_enabled = _vehicle_control_mode.flag_control_manual_enabled |
There was a problem hiding this comment.
Suggested change
| bool stab_mode_enabled = _vehicle_control_mode.flag_control_manual_enabled | |
| const bool stab_mode_enabled = _vehicle_control_mode.flag_control_manual_enabled |
There was a problem hiding this comment.
Please go through and set const also below where applicable
There was a problem hiding this comment.
Scanned all files and added the missing const prefixes here: 454995a
There was a problem hiding this comment.
Mhh cannot find this commit anymore - is it still applied? And my comment here for a const was not valid?
There was a problem hiding this comment.
Yes, that should be const. Fixed in e226138
src/modules/rover_ackermann/AckermannRateControl/AckermannRateControl.cpp
Show resolved
Hide resolved
src/modules/rover_ackermann/AckermannPosControl/AckermannPosControl.hpp
Outdated
Show resolved
Hide resolved
src/modules/rover_ackermann/AckermannPosControl/AckermannPosControl.cpp
Outdated
Show resolved
Hide resolved
chfriedrich98
force-pushed
the
pr-rover_refactor
branch
from
8d3e04b to
5cdc876
Compare
|
@sfuhrer Thanks for the review!
Implemented that in 5cdc876, also renamed the parameter group
Yes, while the refactor does technically enable setting yaw/yaw rate setpoints through the TrajectorySetpoint also for ackermann it gets messy because the yaw/yaw rate is not decoupled from the velocity (or speed) as it is in differential/mecanum rovers. |
chfriedrich98
force-pushed
the
pr-rover_refactor
branch
4 times, most recently
from
3cdb723 to
53c100f
Compare
|
rebased on main |
chfriedrich98
force-pushed
the
pr-rover_refactor
branch
from
729c948 to
7c03dec
Compare
|
@Pedro-Roque Just FYI i properly disabled the rover modules on the spacecraft board here. |
sfuhrer
reviewed
Feb 7, 2025
|
|
||
| void AckermannAttControl::generateAttitudeSetpoint() | ||
| { | ||
| bool stab_mode_enabled = _vehicle_control_mode.flag_control_manual_enabled |
There was a problem hiding this comment.
Mhh cannot find this commit anymore - is it still applied? And my comment here for a const was not valid?
sfuhrer
reviewed
Feb 7, 2025
| @@ -71,13 +71,13 @@ void AckermannRateControl::updateRateControl() | |||
| vehicle_angular_velocity.xyz[2] : 0.f; | |||
| } | |||
|
|
|||
| if (_vehicle_control_mode.flag_control_rates_enabled && _vehicle_control_mode.flag_armed) { | |||
| if (_vehicle_control_mode.flag_control_rates_enabled && _vehicle_control_mode.flag_armed && runSanityChecks()) { | |||
There was a problem hiding this comment.
How about just checking _sanity_check here and calling runSanityChecks only on param update?
There was a problem hiding this comment.
I agree that it is suboptimal to run this check on every update rather then on paramUpdate, but the check should also only be performed if the corresponding flag of the mode is active (i.e. only do the check for posVelControl if it is actually active). And there is the scenario where the mode is switched from i.e. acro to position without changing parameters which will then not perform the checks for posVel parameters.
sfuhrer
reviewed
Feb 7, 2025
| @@ -112,6 +119,7 @@ class AckermannRateControl : public ModuleParams | |||
| float _vehicle_yaw_rate{0.f}; | |||
| hrt_abstime _timestamp{0}; | |||
| float _dt{0.f}; // Time since last update [s]. | |||
| bool _sanity_check{true}; | |||
There was a problem hiding this comment.
I like when bools are very clearly visible as such - e.g. _parameter_sanity_checks_passed
There was a problem hiding this comment.
Clarified in 3734e4a
Thanks @chfriedrich98 ! 🙏 |
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.
Solved Problem
This PR starts a restructure of the rover modules to the following architecture:
This PR focuses on restructuring the ackermann rover module (differential and mecanum will be updated in subsequent PRs).
It also moves functions and parameters that are shared by the different rover modules to a common rover library (
rover_control) to reduce redundancy. This library and parameters will also be used in follow up PRs for the differential and mecanum rover modules.In order to homogenize the 3 rover modules, the rate control for the ackermann module is transformed from lateral acceleration to yaw rate control (to get in line with differential/mecanum). This does not lead to a functional difference as lateral acceleration ($$a_y$$ ) and yaw rate ($$\dot{\psi}$$ ) are direclty linked with this equation: $$a_y = V \cdot \dot{\psi}$$ with $$V: $$ Forward speed.
This PR also introduces offboard support for ackermann rovers through the
trajectorySetpoint.msg. (Works on #23663)The Ackermann module supports position and velocity setpoints.
Test coverage