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.

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

mavlink: fix offboard velocity input #17958

Merged
merged 1 commit into from
Jul 22, 2021
Merged

Conversation

julianoes
Copy link
Contributor

This reverts the behavior for offboard velocity setpoint.

Back in v1.11, the velocity input in NED_BODY was assumed to be in the world frame but rotated by yaw to the vehicle frame. With the current state the frame is completely fixed to the body. While this might be
technically correct, it doesn't seem much intuitive for multicopters and breaks the MAVSDK offboard velocity API.

So as an example, with a velocity setpoint of 5 m/s forward, the drone
would in

  • v1.11: fly forward with 5 m/s
  • v1.12: start to fly forward by pitching down but then descend rapidly as the forward velocity would translate to a setpoint in Z into the ground as it is pitched down.

This commit restores the behavior to what we had previously.

This problem was found by @farhangnaderi in https://px4.slack.com/archives/CL3LV4FUH/p1626820069036900.

This reverts the behavior for offboard velocity setpoint.

Back in v1.11, the velocity input in NED_BODY was assumed to be in the
world frame but rotated by yaw to the vehicle frame. With the current
state the frame is completely fixed to the body. While this might be
technically correct, it doesn't seem much intuitive for multicopters
and breaks the MAVSDK offboard velocity API.

So as an example, with a velocity setpoint of 5 m/s forward, the drone
would in
- v1.11: fly forward with 5 m/s
- v1.12: start to fly forward by pitching down but then descend rapidly
  as the forward velocity would translate to a setpoint in Z into the
  ground as it is pitched down.

This commit restores the behavior to what we had previously.
@julianoes julianoes requested review from dagar and bresch July 21, 2021 09:02
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

I think this will confuse people expecting that the velocity reference is actually in the body frame.

Doesn't this mean that we are not following the mavlink standard anymore?

@julianoes
Copy link
Contributor Author

I think this will confuse people expecting that the velocity reference is actually in the body frame.

It confused me but in the opposite way 😄.

Doesn't this mean that we are not following the mavlink standard anymore?

I don't know what the standard exactly means but this change just reverts to what we did in the past. Users of MAVSDK will crash into the ground when upgrading from v.1.9/v1.10/v1.11 to v1.12.

@julianoes
Copy link
Contributor Author

julianoes commented Jul 21, 2021

I will add the other frames back in after this PR. For now let's leave just this fix, so that we have it to cherry-pick into the point release.

@farhangnaderi
Copy link
Contributor

I will checkout this for now then to make my actual drone work with MAVSDK. No intention for ROS though.

@julianoes julianoes merged commit 11480cd into master Jul 22, 2021
@julianoes julianoes deleted the pr-fix-offboard-velocity branch July 22, 2021 07:40
@julianoes
Copy link
Contributor Author

julianoes commented Jul 23, 2021

@Jaeyoung-Lim should we do the same change for the acceleration setpoint? I'm a bit confused there.

I guess we don't actually support acceleration setpoints, right? We only support x,y, rates and thrust?

@Jaeyoung-Lim
Copy link
Member

@julianoes We actually do 😅 : #16498

The full list of supported offboard setpoints are documented here: http://docs.px4.io/master/en/flight_modes/offboard.html#copter-vtol

@julianoes
Copy link
Contributor Author

@Jaeyoung-Lim ok nice. I'm just trying to figure out how to handle the various frames.

@farhangnaderi
Copy link
Contributor

Fixed.
https://review.px4.io/plot_app?log=fa0acc23-3d2e-47f9-87f1-44b00b927813

@dagar
Copy link
Member

dagar commented Aug 3, 2021

Thanks for the follow up @farhangnaderi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants