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

common: add note about lan and lon = 0 #1426

Closed
wants to merge 2 commits into from
Closed

common: add note about lan and lon = 0 #1426

wants to merge 2 commits into from

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented Jul 16, 2020

This just describes the state as it is implemented by ArduPilot and QGC.

Relevant discussion in PX4/PX4-Autopilot#15308.

MAVSDK change to match it: mavlink/MAVSDK#1144

This just describes the state as it is implemented by ArduPilot and QGC.
@@ -4918,8 +4918,8 @@
<description>The filtered global position (e.g. fused GPS and accelerometers). The position is in GPS-frame (right-handed, Z-up). It
is designed as scaled integer message since the resolution of float is not sufficient.</description>
<field type="uint32_t" name="time_boot_ms" units="ms">Timestamp (time since system boot).</field>
<field type="int32_t" name="lat" units="degE7">Latitude, expressed</field>
<field type="int32_t" name="lon" units="degE7">Longitude, expressed</field>
<field type="int32_t" name="lat" units="degE7">Latitude, expressed (if both lat and lon are 0, it means that they are not available yet)</field>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is ", expressed" needed here? I suspect a hangover from when the units were removed.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianoes Yes, we need a way to indicate that specific values are not ready (if all values are not ready the message probably should not be emitted).

  1. But 0,0 is actually valid (though admittedly an edge case). As you know it is considered bad form to use a valid value to indicate "not supported/supplied", which is why we use an INT32_MAX for things like this.

    Could we work with ArduPilot to "do the right thing" here? Practical code could still recognise that 0,0 is invalid for now.
    @auturgy FYI?

  2. Presumably some of the other fields might also not be supplied - either on startup, or even in some GPS units. Can we add "not supported" cases for those too?

@hamishwillee hamishwillee added the Dev Call Issues to be discussed during the Dev Call label Jul 22, 2020
@auturgy
Copy link
Collaborator

auturgy commented Jul 22, 2020

Let’s not scope creep: I’d get this in (the edge case, aka Null Island, isn’t likely to be encountered), and perhaps raise an issue to review/add extras

@LorenzMeier
Copy link
Member

Entering magic values into a valid range is against best practices. It is unlikely that a lat / lon of 0/0 ever hits a real system - yes. But if I were to review the MAVLink spec and found stuff like that I would loose trust into the implementers because very apparently they are too lazy to implement things correctly and have all sorts of magic easter eggs hidden.

Magic values are fine until they are not. And it is very simple to objectify this: If a unit test that does an input data sweep would fall over, then it is a bad change. In this case a sweep across all possible coordinates would generate an incorrect result here, so it is a bad change.

@DonLakeFlyer If you need altitude, please use the correct packet for it: http://mavlink.io/en/messages/common.html#ALTITUDE

@LorenzMeier LorenzMeier deleted the pr-lat-lon branch July 22, 2020 08:23
@hamishwillee
Copy link
Collaborator

@LorenzMeier Is it worth though tightening the spec to do this "correctly"? i.e. specify reasonable values like INT32_MAX for the values that potentially might not be sent, and which are therefore sent as zero.

@DonLakeFlyer
Copy link
Contributor

specify reasonable values like INT32_MAX for the values that potentially might not be sent

We should do that. There needs to be something, otherwise a GCS doesn't know how to get altitude information from what message.

@LorenzMeier
Copy link
Member

The INT32 max approach is perfectly adequate and fine by me. It might however break existing receivers which lack range checking.

@DonLakeFlyer You really should be getting altitude information from the ALTITUDE message. GLOBAL_POSITION_INT is designed as a global position estimate that ONLY gets sent if you actually have one. The MAVLink spec has an approach to do all of this correctly for a long time, so it should be used.

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Jul 25, 2020

The MAVLink spec has an approach to do all of this correctly for a long time, so it should be used.

Doc'ed where? Point me at the docs which tell a GCS what it should do and I'll update to it.

@LorenzMeier
Copy link
Member

LorenzMeier commented Jul 25, 2020

@DonLakeFlyer If you need altitude, please use the correct packet for it: http://mavlink.io/en/messages/common.html#ALTITUDE

@DonLakeFlyer
Copy link
Contributor

Ages ago, this was changed: mavlink/qgroundcontrol#4346. It made GLOBAL_POSITION_INT take precedence over ALTITUDE. No comments in there as to why unfortunately.

So the question for mavlink spec is which one of these statements is correct:

  • Send of ALTITUDE is required and should be the only place GCS gets altitude telemetry from
  • Send of ALTITUDE is optional and if not sent a GCS should fall back to GLOBAL_POSITION_INT for altitudes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Call Issues to be discussed during the Dev Call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants