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

Yet another effort towards unified gimbal device messages #1860

Closed
olliw42 opened this issue Jun 28, 2022 · 44 comments
Closed

Yet another effort towards unified gimbal device messages #1860

olliw42 opened this issue Jun 28, 2022 · 44 comments
Assignees

Comments

@olliw42
Copy link
Contributor

olliw42 commented Jun 28, 2022

Hey Folks, especially Julian (@julianoes)

quite some time ago we had extensive discussions on gimbal protocol v2, and as a result the gimbal device messages resulted. Unfortunately, we could not completely agree on them, and as a result I felt forced to introduce STorM32 variants of them, which can be found in the storm32.xml dialect.

Note: I am talking in this post exclusively about the gimbal device messages, not the gimbal manager ones.

In fact, the differences in the gimbal device messages as they can be found in common.xml and in storm32.xml are quite small, and I hence felt since ever that it is kind of really sad that a common agreement could not be reached.

I now, with this post, want to again try to come to a common set. This is very largely motivated by the recent efforts of Randy Mackay from the ArduPilot project (https://discuss.ardupilot.org/t/ardupilot-4-3-gremsy-gimbal-improvements-testers-wanted/86479), to implement a driver for the Gremsy gimbals. Importantly, in order to help with that, it appears that Gremsy is very willing to also work on their code, at least they offered some firmware updates related to Randy's efforts. Thus, I think, this is a good moment in time to try again to get a unified set of gimbal device messages, and it might be actually the last chance.

The differences are of two kinds

  1. Handling of yaw absolute and relative
  2. Handling of RC signals

Justification for these differences:

Both extensions I found to be required in order to deal with the STorM32 gimbal controller properly, and account for real-world applicvations which are still in widespread use.

In gimbal protocol v2 the gimbal is required to understand the notion of relative and absolute yaw, e.g., the ability to set an axis into LOCK mode requires the attitude information to be in earth frame. This however in turn requires (for STorM32, but also Gremsy) that the autopilot sends some data to the gimbal, which not all STorM32 users do. Yet, even though this info to determine absolute yaw is not available to STorM32, it allows an axis to be set to LOCK. And this feature was in fact demonstrated by users in real world to be highly useful for video taking. With gimbal protocol v2 this capability is simply lost completely.

In order to account for that, the STorM32 dialect has few more gimbal device flags, as well as an additional yaw value in the GIMBAL_DEVICE_STATUS message.

In gimbal protocol v2 the gimbal can only be controlled through the mavlink messages. However, STorM32 also allows the gimbal to be controlled by a rc input, at the same time. While this can be mimiced by additional code in e.g. the flight controller or in the GCS, for many STorM32 users it is by far the simplest and most straightforward option to control their gimbal from e.g. a transmitter. I note that e.g. the HereLink (to the best of my knowledge) is taking such an "old-fashioned" approach. Moreover, this "old fashioned" apporach allows STorM32 users to make use of many other features, like scripts, and so on, which are releatively popular. With gimbal protocol v2 all these capabilities are simply lost completely. It was simply decided "no old fashions".

In order to account for that, the STorM32 dialect have few more gimbal device flags

Summary:

So, at the end, all it is, is a number of additional flags, and one field in one message (and few renamings, which are nfc).

Sorry for reopening this chapter, but I really would not want to miss the chance and be most happy if this last call could open a path to one and the same set of gimbal device messages for all and every mavlink gimbals.

Personally I just feel that the differences are so minor that it's hard to see why they need to be rejected. The additional flags really can't harm, I would think, and the additional field could be added as an extension if one doesn't want to get it fully clean.

Cheers,
Olli


Appendix: Overview of changes between gimbal v2 and storm32 dialect

  • GIMBAL_DEVICE_INFORMATION:
    used also by storm32 dialect, no differences in message fields

  • AUTOPILOT_STATE_FOR_GIMBAL_DEVICE:
    used also by storm32 dialect, no differences in message fields

  • GIMBAL_DEVICE_SET_ATTITUDE vs STORM32_GIMBAL_DEVICE_CONTROL
    no change in fields, storm32 dialaect has differences in the flags (and differences in the message name)

GIMBAL_DEVICE_SET_ATTITUDE:
target_system
target_component
flags
q
angular_velocity_x
angular_velocity_y
angular_velocity_z

STORM32_GIMBAL_DEVICE_CONTROL:
target_system
target_component
flags
q
angular_velocity_x
angular_velocity_y
angular_velocity_z

  • GIMBAL_DEVICE_ATTITUDE_STATUS vs STORM32_GIMBAL_DEVICE_STATUS
    storm32 dialect has one addition field, and differences in the flags (and differences in the message name)

GIMBAL_DEVICE_ATTITUDE_STATUS:
target_system
target_component
time_boot_ms
flags
q
angular_velocity_x
angular_velocity_y
angular_velocity_z
failure_flags

STORM32_GIMBAL_DEVICE_STATUS:

target_system
target_component
time_boot_ms
flags
q
angular_velocity_x
angular_velocity_y
angular_velocity_z
yaw_absolute
failure_flags

  • GIMBAL_DEVICE_CAP_FLAGS vs MAV_STORM32_GIMBAL_DEVICE_CAP_FLAGS
    storm32 dialect has these flags in addition:
    MAV_STORM32_GIMBAL_DEVICE_CAP_FLAGS_HAS_ABSOLUTE_YAW
    MAV_STORM32_GIMBAL_DEVICE_CAP_FLAGS_HAS_RC

  • GIMBAL_DEVICE_FLAGS vs MAV_STORM32_GIMBAL_DEVICE_FLAGS
    storm32 dialect has these flags in addition:
    MAV_STORM32_GIMBAL_DEVICE_FLAGS_CAN_ACCEPT_YAW_ABSOLUTE
    MAV_STORM32_GIMBAL_DEVICE_FLAGS_YAW_ABSOLUTE
    MAV_STORM32_GIMBAL_DEVICE_FLAGS_RC_EXCLUSIVE
    MAV_STORM32_GIMBAL_DEVICE_FLAGS_RC_MIXED
    MAV_STORM32_GIMBAL_DEVICE_FLAGS_NONE

@olliw42 olliw42 changed the title Yet another effort on agreeing on unified gimbal device messages Yet another effort towards unified gimbal device messages Jun 28, 2022
@hamishwillee
Copy link
Collaborator

@julianoes I've assigned you to this. It is obviously a good idea to align if at all possible. Loop me back in if you think I can help.

@julianoes
Copy link
Collaborator

Thus, I think, this is a good moment in time to try again to get a unified set of gimbal device messages, and it might be actually the last chance.

Alright, sounds great. Let's do it. If possible, let's try to do it in a way not to break existing PX4 and Auterion users though, e.g. using extensions, etc.

  1. Handling of yaw absolute and relative

So you're saying that in order to support LOCK (with an absolute angle), certain messages need to be sent to the gimbal, however, that some implementations don't send the required data. That data would be AUTOPILOT_STATE_FOR_GIMBAL_DEVICE, right?

So, to me, this sounds like the v2 protocol is not correctly/completely implemented. So if that message is not sent, then something is missing for the protocol to work properly. So if the reason that this data is missing is that an implementor does not send that message, so doesn't implement the protocol correctly, then my response is that the implementation should be fixed.
Is there a reason this is not implemented that I'm missing?

Don't get me wrong, I'm not trying to argue against your suggestion, I'm just trying to understand the problem, and - in general - keep the protocol as simple as possible, so with the least options and flags required, whenever possible.

Handling of RC signals

You write that it is possible to mimic the RC input in the autopilot but that it is the "simplest and most straightforward option" to just directly feed RC through to the gimbal, if I understand that right.

So to explain this I'd like to go back to the goals which were laid out when it was decided that we needed a new gimbal protocol:
(from https://mavlink.io/en/services/gimbal.html#known_issues)

  • Confusion and conflicts between gimbal commands used between ground station and gimbal messages between autopilot and gimbal.
  • Control conflicts between different sources. It is not clear what takes precedence and how the deconfliction between different sources and commands should be implemented.

Now - as you already know very well - to deal with conflicting inputs (and to make it easier to debug), we moved the deconfliction to one place in the gimbal manager which owns the rules (and can be queried for debugging). In my mind, if we were now to allow RC signals to go directly past the manager into the gimbal, then we ignore the requirements of the new gimbal protocol, and essentially end up with problems that we set out to resolve.

Personally, I find it quite pleasing having a single point where the gimbal commands are routed through. This is for instance the debug output you can get on PX4 to check if the gimbal is not working:

pxh> gimbal status
INFO  [gimbal] Input Selected
INFO  [gimbal] Input: Mavlink (Gimbal V2)
INFO  [gimbal] Input not selected
INFO  [gimbal] Input: Test
  roll :   0 deg
  pitch:   0 deg
  yaw  :   0 deg
INFO  [gimbal] Output: MAVLink gimbal protocol v2
  quaternion: [1.0 0.0 0.0 0.0]
  angular velocity: [nan nan nan]
pxh> 
pxh> gimbal test pitch -45
pxh> 
pxh> gimbal status
INFO  [gimbal] Input Selected
INFO  [gimbal] Input not selected
INFO  [gimbal] Input: Test
  roll :   0 deg
  pitch: -45 deg
  yaw  :   0 deg
INFO  [gimbal] Input: Mavlink (Gimbal V2)
INFO  [gimbal] Output: MAVLink gimbal protocol v2
  quaternion: [0.9 0.0 -0.4 0.0]
  angular velocity: [nan nan nan]

I'm not fully opposed to your suggestion but I do feel like it would make it:

  • Less clean, more ways to do the same, more documentation required
  • Harder to debug
  • Not clearly specced: How is RC applied, which channels? Is it rates, or absolute angles?

To respond to a few specific points that you bring up:

I note that e.g. the HereLink (to the best of my knowledge) is taking such an "old-fashioned" approach.

Ok, and I think that's fine. RC is sent from HereLink to the air module which then forwarded RC to the autopilot. From there, some RC inputs are used to fly drones, some channels for switches and modes, and some channels are gimbal inputs and are translated to gimbal setpoints (if the gimbal manager decides to use them).

Moreover, this "old fashioned" apporach allows STorM32 users to make use of many other features, like scripts, and so on, which are releatively popular. With gimbal protocol v2 all these capabilities are simply lost completely.

Ok, maybe you can explain how these scripts work and we can find a way to enable that use case without the RC kludge. Could the scripts send gimbal commands to the autopilot which then set then set the gimbal? I have the feeling this is a discovery problem. So we need to document the commands required, and so users don't have to fall back to RC workarounds.

It was simply decided "no old fashions".

Any chance we can embrace the future? 😎

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 1, 2022

Hey @julianoes

first off, many thx for your willingness to dive yet again into this, and for your great response. Since we are talking about two topics, "abs/rel yaw" and "rc signals", I will respond to each in separate posts, to hopefully have it a bit more clean.

Don't get me wrong, I'm not trying to argue against your suggestion, I'm just trying to understand the problem, and - in general - keep the protocol as simple as possible, so with the least options and flags required, whenever possible.

don't worry, I fully understand your intention, and in fact fully agree (if I would sit on your side I probably might have written the very same sentences LOL)

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 1, 2022

abs/rel yaw (aka earth/body frame)

So you're saying that in order to support LOCK (with an absolute angle), certain messages need to be sent to the gimbal, however, that some implementations don't send the required data. That data would be AUTOPILOT_STATE_FOR_GIMBAL_DEVICE, right?

yes

So, to me, this sounds like the v2 protocol is not correctly/completely implemented.

you can argue that way, but it doesn't help anything. I don't see a reason to not permit reasonable use of the gimbal device message also then that feature is missing.

just as an OT side note, incomplete implementations of mavlink services are not totally uncommon, and often they are for the good (e.g. you really don't need to implement all of the camera protocol, a minimal set is required, but not all; and that's IMHO a good thing)

Is there a reason this is not implemented that I'm missing?

real world. Auterion can enjoy creating its complete environment and ecosystem. But in other parts of the world the situation is far less optimal and folks just strap their things together as they like, yet want to have at least main features working.

In my opinion, the fact is this:

concerning the input to the gimbal, there can be three cases:

  • pan: gimbal orientation with rel yaw (= in body frame)
  • lock: gimbal orientation with rel yaw (= in body frame)
  • lock: gimbal orientation with abs yaw (= in earth frame)

currently only two of them are supported, my suggestion is to support all three.

concerning the status output from the gimbal, there are currently only these cases:

  • pan: gimbal orientation reported with rel yaw (= in body frame) => abs yaw not easily obtained
  • lock: gimbal orientation reported with abs yaw (= in earth frame) => rel yaw not easily obtained

however, there are sure case there in e.g. pan one would want to also get the orientation in earth frame (and similarly for lock). Right now one also needs to get the vehicle attitude to convert, which can cause all sorts of problems. E.g. in AP/MP world it is streamed and it's rate is set by MP. You can't even ensure with reliability that you get it with the desired rate.

my suggestion is to support to have a means to know the "other" angle too.

If possible, let's try to do it in a way not to break existing PX4 and Auterion users though

I'm afraid it may need tiny changes, as there would be an additional flag to allow to distinguish between the three cases. For the reporting of the "other" angle this could be done by extension, but might not be what would fit "keep the protocol as simple as possible".

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 1, 2022

RC signals

if we were now to allow RC signals to go directly past the manager into the gimbal

that's correct, these signals go past the gimbal manager

then we ignore the requirements of the new gimbal protocol, and essentially end up with problems that we set out to resolve.

and this is exactly why these additional flags are needed, because with them the gimbal manager can send to the gimbal what it is allowed or not allowed to do with its rc signals. So the flags exactly serve the purpose to keep the manager under control, and thus to prevent violation of the basic principle!

btw, the gimbal manager protocol as currently in the spec does NOT allow the system to handle rc signals/joysticks within the principles set out, so with these flags the gimbal device part will be actually in better shape.

Personally, I find it quite pleasing having a single point where the gimbal commands are routed through.

me too! me too! I love it.
I agree that for the rc signals going to the gimbal it is somewhat broken, but the flags at least keep the manager under control.

Not clearly specced: How is RC applied, which channels? Is it rates, or absolute angles?

well, while true, somewhat unfair. For the herelink example, which you considered ok, exactly the very same points apply. In fact, even if one would follow the standard it essentially just says "implementation specific". So, I go the same route down here: implementation specific.

Ok, maybe you can explain how these scripts work and we can find a way to enable that use case without the RC kludge

the point was not scripts and what they exactly do, but that you can connect rc inputs to various functions (like in AP you can connect it to all sorts of things, https://ardupilot.org/copter/docs/common-auxiliary-functions.html)

If possible, let's try to do it in a way not to break existing PX4 and Auterion users though

here it is totally non breaking. All it does is to add some flags, and these flags happen to be such that if not set it would be exactly the correct fallback to when this all does not matter. It is only consuming bit space in the flags but otherwise NFC

@julianoes
Copy link
Collaborator

abs/rel yaw (aka earth/body frame)

just as an OT side note, incomplete implementations of mavlink services are not totally uncommon

real world. Auterion can enjoy creating its complete environment and ecosystem. But in other parts of the world the situation is far less optimal and folks just strap their things together as they like, yet want to have at least main features working.

Side note: I'm no longer with Auterion, I'm doing this just because I care, and opinions are my own.

That being said I understand, my opinion is that we sold fix broken implementations using examples, docs, one on one help, whatever it takes, but I don't feel like adding complexity to a protocol to work around incomplete implementations is the right way to go.

To me it feels a bit like we designed a protocol and then people implementing it are too confused or lazy to actually implement it correctly now forcing us to iterate on the protocol.

Do you remember me telling you that every complexity of the protocol has to be avoided if possible because people implementing it will be confused, and you argued that I was being unfair on them (or something along these lines) and that it would not be that hard. Well, now we are exactly in such a situation where it doesn't get implemented correctly, and it's hard to communicate/document it properly.

I think one part of improving the implementations would be a proper test suite which tells an implementer what is missing. Oh this reminds me that I wrote that: https://github.com/mavlink/MAVSDK/tree/main/examples/gimbal_device_tester

lock: gimbal orientation with rel yaw (= in body frame)

Ok, so ignoring my push back above for now. I don't actually properly understand how this would work. If you set an angle relative to the body frame and then you lock it, then the angle is wrong as soon as the drone yaws, right? So it's really just a command saying: keep the current angle and lock it without caring what that angle actually is, right?

Or could we just set the mode to lock and set the yaw angle to NAN to signal keep as is, rather than a set an angle value which is instantaneous, so outdated by the time you set it. All you want to set is "lock" and not "lock some other angle", right?

Out of curiosity: a gimbal which doesn't expect or get the AUTOPILOT_STATE_FOR_GIMBAL_DEVICE and doesn't know North, so doesn't have an absolute yaw estimate, how does it implement LOCK? And wouldn't it drift over time?

Right now one also needs to get the vehicle attitude to convert, which can cause all sorts of problems.

This, I think, is a very valid point though and something that can probably be improved. Should a gimbal just send both relative and absolute at all times in two fields? And if a gimbal doesn't understand absolute angles, it can just set it to NAN, right?

For the reporting of the "other" angle this could be done by extension, but might not be what would fit "keep the protocol as simple as possible".

I'm fine with an extension, and having both fields together and not have them switch between relative and absolute would actually be "simpler" in my mind. Right now it's confusing how it switches based on the flag. Now the problem here is that we have implemented the switching using the flags and that's being used, so we need to come up with a transition path, e.g. a flag to signal the new behavior.

@julianoes
Copy link
Collaborator

julianoes commented Jul 1, 2022

RC signals

btw, the gimbal manager protocol as currently in the spec does NOT allow the system to handle rc signals/joysticks within the principles set out, so with these flags the gimbal device part will be actually in better shape.

It would be in much worse shape because you have now two ways to do the same thing. Once RC via gimbal manager and once RC directly. Now you need to explain both ways, and explain when to use which one, and write tests for both. Then you still don't know what to do when both are sending RC, and you have essentially two places of deconfliction, once in the manager, and once in the gimbal device. Well, no thank you. I'm certainly not doing all that work just because people want to stick with some old poorly specced and poorly typed interface.

In fact, even if one would follow the standard it essentially just says "implementation specific".

We are designing a protocol for people to adhere to. If we wanted something "implementation specific" we would not need a protocol. So either we agree we want a protocol, or we decide we don't need it but please not both.

the point was not scripts and what they exactly do, but that you can connect rc inputs to various functions (like in AP you can connect it to all sorts of things, https://ardupilot.org/copter/docs/common-auxiliary-functions.html)

Then just create auxiliary functions which translate to gimbal commands. I think that's much neater.

All it does is to add some flags, and these flags happen to be such that if not set it would be exactly the correct fallback to when this all does not matter.

Having more flags mean that gimbal and autopilot implementers need to implement more stuff, or have more unexpected "doesn't work" or "not supported" moments. So while adding a flag to the spec is trivial, having it properly supported end to end is very much not. You argued yourself that sometimes someone just doesn't implement one piece. What happens if we have more of these flags but let's say gimbal manufacturer X does not implement the "direct RC functionality"? Well, it's not gonna work when someone wants to use that feature. And that person will be unhappy and say that the gimbal v2 protocol sucks, and they would be correct.

@julianoes
Copy link
Collaborator

@olliw42 let me know what you think.

In short:

Regarding relative/absolute: I'm open for improvements there, let's find a way to make it happen so we don't break existing implementations.

Regarding RC: maybe I completely misunderstood how RC works in your case and need to reconsider what you mean.

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 7, 2022

thx
sorry for my momentary silence, i happen to be quite busy and not in town, will surely respond properly in some days :)

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 9, 2022

@julianoes

I first have to say that some parts in your responses remind me more like what I find in ideological "fights". My ideology is "Simple things should be easy to do, and complicated things should be possible to do". I think this shall do it on this. :)

To me it feels a bit like we designed a protocol and then people implementing it are too confused or lazy to actually implement it correctly now forcing us to iterate on the protocol.

this here is not an iteration. I made the points from (kind of) day one and have made few attempts before to get them across ... this here is just yet another attempt :)

I really didn't went with storm32.xml just for fun :)

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 9, 2022

abs/rel yaw (aka earth/body frame)

lock: gimbal orientation with rel yaw (= in body frame)

I don't actually properly understand how this would work.

well, if it's understood or not, fact is: This operation mode is there in STorM32 since nearly a decade, way before even a thing like "MAVLink gimbal" was existing, and it is working fine, and it is factually used, in flight, by real users.

Regarding relative/absolute: I'm open for improvements there, let's find a way to make it happen so we don't break existing implementations.

Should a gimbal just send both relative and absolute at all times in two fields?

I'm fine with an extension, and having both fields together and not have them switch between relative and absolute would actually be "simpler" in my mind. Right now it's confusing how it switches based on the flag.

in principle, yes, it would be the best to send them both

it has some disadvantages

  • if it's quaternion for both, it's increasing the message size quite a bit, and this might be a high rate message (20 Hz e.g.)
  • what's about with rates? would also have to be added
  • this are the two reason why I chose just yaw, which isn't fully ideal however, as it takes away the advantage of q's, i.e., implicitely assumes that yaw is the "outer" axis
  • it needs a redesign, not so easy with just an extension.

We can adopt it by an extension of the message, but we cannot adopt it without any change in implementations, because of this current dependency on the flag. That is, we can't just define that the first quaternion is rel yaw (or abs yaw), and add an extension which is the quaternion for abs yaw (or rel yaw).

Some suggestions come to my mind

  1. If it ought to be an extension and all messages and flags ought to be compatible, I only see the option that the extension shows the other data not shown in the current message, i.e., it flips also. That is, if it is in yaw lock, then q and angular velocity in the main message body would be abs and in the extension rel, while if it is yaw follow it would be vice versa, q and angular velocity in the main body would be rel and in the extension abs. So, presumably doesn't make it really easier to understand.

Anything else will imply some sort of breaking, IMHO (but see 4 below)

  1. If we allow the assumption that yaw is the "outer" angle, we do not need two full quaternions and velocity vectors, but just the info for yaw and yaw velocity to derive the one from the other. Otherwise we will need the two complete sets of q and velocities.

  2. We could add an independent flag which frame it is, in order to decouple the frame from the yaw lock/follow frame. A STATUS message hence would either provide abs or rel. If a system wants to send both, it would send out two STATUS messages, with this flag toggled.

  3. We could add two flags like YAW_IS_EARTH and YAW_IS_BODY, which when set override the YAW_LOCK relation. That is, if neither YAW_IS_EARTH nor YAW_IS_BODY is set, one would fall back to the current behavior. If YAW_IS_EARTH is set, it would be abs, independent on the yaw lock/follow state. If YAW_IS_BODY is set, it would be rel, independent ion the yaw lock/follow state. If both YAW_IS_EARTH and YAW_IS_BODY is set ... well, could be forbidden, or could be used to indicate that there is an extension. This approach would actually be backwards compatible. Not so easy to understand either, at least for as long as the transitioning state exist.

The disadvantage of 3&4 is that it would not be easily possible to get the message of desire with the request_message approach (as one only can call for the message, not which flags)

  1. We could introduce two new status messages, which should replace the current STATUS, and call them like STATUS_REL and STATUS_ABS. The layout of the messages would be exactly like now, only that the data is rel in the first and abs in the second. So, if one wants to get both abs and rel, the system would have to send both, but one can call them with request_message.

There is a bit of overhead with having to send two messages instead of one with both data, but maybe it's not soo bad.

I'm not saying any of them are good, just food for brain.

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 9, 2022

RC signals

Regarding RC: maybe I completely misunderstood how RC works in your case and need to reconsider what you mean.

except of the ideology side, your comments are not incorrect. It is an additional path, with the implications you describe. The question is however if the implications are as catastrophic as you make them sound.

Now you need to explain both ways, and explain when to use which one, and write tests for both.

Maybe not as much, since one could clearly state that the "other" mode is discouraged.

Then you still don't know what to do when both are sending RC, and you have essentially two places of deconfliction, once in the manager, and once in the gimbal device.

incorrect. As I've tried to explain before, that's the reason for the flags. ALL deconfliction stays FULLY with the manager. And it is absolutely clear for the gimbal what it is should do when it sees the flags, or not do. And any gimbal can decide to simply not support/implement this. Likewise, one also do not have to change neither the PX4 nor (upcoming!?) ArduPilot manager implementation. So it is absolutely back-wards compatible, and doesn't affect any existing implementation.

simply incorrect complains :)

What happens if we have more of these flags but let's say gimbal manufacturer X does not implement the "direct RC functionality"? Well, it's not gonna work when someone wants to use that feature.

that's absolutely not different to when any other feature is not implemented. Your argument only could make sense if a protocol in your mind implies that ALL related features&messages are ALL COMPLETELY implemented - but that is never and was never realistic in many of the other mavlink protocols. IMHO it doesn't make sense to raise the bar that high. Even if a camera only provides photo, and no streaming, the camera protocol should work and be of use. Anything else would IMHO be just insane. Gladly, the camera protocol has that flexibility...

So, how would this be noted here? Simply like any other unsupported feature, by the fact that the capability flag would not be set.

To be clear:
I am not as stiff on the rc signals thing as I'm on the abs/rel yaw thing.
The abs/rel yaw thing is just a must for me. No way out. Without it, STorM32 won't use mavlink's gimbal device protocol.
Not so with the rc signals thing. If it isn't supported would not make me decide against using mavlink's gimbal device protocol (I only would constantly put blame on it if users complain LOL). Give it's near free cost I just don't see it.

Maybe one should add some user flags area, which are gimbal specific.

@julianoes
Copy link
Collaborator

abs/rel yaw (aka earth/body frame)

what's about with rates? would also have to be added

But the rates don't change, right? They are independent of the yaw reference.

  1. We could add two flags like YAW_IS_EARTH and YAW_IS_BODY, which when set override the YAW_LOCK relation.

This sounds like the least breaking change, and ok to explain. Would it then be ok to send the quaternion in only one of the two frames?

@julianoes
Copy link
Collaborator

RC signals

Maybe not as much, since one could clearly state that the "other" mode is discouraged.

So we're adding a feature and calling it discouraged at the same time? That's a bit ironic 😄.

And it is absolutely clear for the gimbal what it is should do when it sees the flags, or not do.

Correct, it's clear for the gimbal. Whether it's clear for the confused user or developer when it doesn't work as expected, maybe...

IMHO it doesn't make sense to raise the bar that high.

Right, but I'm still trying to keep the complexity low, whenever possible.

Give it's near free cost I just don't see it.

That's where I disagree. I think it is a cost.

Actually, talking about the ArduPilot implementation. This could be something to get their opinion on. E.g. if Randy tells me that it would be worthwhile to have for ArduPilot, that would be more convincing. So far I haven't heard or read that from them but I might have missed it. Right now all background I have is based on the implementation for PX4/Auterion and working with Gremsy, plus your opinion, of course.

For the absolute/relative yaw thing I'm more convinced because it's actually something that we got wrong a couple of times when implementing it at Auterion, and something I explained to Gremsy, and had to admit that it's a bit odd.

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 11, 2022

abs/rel yaw (aka earth/body frame)

what's about with rates? would also have to be added

But the rates don't change, right? They are independent of the yaw reference.

no, they change. The rate in earth frame would be the sum of the rate of the gimbal in body frame plus the rate of the vehicle ...

  1. We could add two flags like YAW_IS_EARTH and YAW_IS_BODY, which when set override the YAW_LOCK relation.

This sounds like the least breaking change, and ok to explain. Would it then be ok to send the quaternion in only one of the two frames?

this would be the idea of it, that you send in one message the q and rate for the specified frame

if you want to get it for both frames you need to send TWO messages, one with the flag set for the one frame and another one with the flag set for the other frame

that's the two things I do not like soo much about it

  • the data is not "coherently" send, but distributed over time in two messages
  • you cannot easily set the stream or request the desired message using the request stream/message concepts

one could alleviate the first obstacle by adding an extension to the message holding a further q and further rate, and specify that if both flags are set the message contains both frames, e.g. the body in the first q&rates and the earth in the extended q&rates

I'm not sure I like that all so much, but they appear least breaking. I still think however it would be neater to just define the one message properly.

EDIT:
Another non-breaking approach, which is maybe neater as it would avoid resulting in two messages, is to add only one new flag which says that the message carries both body and earth data. That is, with this flag not set, one would fall back to the current behavior, there the first q&rates are in earth or body depending on the yaw lock flag, but with this new flag being set, the message is extended such as to carry both body and earth q&rates (e.g the body in the first q&rates fields, the earth data in the extended q&rates fields).

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 11, 2022

RC signals

Correct, it's clear for the gimbal. Whether it's clear for the confused user or developer when it doesn't work as expected, maybe...

well, isn't this the general issue with ANY feature which can be supported but may not be supported in an actual implementation ... like mavftp, like param ext, like, all the capabilities in camera, like ... should I have to produce a list?

Give it's near free cost I just don't see it.

That's where I disagree. I think it is a cost.

I didn't say zero cost. So, yes, there is cost ... but ... it's near free

gimbal device manufacturers don't have to support it
gimbal managers don't have to support it either

Actually, talking about the ArduPilot implementation. This could be something to get their opinion on. E.g. if Randy tells me that it would be worthwhile to have for ArduPilot, that would be more convincing. So far I haven't heard or read that from them but I might have missed it. Right now all background I have is based on the implementation for PX4/Auterion and working with Gremsy, plus your opinion, of course.

well, I do not think ArduPilot will consider this relevant. They since ever did their own mimicry for handling rc inputs, and the new driver is no different. It's a main reason why people to the day do things like sbus y splitters ... LOL ... since they otherwise can't get done what they want to do.
Also, I just can't consider their current attempt to be any good in terms of an example to look at, given that it breaks all basics and the fabric of mavlink (key word: private channel)

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 14, 2022

RC signals

@julianoes sorry for spilling you, but I scanned the rcgroups STorM32 thread for user cases where the rc signals thing would matter, to just recollect its relevance, and (at least) one application might help to convince you: dual operator control. A simple way to achieve this is by having a 2nd receiver on the vehicle, with its ppm/sbus/xxx output connected to the gimbal controller. With the flags as suggested in the above this would be fully conform and not break the gimbal manger/device concept. I can't tell how it would be done with Px4, but I can tell that with ArduPilot one would really have to jump through very tiny hops to achieve this otherwise. PS: I am not saying that it is impossible to achieve otherwise, all I'm saying is that any other approach really requires a more complex and sophisticated frame work (and in the case of ArduPilot substantial recoding, or a companion with respective code).

Just wanted to share this.

@julianoes
Copy link
Collaborator

dual operator control

Interesting. I'll think about it.

@julianoes
Copy link
Collaborator

@hamishwillee, @auturgy and I discussed this on the call and concluded that the RC override might be worth it, mostly for the case that you describe where you have two receivers, one directly connected to the gimbal.

However, @auturgy was a little worried what happens if you implement RC input as nudging (mixed?), and the gimbal manager would start to fight the control through RC.

This concern is mostly based on the fact that there are these two flags, the latter being RC_MIXED.
MAV_STORM32_GIMBAL_DEVICE_FLAGS_RC_EXCLUSIVE
MAV_STORM32_GIMBAL_DEVICE_FLAGS_RC_MIXED

Maybe you can elaborate a bit more how you see that to work or how we would prevent the gimbal manager from "fighting" it?

Or should we use the same terminology with primary and secondary control?

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 20, 2022

these flags are set by the gimbal manager!
So, by construction, the gimbal manager has nothing to fight against, since it has the full and sole decission power of what to allow. If it does not want rc_mixed, it simply does not set the flag, and the gimbal device would not (be allowed to) do it.

same so for rc_exclusive.

Remind, these flags are exactly introduced (required) for allowing the gimbal manager to be under FULL control of what's going on. That's the argument for their existance.

obviously, the two flags are mutually exclusive, i.e., it doesn't make sense to set both. In order to help with possible mistakes, one could define that one has priority over the other, e.g., if both rc_exclusive and rc_mixed would be set one could define that it's equal to rc_exclusive.

In principle, there is similarity to primary and secondary control, except that

  • I never liked that language, since it is nowhere properly defined what primary and secondary means. That is, what is the difference of setting primary = sysidA/compidA and secondary = sysidB/compidB versus setting primary = sysidB/compidB and secondary = sysidA/compidA? I see a logical fault here as there is none. The fact is simply that one can assign control to up to two id's, with no speficied ordering.
  • rc_exclusive would then have to relate to rc_primary, and rc_mixed to rc_secondary. I am not convinced that this makes the function easier to understand.
  • primary/secondary is a gimbal manager concept. I would think that it's useful to keep it that way, and not bring it in also into gimbal device.
  • The language in the docs clearly imply that e.g. nudging is a special case of the general mechanism: "but assign a ground station for secondary control to e.g. nudge during the smart shot". (note the "e.g."). Also, the docs say "The implementation of how primary and secondary control are combined or mixed". It uses the word "mixed" to explain what it is about.

I find exclusive and mixed quite descriptive :)

I'm however fully open to better naming.

@julianoes
Copy link
Collaborator

primary/secondary is a gimbal manager concept. I would think that it's useful to keep it that way, and not bring it in also into gimbal device.

Ok, that's a good point.

It uses the word "mixed" to explain what it is about.

That's funny.

I'm fine with calling it mixed then, and it seems like it is not conflicting because the gimbal manager is aware, as you explained. So then, how would mixing look like exactly? I assume it's somewhat implementation and situation dependent? Just because you can have angle or angular rate inputs for both, right?

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 20, 2022

So then, how would mixing look like exactly? I assume it's somewhat implementation and situation dependent? Just because you can have angle or angular rate inputs for both, right?

well, the simple answer is to use the doc's answer for primary and secondary: "Note The implementation of how ... control are combined or mixed is not defined by the protocol but up to the implementation. This allows flexibility for different use cases."

in the case of the STorM32 it is simply a + on the angles from the two sources. That is, it gets an angle from the mavlink command and an angle from the rc input, and these are just added up. Rate inputs are internally converted to angle set point values for the PID controller, so if it's a rate input, the + happens simply after that conversion from rate to angle set point. It's quite straightforward and actually easy to comprehend for users. As it does what one colloquially could call "it's the combination of what both would do individually".

@julianoes
Copy link
Collaborator

Ok, let's put that in the docs as "an example of how it could be implemented.

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 21, 2022

ok, great
so, it seems to me we have agreed on adding flags
GIMBAL_DEVICE_CAP_FLAGS_HAS_RC
GIMBAL_DEVICE_FLAGS_RC_EXCLUSIVE
GIMBAL_DEVICE_FLAGS_RC_MIXED
fantastic !!!

it seems to me we also have agreed on adding something for abs/rel yaw, but we haven't yet determined it exactly. Some options were discussed in the above.
I have to say that while some discussed option my do the job, I kind of prefer adding a new status message, which is just clean and as we want it to be. Consumer codes would have for a time to come digest both messages, but gimbals could just go to the newest in a next firmware update.

@julianoes
Copy link
Collaborator

but gimbals could just go to the newest in a next firmware update.

Yer maybe that's the easiest indeed.

I don't actually properly understand how this would work. If you set an angle relative to the body frame and then you lock it, then the angle is wrong as soon as the drone yaws, right? So it's really just a command saying: keep the current angle and lock it without caring what that angle actually is, right?

I asked this a while back. Can you remind me how this works, or why I'm confused? 😄

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 21, 2022

but gimbals could just go to the newest in a next firmware update.

Yer maybe that's the easiest indeed.

so, what dou you prefer?

  • Full q & rates for both?
  • you consider it ok to just go with yaw and yaw rates to allow converting from one to the other (which implies the assumption of specif Euler angle representations)?

I would prefer to not have abs and rel distributed over two messages, but to have them both in one message. What do you say?

I don't actually properly understand how this would work. If you set an angle relative to the body frame and then you lock it, then the angle is wrong as soon as the drone yaws, right? So it's really just a command saying: keep the current angle and lock it without caring what that angle actually is, right?

I asked this a while back. Can you remind me how this works, or why I'm confused? 😄

well, I indeed haven't explained how that works (for STorM32) ... and this is because it's one of the things which I find extremely difficult to explain in words ... so, it's not becasue I didn't wanted to but just that I'm worried that it would result in endless text without really clarifying things.

the lock kicks in at the current orientation, and while in lock you still can move it with rel commands, but also with abs commands! Now, for rel commands it's quite obvious how it works, one simply moves the camera, i.e., the lock set point accordingly. For abs commands, when they are expressed in body frame, it's less obvious how to deal with them. What the STorM32 does is to take the increment/difference from when it was switched into lock. It keeps an additional variable just for that purpose. I am not sure if that's a proper statement, but I guess one can say that the body frame changes from x forward = vehicle forward to x forward = camera forward at time it switched into lock. The effect is however quite natural.
I also can say that it indeed took few iterations in the code, before the users had been happy, and found it quite natural. So, it's indeed not immediately obvious how to exactly implement it.

@julianoes
Copy link
Collaborator

Sorry, I'm a bit slow here. I'm trusting your suggestions here basically, as long as we have a transition path, and docs for it.

you consider it ok to just go with yaw and yaw rates to allow converting from one to the other (which implies the assumption of specif Euler angle representations)?

If we can agree on the Euler representations to use and get it right, I would be ok with that.

I would prefer to not have abs and rel distributed over two messages, but to have them both in one message. What do you say?

I agree with that.

to take the increment/difference from when it was switched into lock.

Right, that's what I guessed.

The effect is however quite natural.

Makes sense, yes.

I also can say that it indeed took few iterations in the code, before the users had been happy, and found it quite natural.

And that's interesting, ok.

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 26, 2022

so, I take this to mean that we can agree on that the additional mode "lock with body frame" exists, i.e., that in the SET_ATTITUDE message we indeed want to cover the three cases "pan with attitude/rates in body frame", "lock with attitude/rates in body frame", and "lock with attitude/rates in earth frame". This implies that we cannot correlate lock/pan with the frame type, and that we thus need a away to specify independently if the attitude/rates in the message are in earth or body frame.

my suggestion was to add a respective flag to specify which frame it is

If we can agree on the Euler representations to use and get it right, I would be ok with that.

no, we cannot agree on the Euler representation. First, the Euler angles generally have the singularity/gimbal lock issue, and thus are not most suitable in general. Secondly, and that's maybe what will trip off most "users", the standard aviation euler angles are nearly always totally unsuitable for 3 axis gimbals, since they do have their singularity for tilting down, which however is a very frequently desired operation range. That is one would have to define an "unusual" Euler representation. These had been reasons to go with quaternions in the first place, and I still think they are very good reasons.

I would prefer to not have abs and rel distributed over two messages, but to have them both in one message. What do you say?

I agree with that.

great. So we want to design a ATTITUDE_STATUS message, or a substitute, which carries both at the same time.

as long as we have a transition path, and docs for it.

that's at this point in time probably the difficult part. I can't see a "solution" which would satisfy all wishes and requirements simultaneously. That is, some tradeoff will have to be made, some grain of salt will have to be swallowed. Some suggestions had been made in the above. Any preference already?

@julianoes
Copy link
Collaborator

Sorry for the silence here @olliw42. I'll try to make an effort tomorrow to come up with something.

@julianoes
Copy link
Collaborator

so, I take this to mean that we can agree on that the additional mode "lock with body frame" exists, i.e., that in the SET_ATTITUDE message we indeed want to cover the three cases "pan with attitude/rates in body frame", "lock with attitude/rates in body frame", and "lock with attitude/rates in earth frame". This implies that we cannot correlate lock/pan with the frame type, and that we thus need a away to specify independently if the attitude/rates in the message are in earth or body frame.

Yes, sounds good.

Secondly, and that's maybe what will trip off most "users", the standard aviation euler angles are nearly always totally unsuitable for 3 axis gimbals, since they do have their singularity for tilting down, which however is a very frequently desired operation range.

You're absolutely right, and I just had to find that out the hard way debugging that "gimbal yaw" didn't work at pitch -90 🤦‍♂️.

So we want to design a ATTITUDE_STATUS message, or a substitute, which carries both at the same time.

Yes, except can we add extensions to the current message and that way be backwards compatible? I don't know why that didn't occur to me earlier.

@julianoes
Copy link
Collaborator

@olliw42 I just got reminded of this in the MAVLink call. Should we try to continue?

@olliw42
Copy link
Contributor Author

olliw42 commented Sep 4, 2022

@julianoes
I am soooooo sorry for my long silence and absence, but I am really just so occupied with all sorts of things that gimbals just didn't find space in this little thing which one calls my brain.
I am defenitely still very intend to continue with this and to hopefully resolve it.

You're absolutely right, and I just had to find that out the hard way debugging that "gimbal yaw" didn't work at pitch -90

I told you so LOL

except can we add extensions to the current message and that way be backwards compatible? I don't know why that didn't occur to me earlier.

I think this option was discussed to some extend in the above. I would have to go through all of it again, since ... you now this little thing called brain ... but my recollection is, it can't be made practically backwards compatible. The message structure could, but not the flags.

@julianoes
Copy link
Collaborator

I told you so LOL

Ha! 😄

it can't be made practically backwards compatible

I'm gonna give it a try.

@olliw42
Copy link
Contributor Author

olliw42 commented Sep 5, 2022

it can't be made practically backwards compatible

I'm gonna give it a try.

great !

pl go back through this thread however, so we hopefully don't go in circles

@julianoes
Copy link
Collaborator

You're right, we did discuss the extension in this thread, I just completely forgot about it and then had the epiphany (again), funny...

@olliw42
Copy link
Contributor Author

olliw42 commented Sep 5, 2022

so, what does it mean, you also conclude it's not possible, or you are going to give it another try (taking into account what was concuded already)?

@julianoes
Copy link
Collaborator

I'm basically tempted by your suggestion number 4 in #1860 (comment):

  1. We could add two flags like YAW_IS_EARTH and YAW_IS_BODY, which when set override the YAW_LOCK relation. That is, if neither YAW_IS_EARTH nor YAW_IS_BODY is set, one would fall back to the current behavior. If YAW_IS_EARTH is set, it would be abs, independent on the yaw lock/follow state. If YAW_IS_BODY is set, it would be rel, independent ion the yaw lock/follow state. If both YAW_IS_EARTH and YAW_IS_BODY is set ... well, could be forbidden, or could be used to indicate that there is an extension. This approach would actually be backwards compatible. Not so easy to understand either, at least for as long as the transitioning state exist.

@julianoes
Copy link
Collaborator

@olliw42 I had to actually write up the changes to think it through:

#1885

@olliw42
Copy link
Contributor Author

olliw42 commented Sep 6, 2022

@julianoes
ok, I am fundamentally fine with this approah, though it may need some refinement/clarifications. I'll continue in the PR, if that is ok.
:)

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 8, 2022

@julianoes

my attempt to get it finally of the hook

#1911
#1912
#1914
#1913

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 28, 2022

the key parts of this issue have all been resolved and merged
so I happily close :)

@olliw42 olliw42 closed this as completed Nov 28, 2022
@hamishwillee
Copy link
Collaborator

Hurrah! Does this mean you will be able to drop using the specific dialect for gimbals going forward?

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 30, 2022

not completely

I will drop the specific gimbal device messages! The storm32 gimbal manager messages will stay.

I in fact already started the process of removing the storm32 gimbal device messages in all of my stuff (not yet completed since it's a lot stuff, STorM32, mLRS, BetaPilot, OpenTx4Mavlink)(wiki is already done LOL). Once completed, you will "notice" however, since I will drop a PR with changes to the storm32.xml dialect. :)

This should serve many users however since the gimbal managers out there should work. I.e. STorM32 should work with PX4 and MAVSDK (which I assume have v2 gimbal manager implementations), or maybe even ArduPilot if they ever implement a compatible gimbal device v2 protocol (as usual, they managed to screw it up)(and they know it as they have been told ;)).

The unification of the gimbal device messages should be a significant step towards less fragmentation.

PS: there is still one tiny gibal device PR open winky, winky :)

@julianoes
Copy link
Collaborator

Amazing, thanks @olliw42. Looking forward to testing this with PX4 and MAVSDK soon. (I've ordered the parts that I need, I believe.)

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