common: Add new attitude target typmask touse YawRate, Pitch and Roll. #2013
Conversation
…rnion can be ignored and to use yaw rate instead
PeterQFR
changed the title
|
Link to #1302 |
|
@julianoes @hamishwillee only real question is consuming the bit, but I'm happy for this to go in. |
There was a problem hiding this comment.
While I agree with the utility of the use case, it is concerning for me that the added flag is used to configure autopilot behavior and not define the setpoint definitions. IMHO, the type mask of the ATTITUDE_TARGEt message should be used to clarify what the information in the message is, rather than configure behavior on the autopilot side.
The attitude quaternion in the ATTITUDE_TARGET message is supposed to be defined as the orientation of the body frame relative to the inertial frame. However, this PR seems to imply that this attitude field is now relative to a yaw-free attitude frame. (which this frame is probably undefined in the mavlink spec)
- This potentially makes the definition of the attitude field more ambiguous, since now it can use two different frame references. If someone wants to define a yaw relative attitude (just pitch and roll with the current yaw), which should be a subfunction of this PR, would be missing.
Isn't the real problem that we are missing a coordinate frame that is yaw relative, in the MAV_FRAME enum and that we are missing a coordinate frame field in the ATTITUDE_TARGET message?
|
No this flag is being used appropriately. It is describing what parts of the message are valid. "Use the quaternion but ignore yaw part. Yaw rate is valid instead." We can make that more explicit to what is valid and not, that would require the removal of the term USE. The reason for this PR is because the above use-case of the message is not defined. We had been using quaternion and Yaw rate and implicitly ignoring Yaw, but this PR makes ignoring Yaw in the quaternion explicit. The attitude field is not ambiguous the frame doesn't change except for the ref that the user of this mode doesn't care what Ardupilot (in our case) has set for Yaw. The problem in the flight controller is that in GPS denied environments, the yaw component is arbitary. I don't know of any frame convention that sets a relative component. They are all usually relative to some fixed frame. This use case is for when you don't care what that fixed frame is, or are otherwise tracking it elsewhere. |
|
Potentially we could rename this flag to just specify that YAW in Quaternion is invalid. Something like ATTITUDE_TARGET_TYPEMASK_IGNORE_QUATERNION_YAW Then we can use the existing ATTITUDE_TARGET_TYPEMASK_BODY_YAW_RATE_IGNORE flag to specify that yaw rate is VALID. |
@PeterQFR This is exactly what I think is problematic. The current message is defined so that quaternions are always defined as the orientation between the inertial and body frame. This PR effectively allows the use case that the fixed frame is another frame, where the yaw is aligned with the current yaw axis. I understand why specifying yaw rate and relative roll/pitch attitude can be useful. I just think the way this PR adds a behavioral change is problematic. The way it is done in this PR potentially compromises the integrity of the message definition by making the meaning of the attitude alter between different frames. |
|
This PR doesn't change the frame. What it says is that the Yaw specification of the quaternion is to be ignored. The yaw rate remains valid. Both are about the same frame. The first is an angle, which we ignore, the second is an angle rate which we use. The frame doesn't change, z is still down. @Jaeyoung-Lim In reality all we want to do is specify the aircraft attitude in MAV_BODY_FRAME_FRD, and ignore the yaw component of the quaternion and provide a rate. Its as simple as that. If thats your main concern, then add an additional field in SET_ATTITUDE_TARGET to specify the MAV_FRAME. |
Then you say:
You are changing the frame. You are just doing it in a hacky way. |
|
Yeah I was thinking on the fly. To recover a pitch and roll value doesn't require a yaw, and there is no documentation of what the frame is for set attitude target. So the next thought is well, the SET_ATTITUDE_TARGET should have actually specify the frame, it doesn't at the moment, except by some convention in some developers head. |
|
@PeterQFR "Ignoring yaw" effectively means that you want to interpret the attitude quaternion relative respect to the yaw aligned axis. This opens up possibilities of missing interpretations: I still think adding a type mask to induce behavioral change of a downstream software makes the message definition more and more confusing and incomplete. The message definition becomes incomplete without the downstream application, which IMHO is what we dont want. Why does this need to propagate into mavlink? The mavlink message already allows you to specify attitude and yaw references together. Why not ignore yaw on the downstream application side, if a valid yaw rate is defined? OR if we are hitting a fundamental problem where the message defined the attitude between two fixed frames, we should try to solve the real problem. |
|
This is a valid use case
We have a blocker @lthall on a Copter PR #24033 who seems to think we cant do that if its isn't directly specified in the messages (ie to ignore the yaw component of the Quaternion). So we're specifying it in the message, as previously we were doing exactly as you suggested. You've made valid issue around the Mav frame being specified, which is true, it is unclear what frame is being used when first using the SET_ATTITUDE_TARGET. Mavlink would be enhanced by allowing different frames to be specified, particularly a body centric frame. But as you've identified, this PR is about the message not the implementation on the FC. People who care will gradually upgrade the stacks to support the additional information, but as far as I can see this message type wont break existing implementations. |
message_definitions/v1.0/common.xml
Outdated
| @@ -3912,6 +3912,9 @@ | |||
| <entry value="4" name="ATTITUDE_TARGET_TYPEMASK_BODY_YAW_RATE_IGNORE"> | |||
| <description>Ignore body yaw rate</description> | |||
| </entry> | |||
| <entry value="16" name="ATTITUDE_TARGET_TYPEMASK_IGNORE_QUATERNION_YAW"> | |||
| <description>Ignore yaw in attitude quaternion. Use yaw rate instead</description> | |||
There was a problem hiding this comment.
@Jaeyoung-Lim here we have limited the flag to ignore only the yaw in quaternion. Use the rate flags for specifying what you want to listen to.
Ah, no. That isn't what I said. But it is a serious consideration when you realise this will be difficult for us to ever remove from the code. I actually suggested the best way to solve this problem would be to create a carefully thought out Mavlink message that does have the features needed. I also pointed out that with a little addition and subtraction you could use the current message without any need to change it. |
|
I'm not discussing the ardupilot #24033 here. |
Oh, sorry. I think you posted this in the wrong PR then:
Just to be clear, this PR has not been put forward with the support of ArduPilot developers and PeterQFR is not an ArduPilot developer. So when PeterQFR says "we're specifying it in the message" He is talking about his own branch of the code. When this was last discussed in the ArduPilot developers meeting the problems associated with changing the message or the message interpretation were recognised. The consensus appeared to be that a new message would be the least problematic option but the ArduPilot developer team are very open to a simpler or quicker solution if it can be done in a reasonable way. |
@lthall this is an open PR to discuss a quicker solution which is to modify this message which already has a codepath into attitude control in ardupilot. This PR had already been approved before I made further changes to satisfy concerns by @Jaeyoung-Lim which require approval. Please add you constructive modifications here (if any). A new message is not a good idea because it has to be plumbed through the comms stack to the attitude controller, and provides nothing that can't be handled by SET_ATTITUDE_TARGET of which several open PRs in Ardupilot exist to implement this same functionality and none of these PRs use a new message type. Some of these PRs have been open for years, so if you're a developer and you think that's okay, then there's not much I can do about it. Other than maintain our own branch and not bother providing PRs to Ardupilot in the future. Its not that hard. |
You mean because it is too much work for you and you are only willing to provide the quick easy and compromised solution.
An approval by one person means only that one person doesn't see a problem with it. Each developer brings different knowledge to a problem and before a PR is merged it will need to be approved by a number of people. You may only see a small fraction of those approvals listed at the top of a PR. The approval process is just one barrier to people trying to railroad changes into code bases used by many thousands of people and to try to ensure that changes reach a minimum standard.
This is not ChatGPT. I personally don't think there are any changes to your PR that would make this approach correct. You seem to think that just because you want to do something it is up to everybody else to make it happen, even if it is a bad idea.
Yes I do. Not only Ok but desirable. These PR's are left open to highlight that there may be a need here, even if it is only a transient capability that people use before they work out how to do it properly. These PR's are not accepted because they do not reach the standard required for various reasons.
Given your attitude and refusal to explore all options, I suspect that is probably the best approach you could take. |
|
I actually don’t want to implement your suggestions because I think they
are not very good.
You are literally the only person I have encountered that has a problem
with this approach. Please anyone else speak up to support @lthalls
position.
There are two outstanding PRs including mine that are functionally
identical. Theres another that did the same thing but used different ways
of identifying the mode.
And others on this PR that want this exact functionality and have done for
years.
No one wants to clumsily implement a new message for the reasons explained.
No one wants to track ardupilot yaw offboard because it leads to a bunch of
hacks to ensure they remain consistent and are always subject to lag due to
transmission on an off the flight controller.
…On Mon, 26 Jun 2023 at 12:15, Leonard Hall ***@***.***> wrote:
A new message is not a good idea because it has to be plumbed through the
comms stack to the attitude controller,
You mean because it is too much work for you and you are only willing to
provide the quick easy and compromised solution.
This PR had already been approved before I made further changes to satisfy
concerns by @Jaeyoung-Lim <https://github.com/Jaeyoung-Lim> which require
approval.
An approval by one person means only that one person doesn't see a problem
with it. Each developer brings different knowledge to a problem and before
a PR is merged it will need to be approved by a number of people. You may
only see a small fraction of those approvals listed at the top of a PR. The
approval process is just one barrier to people trying to railroad changes
into code bases used by many thousands of people and to try to ensure that
changes reach a minimum standard.
Please add you constructive modifications here (if any).
This is not ChatGPT. I personally don't think there are any changes to
your PR that would make this approach correct. You seem to think that just
because you want to do something it is up to everybody else to make it
happen, even if it is a bad idea.
Some of these PRs have been open for years, so if you're a developer and
you think that's okay
Yes I do. Not only Ok but desirable. These PR's are left open to highlight
that there may be a need here, even if it is only a transient capability
that people use before they work out how to do it properly. These PR's are
not accepted because they do not reach the standard required for various
reasons.
Other than maintain our own branch and not bother providing PRs to
Ardupilot in the future.
Given your attitude and refusal to explore all options, I suspect that is
probably the best approach you could take.
—
Reply to this email directly, view it on GitHub
<#2013 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABIBV442JPWVBNIUSFZJOKTXNDWDZANCNFSM6AAAAAAZS4TUIU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I haven't made any suggestions. I have simply tried to explain why this is more difficult to do than you realise. I have also tried to explain the other options that have already been discussed. I actually think the concept of interpreting the ignore roll and pitch rate but not yaw is probably the least bad solution without implementing another message tailored to this form of control. Some sort of Euler based control, or adding a frame specification to this message. But that requires open constructive minds. The problem with the ignore roll, pitch rate but not yaw rate, is this has real implication for people that are already using this message. This could cause existing users to crash. But you are too focused on getting what you want to listen.
This should indicate to you that there are real issues here that are serious enough to not put them in. If you took the time to understand them you may get further. I am a busy person and wouldn't waist my time preventing you from bulldozing this through if I didn't think there was a real cost that needed to be evaluated and accepted by the community.
Yes, this maybe too much work for undergraduate level projects and simple concept demonstrations. However as soon as you start trying to do this at a basic professional level you discover that your maximum performance is limited by yaw controller errors (among other issues) that you can never address as well off board as you can by working with the autopilot directly. I completly understand that it is important to provide a basic interface that helps people get started, and learn about the limitations of this approach. The question is how best to do that and as you can see from the failed PR's that is still an open question. It would help dramatically if the people responsible for those PR's would discuss the issues and options with an open mind. |
|
Okay well this PR is about the message. @lthall will the proposed changes to this message cause people to crash? |
|
Sorry, you keep referring to the ArduCopter PR's in the Mavlink Repo.
Potentially yes. But it would be because they were confused by the swap of reference frames. But this has been pointed out to you numerous times. The point is who was responsible for the crash:
The user is at number 1 so that is a good thing. 2 is irrelevant as anybody can make a PR. At number 3 and 4 the Mavlink and Autopilot developers must be comfortable that they have made well thought out and responsible decisions. In general I don't think the risk of a crash is unreasonably high but my input only has weight at 4. I don't think safety is really the main consideration here. I think it is clarity and quality of the message, as pointed out above by others with more Mavlink experience than me. I have been impressed with the insight and flexibility of the control based Mavlink messages so I will be interested to see the thoughts of the Mavlink Dev's. So far they have had some good suggestions I didn't think of. No easy fixes but good suggestions. |
|
@auturgy @Jaeyoung-Lim @Ryanf55 Are there any concerns/comments with the additional fields added to message to specify frame for attitude and limit the flag to just indicate ignoring yaw? |
I wouldn't consider myself having a stake in it; I'm focusing on the ROS 2 interface, which you can take a look at here: Instead, we follow the frames outlined in ROS REP 105, and it doesn't have anything to do with Mavlink. I'm mainly just following along to make sure the interface I'm developing can support the use case of control needed here that Mavlink currently doesn't support. How exactly you solve it in Mavlink- I have no preference. |
|
@Jaeyoung-Lim still holds the view that while this is the quickest way for someone to implement that feature for an autopilot, but it introduces confusion on the MAVLink level. My take on this is that:
I am supportive of getting this in, if we can do so in a way that is clear, doesn't break anyone else. What I'm hearing though is that this isn't right, not "how we might adjust it to address those issues", or "it is impossible to adjust it to fix these issues". The body fields and thrust are explicitly in the body frame, so the frame only applies to the quaternion. If we set the frame we really need some way to tell consumers what frame is in use and make sure that this defaults to the body frame. @Jaeyoung-Lim Is there anything that you can suggest that would remove the perceived confusion? I'll add to the MAV call next week. @auturgy has expressed support and is usually present. |
|
Thanks, I probably wont make the catchup. The fact that the message contains multiple different frames for different parts of the message seems the most problematic part. Its not clear in the message that the quaternion isn't relative to the level body frame. All the fields are there to encode the data (pitch angle, roll angle, yawrate) that we want, we just need something like a flag to indicate that is what we intend in the message. That being said, it might also be possible rely on the thrust body vector which would not include a yaw component. Though if you confirm that the thrust body is in the FRD and not the NED frame as stated in the comment that would be great. It is also not clear what the norm of that vector represents. (In the comments thrust but in what units? Can I just multiply my thrust value scaled 0 .. 1 by a unit vector that encodes my pitch and roll?). But using thrust vector isn't as clear as a flag to just ignore the yaw in the quaternion and treat attitude as body level frame which is another option for a flag. |
|
Thanks @hamishwillee,
I feel confident in saying that the ArduPilot developer team is keen to support this control combination. I think the team recognises the problems with adapting the existing message. Ideally we would like a solution that is clear enough to be accepted universally in the Mavlink community.
I think you hit the nail on the head here. I am hopeful that if we can get more eyes on this problem there may be an elegant solution or an elegant alternative message that can be proposed. At very lest we can say we made every effort to find an elegant solution before accepting a compromised solution or not supporting the control combination at all.
@PeterQFR I considered this but this also has some problems. First up this vector is unambiguously defined in NED. Secondly there are two possible interpretations here based on weather this should define just the thrust direction, or the thrust direction and magnitude. My current interpretation of this message is that it represents both magnitude and direction. Getting off topic but I don't believe the thrust vector can help us here as it has the same basic problem as the Quaternion being defined in NED. It would be great if we could specify MAV_FRAME_LOCAL_FRD but that field isn't there. I appreciate any help or suggestions the Mavlink team can provide. |
|
As a good example, the ROS family of interfaces have mostly solved this issue, by not making msg fields frame specific but by designating a whole message to be in a specific frame, usually by designating a frame field in the message. That would probably a require a refactor of Mavlink in general or at least a new dialect. |
I think that is basically what has happened with the attitude message, it is NED. This is why creating a new message specifically targeted at the requirements of an attitude message in forward, right, down or something similar may be an attractive option. Users may also prefer the quaternion be replaced with roll, and pitch. |
Absolutely. As @lthall said this is a well defined message you're touching designed without any of the confusion that adding frames can create. Frames are the bane of my life. On the face adding a frame looks great - one message that does everything irrespective of the frame you want to use it for.
I think it is NED. Is that what @lthall said. @lthall I would suggest that the MAVLink team are not the right people to champion this. We're more of a checkbox to make sure that stuff goes into the standard that can be standardized. Personally I know nothing - my role is to try build consensus. Messages should be proposed and tested by the people who are likely to have to implement them, and who have some expertise in the domain. That would be you and @PeterQFR, and possibly @Jaeyoung-Lim . I can find some more advice too. That said, if you have feelings about what this message might look like it would be worth jotting them down. That will far more likely result in something that you won't hate. I appreciate your time is not free, but if you think you will be involved in this use case eventually, being involved now will reduce the ultimate cost IMO. Or to put it another way, if we can't build consensus around this message, we should proactively work to build consensus around another. Otherwise there will be push to take the path of pragmatism (accept the changes), which makes me uncomfortable. |
I completly agree. I am happy to spend the time to work through a specification driven decision making process but I am not willing to do so without defining a set of requirements based on the actual users of this message. So far the 3 users that I am aware of have been unwilling to discuss any option other than the quick and dirty hack, much less help with the work of providing a specification.
Maybe not but by bringing it to your attention you may be able to find people in the Mavlink community that have some insight.
That would make me extremely uncomfortable and to be honest I wouldn't want to be a part of that. I have been working under the assumption that there would be significant rigour put into any mavlink message changes that are accepted and that it was therefore worth attempting to stay complaint with the mavlink specification. If some random user representing a user base of only 2 or 3 people can bulldoze changes to the frame definition of a mavlink message then the responsibility falls back to the Autopilot firmware to decide if that confusion should be allowed back into our own code base. I believe this situation should be avoided. I would prefer that ArduCopter provide a creative interpretation of the existing message using the rate limit flags to support this feature than make an existing and well defined and thought out message permanently confusing and self contradictory. The best option would be to create a message specifically for this basic/introductory control strategy if the user base was interested enough to help create it. So far the user base does not appear to be willing or able to support this effort but sooner or later that will change. --
|
And therein lies the reason we are doing most of our navigation off board. I've adjusted the flags and removed the frame. I've pushed what I think will allow a reasonable user to define how they would like to interpret the attitude and they can use that to indicate this use case. A check on that flag should avoid confusion and executing the wrong code path. If there's a better description happy to hear about. NB as an aside I typically associate NED with inertial frames and FRD with body frames/level body frames. So body in NED doesn't make any sense to me. |
Classy.
Now this is a very interesting observation!!!! The mavlink message calls these "Body roll rate", "Body pitch rate" and "Body yaw rate". It would make perfect sense to interpret these rates as being in the body frame, not NED. In fact when only these rates are provided ArduCopter does interpret these as body frame rates. To get fast and precise attitude control you have to command both attitude and rotation rate or you are constantly dealing will following error. When I implemented this message I assumed these rates to be NED angular velocity vector to match the Quaternion and thrust vector frame. Now I am wondering if that was the wrong interpretation. Perhaps those rates should have been transformed from the body frame (defined by the quaternion) to the NED frame first........ This is why it is so important to try to make these decisions with someone that is actually using these messages. Perhaps this message has not been as clearly defined as I first thought or maybe I interpreted it incorrectly when both attitude and rates are being specified (I am starting to think I messed up). I am looking forward to talking to @Jaeyoung-Lim about this when he has time. If I didn't mess up then it would suggest that the interpretation of this message is more flexible than I initially thought and changes in flag settings could be used to change frames. If I DID mess up then the frames are rigidly defined and shouldn't be changed. Both options still suggest this PR is a bad idea. |
|
I have discussed this with Randy we both feel that the only way forward is a new message that caters specifically to this use case. As such I don't believe ArduPilot will support the proposed change to this message. We also think that our current interpretation of the existing message should be fixed to only accept rates in the aircraft target body frame (not NED angular velocity vector). This will be a further move away from supporting the proposed changes to this message. |
…faults to inertial) and add flag to ignore yaw in the message
|
Reviewing the message, the only thing that has a specific frame set in either the comments or the name is the thrust_body and rates (confusingly set to the NED frame). According to the Message definition as it is now written, there is nothing to say that the ATTITUDE needs to be in the NED or FRD (level) frame other than implementation convention. As we've observed the current message is confusing even to long time users such as @lthall Therefore to clarify the message a FLAG is required to set all values to either an inertial frame (NED) or a body frame (FRD). The rates and attitude can be specified in either frame but not both (ie mixing frames). By default the flag would be not checked so would be by default NED frame, which should not break the 'existing' NED convention. I think also a flag to ignore yaw in attitude would be helpful, particularly for my use case or any other egocentric control methods. Users may think that yaw is already ignored in an FRD control, but as this message could be used to set say a Gimbal or other payload (through the use of the target_component field) then yaw could be valid in an FRD use case. I could also remove all reference to body in the message fields, so that they are attitudes or rates. Frames aren't set by convention or a string in the name field but by whatever frame set by the typemask flag. However changing field names would break compilation immediately, for everyone. @rmackay9 FYI |
Randy and I have made the decision to correct that misinterpretation and to ensure that this message is maintained strictly to the current specification. There will be three combinations of inputs:
No other Attitude combinations will be supported at this time. |
|
I had a great chat with @Jaeyoung-Lim and he suggested that the desired behaviour could be achieved using the POSITION_TARGET_LOCAL_NED if we added support for MAV_FRAME_LOCAL_FRD frame. https://mavlink.io/en/messages/common.html#POSITION_TARGET_LOCAL_NED We could support the input where only X and Y acceleration are not ignored. Then Alt_Hold could be controlled with rate and/or position. Yaw can be controlled with rate and/or angle. This is a much more elegant and appropriate solution to this problem. |
|
@lthall @Jaeyoung-Lim Thanks for making a constructive suggestion that provides a way forward (hopefully) for @PeterQFR Peter - can you work with #2013 (comment) ? |
|
@PeterQFR Do you think you will choose to implement this new proposal, and if so, will you do it in a new PR or over the top of this one (so the full history exists). I'd like to close this as "a compromise path forward" has been found. I don't plan to merge the current PR. |
|
@hamishwillee Hi hamish, at the moment there are no plans to change to the other way which I believe centers around specifying accelerations via a position message. Our interface is roll, pitch, yawrate, thrust from our side and implementing something else is not a priority right now. We will maintain our own branch for the moment as that is preferable to an extended argument over github. |
The new proposal is already fully supported in Mavlink without any changes required. It is also more flexible so has less compromises than this proposal.
I think you can safely close this with "a less compromised path forward has been proposed". |
|
I don't think there's any compromise here, they are two completely different interfaces... doing similar things, you'd have been able to support this interface too if @lthall was not such a difficult child, this PR would also not have been required. At the end of the day, the system can support both interfaces, I'm sure not the only user maintaining my own branch. |
You set the tone mate, I just followed the tune. |
|
I believe you first accused a complete stranger of being lazy.. which I think you've edited as I would have linked it here but I definitely remember it ... because they didn't want to implement your proposal to define a new message for the proposed interface. In fact it your proposal was so good that you aren't even implementing it.. your proposing something different. I believe that set the tone... this literally has been the worst interaction I have had with an opensource project, be respectful in future. Thanks. |
|
Knowing you both, I know you're both better blokes than the interaction here has demonstrated. I hope we can move past this and work towards a productive relationship in the future. |
LOL. I have no reason to delete anything I write, other then correcting poor grammar or spelling. You are imagining a non existent justification for your own behaviour. Own it mate.
I never made any proposal before the issue I created. I tried to discuss options and define problems to solve, in a hope that a good solution would be forthcoming. Maybe you "definitely remembered" that inaccurately too, or maybe I edited all those messages as well :)
I suspect this is simply because this time you didn't get to bulldoze your solution through. The simple truth is your solution was not up to standard, and you refused to work with others to find a solution that could work. I was just the developer tasked with dealing with you.
You need to earn respect, it isn't a participation ribbon. All you have any right to expect is people review your contributions constructively with an open mind and desire to define and solve the problem. In this regard I think I did a significantly better job on your PR than you did. |
|
What @auturgy said. It really doesn't matter who said what first: we're all grown ups and should try assume the best of each other even if when we feel they are frustrating and annoying (and yes, easy to say when I'm not the one on the receiving end). This kind of conversation is contrary to the code of conduct. Locking the thread. |
…rnion can be ignored and to use yaw rate instead
This is needed for quadcopters to use this message in a mode where attitude pitch and roll in the body frame is important, but yaw control is being done offboard. Therefore we just want the system to respond to yaw rate.