-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Added "shortest way" as an option for yaw direction in MAV_CONDITION_YAW #1962
Conversation
|
@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. |
I am using ArduPilot. PS: I can't fully understand why "0" worked for me, apparently it is handled the same as "1" And I fail to find the logic that denies 359° turn to correct 1° error. |
@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 Looking at the arducopter code, if I specify 0 for:
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:
@auturgy @julianoes What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense. And I would argue that it likely is already how it is implemented, or ought to be anyway.
@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? |
@hamishwillee I do not understand exactly what you ask asking me to do: Parameter four decides if the given target heading is an absolute or relative heading 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. |
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. |
@AndKe Did my response make sense? |
@hamishwillee Yes, now I understand what you meant. |
@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. |
Hi @hamishwillee These are my findings: I indicated earlier that I thought Absolute heading changes with "shortest way" worked. The same cannot be said about relative heading changes that are not even trying to force a particular direction, Do you have any clue about what's going on here? |
Maybe! So
Is the column with R180 "absolute"? I.e. after this it is facing down south? In that case the behaviour is clear from code.
Where direction = -1 (L), angle_deg = -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. |
PS Sorry for delay, I only work Weds, Thurs on this stuff. |
Mission item 18 sets heading to 0 If you could run the test in SITL yourself... It would be obvious. The SITL location is "snarbyeidet" |
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? |
@AndKe If not, let me know so I can add a bug myself "in time" |
@hamishwillee I am sorry to have forgotten - thank you for reminding me. I've done it now: ArduPilot/ardupilot#23874 |
No worries at all - thank you for chasing up. Hopefully we can get some eyes on that. |
@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.) |
I will add it to the dev call to discuss. |
@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. |
@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. |
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).
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:
|
I have never considered a negative number of degrees being a valid number, simply because there are direction alternatives for both CW and CCW. |
@@ -758,8 +758,8 @@ | |||
<description>Reach a certain target angle.</description> | |||
<param index="1">target angle: [0-360], 0 is north</param> | |||
<param index="2">speed during yaw change:[deg per second]</param> | |||
<param index="3">direction: negative: counter clockwise, positive: clockwise [-1,1]</param> | |||
<param index="4">relative offset or absolute angle: [ 1,0]</param> | |||
<param index="3">direction: negative: counter clockwise, 0: shortest way, positive: clockwise [-1,0,1]</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
@rmackay9 yes... you perfectly pinpointed my confusion .. I must admit that I am clearly confused too :) As for the "nitpick" way/direction - I totally agree with you. |
Firstly, I am very confused. MAVLink common head revision looks like this: It does not look like It used to match the behaviour until @amilcarlucas stripped it out a few years ago in #1105:
PX4 does not implement this so I think we can choose any of the options that makes sense.
However I propose we do that in a new PR starting off current 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? |
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.