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

MAV_CMD_CONDITION_YAW inconsistencies - for relative turns. #23874

Open
AndKe opened this issue May 24, 2023 · 4 comments
Open

MAV_CMD_CONDITION_YAW inconsistencies - for relative turns. #23874

AndKe opened this issue May 24, 2023 · 4 comments

Comments

@AndKe
Copy link
Contributor

AndKe commented May 24, 2023

Bug report

Issue details
This is my test-mission used with SITL:
yawtests.txt

These are my findings:
https://docs.google.com/spreadsheets/d/1FJebVrADxDw2-KrqyvU_h31yHybQQziYpFmSMtuBGWA/edit?usp=sharing

I indicated earlier that I thought Absolute heading changes with "shortest way" worked.
..And it does..

The same cannot be said about relative heading changes that are not even trying to force a particular direction,
but I discovered a very strange behavior on mission item 22, commanding relative -90° from an actual heading of 90° ends somehow up at 180° heading.

The objective
The current heading changes with relative headings are actually working as in my PR mavlink/mavlink#1962 (comment)
which is useful, and intuitive.

The only remaining issue is that there is a bug in how relative headings are changed (with negative number of degrees - which should cause a relative heading to be achieved by turning counterclockwise)

Version
AC (master)

Platform
[ ] All
[ ] AntennaTracker
[x ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

Airframe type
X4

Hardware type
SITL

Logs
see mavlink/mavlink#1962 (comment) for more information.

@hamishwillee
Copy link
Contributor

I can verify from testing that if you're facing 90 degrees (east) and command -90° as a relative yaw you don't end up at zero, but instead at 180.

The problematic code is here: https://github.com/ardupilot/ardupilot/blob/master/ArduCopter/autoyaw.cpp#L116 - as explained in the comment there's a multiplication of two negatives turning the direction into a +90 degree relative turn.

The linked MAVLink PR is to allow a value of 0 on the direction field to make the vehicle head to the relative yaw in the direction with the smallest angle, rather than specifying the direction. Can that be fixed at the same time?

@auturgy this is the bug we were talking about in the mav call.

@rmackay9
Copy link
Contributor

rmackay9 commented Nov 1, 2023

Hi, sorry for missing this. I've added this to the 4.4 issues list.

@rmackay9
Copy link
Contributor

I've looked into this a bit and commented on the MAVLink PR. mavlink/mavlink#1962

Here are my comments again.

I think the confusion on the AP side is because of the target angle (param1) is apparently always suppose to be between 0 and 360. This has led to an inconsistent interpretation of the direction field. In AP, for a relative, CCW rotation (e.g. rotate left) you don't input a negative angle, instead you put in the angle (e.g. 90) and direction of -1 (e.g. CCW).

<param index="1">target angle: [0-360], 0 is north</param>

We could change it but I think AP does allow a user to get what they want. For example if they wanted the vehicle to rotate 270 degrees CCW they would input angle=270, direction=-1.

One bug in AP is that when using mission commands it does not reject negative angles. It does reject negative angles when the commands are received as immediate commands (e.g. within MAVLink COMMAND_LONG or COMMAND_INT messages)

So I guess we have two choices:

  1. fix the target angle description to accept angles in the -180 to +360 range, change the AP implementation
  2. leave it as-is and other stacks adopt the AP method, AP adds the negative target angle check for mission commands

FYI @lthall

@hamishwillee
Copy link
Contributor

hamishwillee commented Nov 15, 2023

I've counter commented here mavlink/mavlink#1962 (comment) - I think we should do "2" but definition to be tidied up in the mavlink/mavlink repo and then pushed down so that it matches with ardupilot/mavlink. Let's clear up any confusion now and make there just be one definition for this.

Why? Because I think the upstream used to match this definition and was changed accidentally, there is no one else implementing this (AFAIK), and it makes sense. I am hoping @rmackay9 will create the upstream PR, but if not let me know and I'll take a shot at it.

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

No branches or pull requests

3 participants