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

Gimbal V2 Protocol Discussion II: Gimbal device information #1370

Closed
olliw42 opened this issue May 5, 2020 · 15 comments
Closed

Gimbal V2 Protocol Discussion II: Gimbal device information #1370

olliw42 opened this issue May 5, 2020 · 15 comments
Labels

Comments

@olliw42
Copy link
Contributor

olliw42 commented May 5, 2020

I am not totally happy with the GIMBAL_DEVICE_INFORMATION message.

It's fields are:

  • time_boot_ms
  • vendor_name, model_name, firmware_version
  • cap_flags
  • tilt_max, tilt_min, tilt_rate_max, pan_max, pan_min, pan_rate_max

First, I wonder about the vendor_name, model_name, firmware_version fields. These are also contained in the COMPONENT_INFORMATION message, so there is a substantial duplication here.
I'm not sure if there is an implicit decision about the design or even fate of COMPONENT_INFORMATION in here, but it somehow makes much more sense to me to have a message which is generic for all kinds of components to request such information instead of having very specific messages for specific types of components. Furthermore, I think that besides the firmware version also the hardware version can be an equally relevant piece of information, and I find it thus somewhat regretable that here the layout of CAMERA_INFORMATION and not of COMPONENT_INFORMATION seems to be followed.

My suggestion:
Either remove these fields and shift to COMPONENT_INFORMATION. Or, if they should remain here, for whatever reasons, then please add hardware_version.

Next, it is actually totally unclear to me what the purpose of time_boot_ms could be. This is totally static data, and thus valid at every time, and not just at the time there the data was requested. So, this field appears to be totally superfluous and just eat away bandwidth.

I realize that this comment is also appllies to messages like CAMERA_INFORMATION and COMPONENT_INFORMATION, there CAMERA_INFORMATION was kind of the template for inspiration or the others. But this doesn't make the argument wrong.

My suggestion:
If the field time_boot_ms is really here just because it is present also in other similarly static messages or because of the better looks of symmetry, I would suggest to remove it, and also e.g. in the COMPONENT_INFORMATION message.

Last, the tilt and pan fields are not totally clear to me. With a gimbal two types of range limits come into play. First, absolute limits, which are e.g. given by mechanical constraints of the gimbal. E.g., a gimbal might be restricted to a range of +-160° in pan because of some hardware stoppers in its construction which prevents larger pan angles. Second, relative limits, which are control limits, such as e.g. the range one can tilt a camera with a joystick. In order to this one usually specifies some min/max values in order to translate the joystick position signal into an angle value. Now, the key point, these ranges can be identical at best only when the camera is pointing forward, when the gimbal base, that is the aircraft, is moving however and the gimbal axes not equal to zero anymore, these two ranges specify different restrictions on the camera. This particularly obvious when the yaw axis is in follow (not LOCK) mode.

In order to prevent damage to the gimbal one rather would be concerned about the mechanical constraints, for control such as joystick things one rather would be concerned about the relative angles.

So, the point here is that it's totally unclear which type of limits this here are. In principle one actually rather would want to know both.

My suggestion:
Make that clear and or provide the fields to give both.

Int16 instead of float would be totally sufficient and could be worth a thought too.

:)

@julianoes
Copy link
Collaborator

First, I wonder about the vendor_name, model_name, firmware_version fields. These are also contained in the COMPONENT_INFORMATION message, so there is a substantial duplication here.

That's just because the initial gimbal version dates more than a year back by now and that's what I came up with at that point. So, please, by all means make a pull request and remove duplicated stuff. We should do that a soon as possible. The quicker we fix it the less stuff we break as things are now being implemented.

So, this field appears to be totally superfluous and just eat away bandwidth.

I have no strong opinion. We just have this message in other messages like this 🤷‍♂️, and it's only the 32bit version, so it's not as bad as the 64bit microsecond version.

In order to prevent damage to the gimbal one rather would be concerned about the mechanical constraints, for control such as joystick things one rather would be concerned about the relative angles.

I'm not concerned about damage. If a gimbal doesn't prevent doing damage to itself, then we have a problem 😄. What I care about is what a ground station can display for a user, e.g. I want to have an indicator telling me where the gimbal currently is relative to its full range. Therefore, the gimbal should probably send the full range, so that I can make the best use of it.

Of course, the ground station might then constrain that range if it doesn't actually make much sense to actuate it all, e.g. a tilt range more than +/- 90 degrees might not be beneficial.
The ground station might also want to constrain the possible input to the gimbal based on tilt limits configured for the autopilot in order to prevent getting to the limit in aggressive maneuvers.

I think what I'm arguing for is "mechanical limit" but maybe you can formulate that better.

Int16 instead of float would be totally sufficient and could be worth a thought too.

Generally I just go - lazily - for float if a message is not actually sent at high rate and optimization in that respect is required but again, no strong opinion.

@olliw42
Copy link
Contributor Author

olliw42 commented Jun 10, 2020

So, please, by all means make a pull request and remove duplicated stuff. We should do that a soon as possible. The quicker we fix it the less stuff we break as things are now being implemented.

perfect. Makes sense.

It however hinges on the fate of COMPONENT_INFORMATION, and it is not clear to me at all if the respective fields will remain in COMPONENT_INFORMATION or not. So:

  • you guys decide that these fields are not going to be removed from COMPONENT_INFORMATION, so I could do a PR to settle this issue here
  • you guys decide that these fields are removed from COMPONENT_INFORMATION, so they stay here and I would add them also to GIMBAL_MANAGER_INFORMATION
  • things have to wait until things become more clear

I think what I'm arguing for is "mechanical limit" but maybe you can formulate that better.

perfect. Makes sense.

@julianoes
Copy link
Collaborator

For component information, make sure to check #1346.

@olliw42
Copy link
Contributor Author

olliw42 commented Jun 10, 2020

I know about this thread, but have kind of pulled myself out of that topic and don't really follow it. Maybe you could just say if it's the 1st, 2nd or 3rd bullet :)

@julianoes
Copy link
Collaborator

Alright, let me check.

vendor_name, model_name, firmware_version
These will be covered by the component information version json file.

Cap flags and limits are gimbal specific and I'd say we need them.

Am I missing something?

@olliw42
Copy link
Contributor Author

olliw42 commented Jun 10, 2020

vendor_name, model_name, firmware_version
These will be covered by the component information version json file.

I had seen these lines, and they didn't make me very happy. Makes it quite difficult for not so sophisticated GCSes and other components to get hands on this useful info. If that is the plan I guess I would vote for keeping these fields in GIMBAL_DEVICE_INFORMATION and add them to the GIMBAL_MANAGER_INFORMATION. But I am somewhat exhausted to argue in that matter, so I'm just going to await whatever the outcome is. :)

Cap flags and limits are gimbal specific and I'd say we need them.

yeah

Am I missing something?

can't say :)

It's easy to extend anyway, for the moment it looks complete to me.

what I would consider handy is also a user-specifiable name field. I have users with several gimbals, some even with dual gimbal setups, and a plain name is more descriptive than GIMBAL1 and GIMBAL2. But this is really just a super feature. So, I will immediately accept if it is down-voted LOL.

:)

@julianoes
Copy link
Collaborator

I think I agree, and I actually ran into exactly that issue just before over in #1394. I don't want to believe that one needs to download a file to get a version number (or 4 bytes or whatever it is).

@olliw42
Copy link
Contributor Author

olliw42 commented Jun 11, 2020

I think that the xml mechanism is really fantastic, but I also think that it should not be used for everything. The "most common" data should be in plain fields, IMHO. What "most common" is supposed to mean is of course debatable and subjective ... but there is probably something like a least common multiple and a greatest common divisor.

@olliw42
Copy link
Contributor Author

olliw42 commented Jun 23, 2020

it seems a decision was made as regards COMPONET_INFORMATION to screw it up , so one is left with praying that that the fields in GIMBAL_DEVICE_INFORMATION will remain as they are

topic can hence be closed

@julianoes
Copy link
Collaborator

I'm bringing it up again.

@hamishwillee
Copy link
Collaborator

FWIW I am hiding from any issue that says "Gimbal". Two things were said in the thread that I noticed:

  1. Component information is cool but not for everything. I think that is a very good point.

  2. it seems a decision was made as regards COMPONET_INFORMATION to screw it up , so one is left with praying that that the fields in GIMBAL_DEVICE_INFORMATION will remain as they are

A decision was made to change it. I am not sure whether you think this is "all screwed up" or merely "has broken something you depend on". If the former, we need more detail to understand better ways of doing things. If the later, then that should not have happened without discussion.

IMO this should follow the model of camera info - you should be able to get core/basic use from mavlink messages without having to support XML download and parsing. So lets get the bits that are "core" back into the message itself. And now I re-read the thread, it sounds like that is what Julian is doing. Duh.

@olliw42
Copy link
Contributor Author

olliw42 commented Jul 1, 2020

A decision was made to change it. I am not sure whether you think this is "all screwed up" or merely "has broken something you depend on". If the former, we need more detail to understand better ways of doing things.

it's the former, "all screwed up"
I btw would not choose the language I chose if all it would be would be breaking something on my end. I understand well that my end is irrelevant to MAVLink as an ecosystem and standard. (it did break my end, but as indicated that's totally irrelevant here)

@hamishwillee
Copy link
Collaborator

Thanks. I really want to do this right, and I respect your opinion. But a statement "all screwed up" is not something I can act on. To make change I need to understand what you didn't like about the changes, and what you would do differently.

@stale
Copy link

stale bot commented Aug 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@olliw42
Copy link
Contributor Author

olliw42 commented Dec 5, 2020

discussion has kind of settled, and the issue went stale anyway, so I close it. :)

@olliw42 olliw42 closed this as completed Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants