Added "shortest way" as an option for yaw direction in MAV_CONDITION_YAW

[AndKe]

We should absolutely have a "shortest way" turn.
As-is, any imperfection in desired YAW since the last YAW command, risk an almost 360°turn.
Flying side-looking radar, we used MAV_CMD_CONDITION_YAW after each WP set to 120° right turn, if the desired heading is set to 120° at a moment when the actual heading is 120.01°... there will be a 359.99° turn to the right.

Having the option for "shortest way" - the usual way a copter turns during the execution of waypoints, is kind of mandatory.

[hamishwillee]

1. This has conflicts, can you rebase.
2. I like the idea a lot
3. What implementations are out there. What flight stack are you using?

[hamishwillee]

@AndKe PS, we discussed this in the MAV call. Yes, excellent idea. However we do need an implementation to verify that it has no unintended consequences. Also would like to know where this is implemented - grep does not show it in ArduPilot or PX4.

[AndKe]

I am using ArduPilot.
@hamishwillee , this is kind of an hen and egg problem.
The story is as follows: I had trouble with defining left/right turning direction (due to undesired turns)
Then I figured out that setting "0" worked fine (and thought that was already supported)
So I made this PC for QGC: mavlink/qgroundcontrol#10605 (comment)
This PR required this to be a mavlink standard before accepting.
If this option needs already an existing implementation to work... then we have a catch-22 :)

PS: I can't fully understand why "0" worked for me, apparently it is handled the same as "1"
https://github.com/ardupilot/ardupilot/blob/master/ArduCopter/autoyaw.cpp#L116

And I fail to find the logic that denies 359° turn to correct 1° error.

[hamishwillee]

@AndKe Thanks for getting back to me - sorry for the delay in my response.

It is not uncommon for flight stacks to ignore the spec when they see these kinds of values, and rather than condition the behaviour on the specified values -1 and 1 to use something like >0 or <0. This makes sense to the developers because it means they can ignore rounding errors and such that have to be considered in an equality check. We should use enums for these cases to make it more obvious that the values matter.

Looking at the arducopter code, if I specify 0 for:

• a relative angle setting, this will default to clockwise. There is no code to choose minimal direction
• an absolute angle setting, it will obey the direction setting for >0 or < 0. On 0 I THINK it will take the shortest direction. My assumption here is that the wrap_180_cd() method essentially does that for you.

I don't see this change as a problem. i.e. if you send 0 to a system that doesn't obey the value it is non-compliant, but you're no worse off. It isn't like the unintended consequence of making 0 have a valid meaning is that the vehicle does something dangerous.

To progress this:

• I think we should accept this because it is a good idea, and the only likely side effect of sending 0 is that it will make arducopter with a relative angle setting non-compliant.
• While we're here we should perhaps change the values to an enum (?).
• I think we should require a PR on arducopter that makes this compliant with the new spec first - both for relative and absolute cases.

@auturgy @julianoes What do you think?

[julianoes]

I think this makes sense. And I would argue that it likely is already how it is implemented, or ought to be anyway.

[hamishwillee]

@AndKe Discussed in the dev call and agreed this is good. But we don't want to make a change that makes ArduPilot non-compliant.

Can you PR a fix so that if 0 is sent the shortest direction will be taken on the relative angle path too? Once that exists and has been tested I can merge this.

OK?

[AndKe]

@hamishwillee I do not understand exactly what you ask asking me to do:
Parameter three is the turning direction:
direction: negative: counter clockwise, 0: shortest way, positive: clockwise [-1,0,1]

Parameter four decides if the given target heading is an absolute or relative heading
relative offset or absolute angle: [ 1,0]

As long a turning direction is set, (be it CW/CCW or shortest) - there is no difference in the target being an absolute or relative heading.

[hamishwillee]

Hi @AndKe

The message is fine for an absolute angle and makes sense. It is also what ArduPilot appears to implement for an absolute angle.

For the relative case, I am not sure. Looking at the ArduPilot code it looks like if you send the command to ArduPilot with a relative angle and direction 0 it will always go clockwise. If we change this message definition as you have suggested, won't that then mean that ardupilot no longer complies with it for the relative case?

So the request was that you should prototype a fix to the ArduPilot code. Does that make sense now. If not, sorry, I must be confusing myself.

[hamishwillee]

@AndKe Did my response make sense?

[AndKe]

@hamishwillee Yes, now I understand what you meant.
Sorry for the late response.
Ok, I see, currently it is so that it always goes left when changing to a relative heading.
My request for change means then the handling must respect the left/right/shortest direction directive regardless of being a relative, or absolute heading.
I will try to find time to fix it, but I must admit that I am not confident that's within my skill level.

[hamishwillee]

@AndKe Thank you - yes, you are correct.

My experience with the ArduPilot dev team is that as long as you do your best and show evidence of testing, they will be supportive. If you get stuck, ping me.

[AndKe]

Hi @hamishwillee
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?

Do you have any clue about what's going on here?

[hamishwillee]

Maybe! So
The lines of interest are:

mission item	new ABS heading - Relative heading starts with REL. Param1=Angle , Param4: ABS=0 , REL=1	turn direction Left, Right, Short stored in param 3 -1=Left, 0 Shortest, 1=Right	expected rotation to heading	observed	result/notes
20	REL +90	R	R 90	R90	ok
22	REL -90	L	L 0	R180	Why?/How?

Is the column with R180 "absolute"? I.e. after this it is facing down south?

In that case the behaviour is clear from code.

    if (relative_angle) {
        _fixed_yaw_offset_cd = angle_deg * 100.0 * (direction >= 0 ? 1.0 : -1.0);
	
    } 

Where direction = -1 (L), angle_deg = -90
That means your offset is

	_fixed_yaw_offset_cd = -90 * 100.0 * (-1) = +90

Since you're already at 90 this is a right turn by 90 degrees right - to 180.

If you mean that it went from 90 (relative to North) rotating right by 180 to absolute -270, then I don't understand the behaviour.

[hamishwillee]

PS Sorry for delay, I only work Weds, Thurs on this stuff.

[AndKe]

Mission item 18 sets heading to 0
Item 20 commands relative heading 90° (annotated as REL90)
The machine turns right from 0/360 to 90° as expected
Then item 22 is executed, commanding a relative turn of -90 ..I would now expect it to turn left from 90° to 0° ..but instead it ends up at 180° (as if the -90 were changed to 90)

If you could run the test in SITL yourself... It would be obvious. The SITL location is "snarbyeidet"

[hamishwillee]

Yes, just tested. I can see that it does as you say, and that is in line with my comments in #1962 (comment)

Specifically, yes, if I put in -90 I would expect it to go from 90 to zero, but the maths above is a negative times a negative, resulting in a +90 offset. It's IMO a bug. Can you cross post to ArduPilot issue repo?

[hamishwillee]

@AndKe If not, let me know so I can add a bug myself "in time"

[AndKe]

@hamishwillee I am sorry to have forgotten - thank you for reminding me. I've done it now: ArduPilot/ardupilot#23874

[hamishwillee]

No worries at all - thank you for chasing up. Hopefully we can get some eyes on that.

[AndKe]

@hamishwillee Please consider merging "shortest way" - after all, it works. Maybe then we can have some drive to fix the one bug with relative turns(which is less common way to do it in a mission , as the result is less precise.)
..Or at least include it in a devcall ?

[hamishwillee]

I will add it to the dev call to discuss.

[AndKe]

@hamishwillee thank you :) The gist is: That except for "relative turn" in a particular direction from a particular heading, the feature (shortest way heading to absolute heading) already works as expected. - and just needs to be documented as such. I see no significant use for relative turns, as those would only accumulate any error from the point of command.

[hamishwillee]

@AndKe You're right, but from a strictly accurate point of view it does make ArduPilot non compliant. Now @rmackay9 has picked this up to look at in the next version I think we might be able to wait. Personally I'm inclined to merge it - but I will see what the team think next Wednesday.

[rmackay9]

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

[AndKe]

I have never considered a negative number of degrees being a valid number, simply because there are direction alternatives for both CW and CCW.
If one is entering an absolute heading then 0-360 - makes sense.
if one would enter a relative turn where CCW/CW is specified, then negative makes no sense
A negative number of degrees would only make sense if we had one field for degrees, and only option for "absolute heading" or "relative heading" - in that case, unspecified relative heading could very well be a negative number (to turn left)
I hope we can get this word-splitting issue solved so that the "shortest direction" - which is the most useful, and sane thing for most flights - can go ahead :)
Right now, it does not make it into QGC due to the simple reason of not being defined here - even if it works as expected.

[rmackay9]

nitpick: I think "shortest way" should be renamed to "shortest direction". "way" and "direction" are synonyms in this case so we don't need to introduce a new word.

[rmackay9]

OK, so if the plan is to stick with only positive angles then the direction=0 option is only useful when an absolute heading is used. When relative angles are used we would continue to interpret "0" as the same as "1" (e.g. positive/clockwise).

@AndKe I'm still slightly confused that you say, "I have never considered a negative number of degrees being a valid number.." when the AP bug report you raised specifies the issue occurs when negative angles are used. I suspect you mean that absolute angles should always be positive (e.g. 0~360) but relative angles could be negative when the direction=0. If this is what you mean then the relative angle range would be -360 to +360 if direction = 0. If direction is non-zero then the relative angle range would be 0 to 360. I'm OK with that but it's tricky.

[AndKe]

@rmackay9 yes... you perfectly pinpointed my confusion .. I must admit that I am clearly confused too :)
It's been a while I worked on this, based on "memory" or "wishful thinking" about how it should be, what I wrote was clearly inconsistent .. disregard,

As for the "nitpick" way/direction - I totally agree with you.

[hamishwillee]

Firstly, I am very confused. MAVLink common head revision looks like this:

It does not look like <param index="1">target angle: [0-360], 0 is north</param> so I'm not sure how it looks like that in this PR but it doesn't look like a "Change" in the compare view.

It used to match the behaviour until @amilcarlucas stripped it out a few years ago in #1105:

<param index="1" label="Angle" units="deg" minValue="0" maxValue="360">target angle, 0 is north</param>

OK, so if the plan is to stick with only positive angles then the direction=0 option is only useful when an absolute heading is used. When relative angles are used we would continue to interpret "0" as the same as "1" (e.g. positive/clockwise).

PX4 does not implement this so I think we can choose any of the options that makes sense.
To me that is as you have suggested, leave it as it is.

shortest direction is much better.

However I propose we do that in a new PR starting off current Mavlink/mavlink master. Add back in the minValue="0" maxValue="360". Add some words to note how the directions should work.

Then we make this align in Ardupilot version of mavlink/common.xml too so that they are identical.

@rmackay9 If you agree, do you want to create the new PR or should I?

[hamishwillee]

@AndKe @rmackay9 I have taken the information here and created #2058 which has no conflicts and which I believe matches our current discussion. Closing this but we can still refer to it if needed.