Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Rover controller refactor #19957

Closed
wants to merge 7 commits into from
Closed

[WIP] Rover controller refactor #19957

wants to merge 7 commits into from

Conversation

junwoo091400
Copy link
Contributor

Describe problem solved by this pull request

This started out as testing the #18317, but I ended up doing a lot of side refactoring to improve code readability and update comments / parameter definitions. This is not directly related to improving the PR, and was an initial step I took since I thought the code was pretty messy and unreadable in the beginning.

Also there's new airframe file like the one for the RC speed boat I am testing this on, which shouldn't make it to the main branch anyways (since we are only going to support generic geometries from now on).

Describe your solution

A clear and concise description of what you have implemented.

Describe possible alternatives

A clear and concise description of alternative solutions or features you've considered.

Test data / coverage

How was it tested? What cases were covered? Logs uploaded to https://review.px4.io/ and screenshots of the important plot parts.

Additional context

Add any other related context or media.

@junwoo091400 junwoo091400 mentioned this pull request Jul 25, 2022
Closed
2 tasks
@junwoo091400
Copy link
Contributor Author

Rebased on main

Jaeyoung-Lim
Jaeyoung-Lim reviewed Jul 26, 2022
View reviewed changes
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that it is tempting to fix everything at once, but would it be possible to prioritize on getting #18317 in with the current state and we separate the refactoring with the rate controller PR? Note that the current rover PR have already gone through a recent refactor (#16847) and is still not in a good state.

Since people have been already using the current state of the rover position controller we should not try to break it for other people while we move forward for boats.

If we combine refactoring with the added rate control it will be hard to verify whether the behavior is consistent with the previous state. I think it would be much easier to review if we separate refactoring and feature additions.

@junwoo091400
Copy link
Contributor Author

image

Initial rate control SITL test shows good progress. All the PID values are 0 and only the feed-forward was enabled.

  1. With full differential thrust, vehicle hardly reaches the yaw rate setpoint of +/- 1.0 rad/s.
  2. Feedforward gain of 1.5 is too big (e.g. with 1 rad/s rate command, we don't want to command actuator control of 1.5 😉)

@junwoo091400
Copy link
Contributor Author

I understand that it is tempting to fix everything at once, but would it be possible to prioritize on getting #18317 in with the current state and we separate the refactoring with the rate controller PR? Note that the current rover PR have already gone through a recent refactor (#16847) and is still not in a good state.

Yes I understand 😿, that's why I am now adding marginal commits like this one so that I can extract only the rate control related commits out of this PR and get it merged.

This PR is mostly a reflection of this branch I am testing the boat on that shows the progress / overall changes, and as you mentioned it would be easier if I separate this into the rate control improvement PR + refactor PR! Which I am hoping to do so with marginal commits 😏

@junwoo091400 junwoo091400 force-pushed the boat_bigstorm branch 2 times, most recently from ea1a866 to 3b2cdda Compare July 27, 2022 18:47
@junwoo091400
Copy link
Contributor Author

junwoo091400 commented Jul 27, 2022 •
edited
Loading

SITL test:

image

Velocity based torque control output scaling has been added. Work in progresszzzz!

@junwoo091400
Copy link
Contributor Author

junwoo091400 commented Jul 28, 2022 •
edited
Loading

Screenshot from 2022-07-28 14-29-01

Log is here: https://logs.px4.io/plot_app?log=3600d086-09a5-4d06-8433-bfc94a1b6efc

Just completed a real life test of the acro mode. On the graph attached above, the rate setpoint / actual rate are displayed on left-right corner, as well as other debugging values in other panels.

Overall, yaw rate acro mode seems to work kinda ok-ish on a real vehicle.
Few things that I adjusted during the test:

  1. Setting the GND_RATE_MAX to a sane value is important. On the first test, having 1.5 rad/s as the max rate, and hence making that the yaw rate setpoint completely saturated the acuator control for yaw. Therefore on this test I set it to 0.5 rad/s, after observing that vehicle often has yaw rate of around 0.5 ~ 1.0 rad/s during the turn.
  2. Adjusted the GND_RATE_FF, and this alone seemed to control the boat just fine actually! (Maybe since it's a catamaran vehicle, the water surface damping is more prevalent)
  3. Enabled the GND_RATE_P, which gave some active control in acro mode, but didn't notice a big difference in performance increase.
  4. The yaw control authority scalar depends on the GND_SPEED_TRIM. However, with this run having the value as 1.5 m/s, when the vehicle was moving at ~4 m/s (which happened frequently during fast runs), the scalar went down to 0.25, which diminished the controls significantly. I think this speed needs to be above a sane value as well, otherwise the scalar will diminish the yaw control significantly 🌴

Also, for now acro and manual mode feels pretty much the same (maybe it should be?), but glad to have the first real life test! 🎉

Other notes:

  • Yaw control is more aggressive on the right turn command, due to the rudder having asymmetric angle on min/max control input (right rudder command is ~45 degrees, as left rudder command gives only ~25 degrees deflection). Probably needs to be mitigated by modifying the hardware, but this is also noticable from the log (on manual control, right turns reach 1.0 rad/s easily, but left turns are around 0.6 rad/s maximum)
  • Tuning seems to be a big deal here, as I am moving the rate maximum setpoint around a lot (1.5 -> 0.5), the Feed-Forward term's magnitude varies a lot. Need to settle on a value that makes sense. And this value will in turn be settled as I find out the sane value for the maximum yaw rate with more logs to come 🤔

Btw, it is quite surprising how slow the yaw rates are when I don't go crazy high on the throttle input. It's almost like the SITL boat (which feels extremely slow), around 0.5 rad/s! For now, I am not going crazy high throttle, since the boat often flips and it's hard to rescue it 🤦‍♂️

p.s. here's the first test log link (First ever real life test of the Boat in Acro mode!) : https://logs.px4.io/plot_app?log=9eb535b7-57c2-4804-8c2e-5eb1690711bc

@Jaeyoung-Lim Jaeyoung-Lim mentioned this pull request Aug 21, 2022
Closed
@junwoo091400
Initial boat airframes cleanup / refactor
d232e99 
- Simulating the ackermann rover mechanism would be best done with the
rover model, but that's not working so well (gazebo model problems)

Add big storm airframe to CMakeLists

Add settings to catamaran airframe

Configure catamaran parameters for better tuning
@junwoo091400
Tidy RoverPositionControl header
96b308e 
- Organize and group the uORB Subscription and Publications

Format RoverPositionControl and tidy

- Use the update_parameters functionality properly (previously, it was
calling this function on every loop of the function, which is obviously
wrong!!)

Small format fixes

Continue refactoring and tidy

Refactor position control and param descriptions for Rover
@junwoo091400
Copy link
Contributor Author

junwoo091400 commented Oct 27, 2022 •
edited
Loading

Rebased on #20082. Now the PR target branch is this PR as well!

@junwoo091400 junwoo091400 changed the base branch from main to pr-rover-ratesp October 27, 2022 14:05
@junwoo091400
Edit attitude / rate control logic in Rover pos control
6288ae9 
Reorder parameters and group them to improve readability

- Previously it was really messy and hard to track where each parameter
comes into play

Minor parameter deocumentation / default value changes

- Units in PID gains for rate control are not so relevant, remove them
(also it isn't strictly rad/s, but rather should be s^-1, and so on
since it essentially applies derivative to the unit dimension)

Add Rover Rate control debug output custom command

- But turns out, the reason I had the boat spinning was because it was a
differential and not an ackermann geometry boat with thruster & rudder
:P

- Also, the feedforward term wouldn't come into effect if I don't set
the rate setpoint to non-zero value. Didn't know that :P

Use differntial boat thruster geometry since it's the only option

- Until a thruster + rudder combination boat model is added in
sitl_gazebo :P

Apply max rate multiplication for acro rate control

Adjust rover max rate parameter value & max range

- Initial values just didn't make sense
@junwoo091400
Implement forward speed based yaw output scaling & debugging uORB topic
d6b4c05 
Apply forward speed lower bound clipping to prevent scalar explosion
@junwoo091400
Adjust SITL boat airframe parameters to satisfy physical constraints
e77990d 
- Setting RATE_MAX to a sane value is important, since controller will
saturate if it can't achieve that rate. For the SITL boat, we need to
use 0.2 rad/s (30 deg/s!!). That is super slow, but what we need to
live with for now.
@junwoo091400
RateControl: Add debug functions to return gains & Integral values
e41a07a 
- For testing
@junwoo091400
RoverPositionControl.hpp: Fix rebase errors
882e009
@junwoo091400
Copy link
Contributor Author

Removed un-removed RoverRateControl files 😓

@junwoo091400
Copy link
Contributor Author

This has diverged quite a lot, it would be better to make #20082 work, then revisit this PR later.

Although I would like to preserve the debug uORB topics and such, but that can be copied over manually and create a minimal diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jaeyoung-Lim Jaeyoung-Lim Jaeyoung-Lim left review comments

Assignees
No one assigned
Labels
Boat 🚤
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@junwoo091400 @Jaeyoung-Lim