Implements LocationGlobalRelative.

[tcr3dr]

See #306.

@hamishwillee What is a good test here, other than checking (on copter) that is_relative is False for .location.global_frame and True for .home_location?

[hamishwillee]

@tcr3dr Well, if you get the current location when you're at the home position (immediately after this is set) that should have alt==0. The alt of the home location should be (usually) significantly greater than zero.I can't think of anything else.

As noted in #306 this is all reasonable for Copter which only supports global and global relative frames, but may break on Plane, which also supports terrain following mode. It will also break on the local locations because we have no way to specify that a value is relative to the home location or to the vehicle current position.

[tcr3dr]

@hamishwillee So just to be clear:

1. is_relative=True means relative to ground and is_relative=False means AMSL.
2. Our .location.global_frame property is fed by GLOBAL_POSITION_INT.alt, so that should always be is_relative=False (to MSL).
3. Our waypoints (thus home location) are relative to given frame and we can parse this from the frame property of the waypoints themselves (though I believe we don't right now.)

Bullet #3 seems like it needs to be fixed. What other scenario would need to be correct? I'm also confused about how to behave in Plane but that is probably an omission from the MAVLink message docs.

[hamishwillee]

Hi @tcr3dr

We probably need to know more about what ardupilot thinks it is sending. As you say, "is_relative=True means relative to ground" and this is what is returned (GLOBAL_POSITION_INT.relative_alt). But it is possible that ardupilot doesn't know where the ground is by default, so it is actually returning relative to home if it doesn't know height and relative to ground if it does. This would sort out the plane issue for terrain following.
It is also possible that it is returning the position above home for the .alt too!

All guesses of course ... we need to ask/test.

With respect to the other points ...if our global location is fed from GLOBAL_POSITION_INT.alt then yes, is_relative=False (ALT is actually fed from VFR_HUD right - which says it is in MSL?). BUT, why then are we actually seeing values that are relative to home - ie the alt of zero returned when sitting on the home position (which is IMO what we want)?

[tcr3dr]

Considering the difficulty in getting this right, and the testing required, perhaps we want to actually strip the is_relative property from DKPY2 release and integrate it in a minor point release later? I think we should start over and consider what frame of references we are using and how we expose them to users. By convention, most people have figured it out through usage what frames they are using, even though is_relative has been incorrect in DKPY1.

[hamishwillee]

@tcr3dr IMO We need to do this testing of what is in GLOBAL_POSITION_INT in order to confirm what information is being supplied in the alt value in the docs - however we deal with is_relative.

What I hope is really sent in GLOBAL_POSITION_INT is both a relative and MSL value as indicated by the mavlink docs. The ideal change to the API would then be to expose both of these (rather than exposing one of them and using Bool to tell us which). The problem of course then being how we set a new location alt with such an API ... probably accept either (converting where needed) and note in docs that if both are supplied only one will be used.

Yes we need to look at frames more broadly, especially in the context of goto functions that support velocity and/or position control, and relative to the vehicle itself.

I'd like to do this testing. Can we leave this issue open and unfixed for DKPY2 release until I get around to it?

[tcr3dr]

@hamishwillee Sounds okay. I'd rather remove the is_relative attribute entirely but maybe the best course of action is to just leave it alone. So, I'll leave this PR open for now, but unassign our milestone.

[hamishwillee]

@tcr3dr OK, I have done due diligence and I think we DO need to do "something" for DKPY2.0 because we have changed the meaning of location from DKPY 1 in an unhelpful way.

So what we used to have was a Location class which reported location using a boolean is_relative that tells you whether the height is above ground or not.
Now we return a Global location that is *always relative to MSL. This is a big problem because any code that thinks about alt in the old terms (above Home position) will need to be modified - and because it is much easier to think in terms of "above ground". We do also have a local_frame, which does give us height above home position, but position in horizontal plane is relative to home position too - so annoying if you want to work in gps co-ordinates.

In addition, we get the location position from GLOBAL_POSITION_INT but we get the alt (in MSL) from VFR_HUD. This is a bug - we should get the alt from GLOBAL_POSITION_INT, though we might choose to ALSO get it from the VFR_HUD.

A little aside here about what the vehicle actually supplies before I explain my proposed solution.

The vehicle supplies GLOBAL_POSITION_INT which includes the altitude in both relative and msl terms. The relative_alt is documented as "above the ground". Looking through the ardupilot code I believe that if it has a lidar or a terrain map vs GPS it will give you the actual height above ground. If you don't have those then it isn't so obvious how it works this out - but on Copter it appears that it uses inertial movement the home location ... and there is some other code there which adds and removes home location from the returned alt. I suspect that this is where the idea that the relative_alt is relative to home location comes from.

I think the best solution is to give users what they are familiar with. That is, to remove the is_relative flag from LocationGlobal and create a new class LocationGlobalRelative which is the same as LocationGlobal but with the relative_alt from GLOBAL_POSITION_INT. Obviously we'll need a new attribute global_relative_frame. When setting a location in goto the code will need to be smart enough to work out what type of location is being passed. Initially we could just pass our new LocationGlobalRelative.
The home location should be a LocationGlobal. .location should return a LocationGlobalRelative

[hamishwillee]

Hi @tcr3dr

Nicely done. I updated the alt so that the values are divided by 1000 (making them metres as for previous version).

I've updated the documentation for all the associated classes in the init.py. Can you please sanity check.

Problems:

1. If you start dronekit-sitl and run the vehicle_state.py example you will see that the global_frame.alt is reported as 0 (as is the relative alt). A further test shows that the gps signal is reporting as "good" at this point. After about 6 seconds the value updates to a sensible value. It would be good to find out why, and force that alt to be None until we have a valid altitude. This may be tough, because of course 0 is a valid altitude.
2. local_frame is Noneall the time (by experimentation)
3. Creating an observer on global_frame, global_relative_frame, local_frame still isn't working. The observer for location works great. (by experimentation)
4. You've removed is_relative from the API but it is still set in the init for both location classes. Do we actually need it?

This update to docs is not comprehensive. I will update everything else when I'm sure that the functionality is confirmed.

[tcr3dr]

1. Fixed local_frame to not be None (typo!)
2. Added a test for @vehicle.location.on_attribute('global_frame'), so see that that is successful.
3. Removed is_relative from the API and fixed goto to accept either LocationGlobal or LocationGlobalRelative.

Now investigating global_frame.alt...

[tcr3dr]

I could not find any plausible condition to detect global_frame.alt being zero, so I just detected that the first global_frame alt value must be non-zero. Since it varies considerably over a few seconds, I think this is a good enough solution that is also marked by a TODO.

[tcr3dr]

I think this PR is in good shape to test again.

[hamishwillee]

Hi @tcr3dr

@tcr3dr - This works as stated ... but of course I have more statements/questions.

Re ensuring the first value of alt in the global must be non-zero, works fine on SITL. What would happen if you armed at sea level?

The location attributes (e.g. Vehicle.location.global_frame) are None until populated. It feels like the members of Vehicle.location.global_frame should be None until it is populated. Similarly the local_frame is None until populated. Would have thought it's members would be None.
Thoughts?

Is it possible or a good idea to make Vehicle.location.global_frame.alt None but display the other values since we know them (even though alt is not populated). That feels more sensible to me ....

I updated simple_goto.py to use global_frame_relative for reporting alt during takeoff. The alt values are all integers - previously these were floats. IMO this is a problem - we need a precision of better than metres.

I updated most of the examples but only tested a few of them ... more to do once you come back to me.

[tcr3dr]

@hamishwillee It's speculative but the altitude value seems to vary pretty quickly—I suppose if someone were arming at sea level and had a perfect barometer, we would have issues. But otherwise the error threshold gets crossed in under a half second. Also, we can address this bug properly in the future pending more investigation.

I'm good with creating *_frame and leaving its values unpopulated. If this weren't how we did it on Vehicle, I would say this is a worser design—but we have a precedent and Python is lenient.

I'm not sure about populating .lat and .lon without .alt. Example: we can only emit one event for "global_frame". Is that when we have a 2D or 3D fix? Given our high-level abstraction I think it breaks the contract that a Frame should always be populated. I think having a global_2d_frame would be better if one wanted to deal with that case.

The floats issue is a bug and we can test for regressions.

[hamishwillee]

@tcr3dr

1. OK re altitude value measurement at sea-level

2. I'm good with creating *_frame and leaving its values unpopulated. If this weren't how we did it on Vehicle, I would say this is a worser design—but we have a precedent and Python is lenient.

   I agree, it is a bit ugly. The only benefit is that for the example code we end up getting values we can print to give people and idea of "what is coming" even if the values are none (e.g. for local_frame we get nothing until after arming).

   If we are "allowed" to break, I'd be tempted to do this right and return None for all attributes until we have a value. If not, lets be consistent. Your call.

3. Re "I'm not sure about populating .lat and .lon without .alt. ". Agreed, lets leave "as is". For the examples I think I'll add some additional waiting code for the empty values. Probably better to show users that there are delays they need to compensate for.

Yes, looking forward to your fix for the integer alt!

Thanks!

[tcr3dr]

@hamishwillee This PR is updated. Location objects always exist and just have None values when not populated.

This actually has the side effect of .lat and .lon being populated independent of .alt, but the event is only emitted if we have all three. I think this is fine and we should document accordingly—the notification event is emitted only when we have a complete location.

Take a look and lmk if this is good to merge.

[hamishwillee]

@tcr3dr Yes, this PR looking good. I've updated the docs appropriately. Feel free to merge.

Note, this was quite a big change, affecting lots of examples. In particular, breaking compatibility might have got us. I won't feel confident on this until I retest all the examples.