Skip to content

Home position listener #646

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

Merged
merged 2 commits into from
Jul 15, 2016
Merged

Conversation

georgehines
Copy link
Contributor

Adds a listener to set the vehicle's home location when the autopilot broadcasts it (after it is set internally in the autopilot).

@ghost
Copy link

ghost commented Jul 14, 2016

This works as intended with Solo ShotManager. My only concern is that the _home_location also gets set on a MISSION_ITEM message, silently overwriting this.

Seems confusing. IMO the home location should be APM's home location and the waypoint_0 location should take another name. But this is up to someone who knows how this is used more than me and George.

@mrpollo can you please review this and merge with any necessary changes ASAP? Thanks!

@hamishwillee
Copy link
Contributor

hamishwillee commented Jul 14, 2016

@nick3dr @mrpollo My understanding is that the fact that the Waypoint 0 location is used as the home location is a historical bug, but unfortunately one that everyone now depends on. The actual value of both should be the same (though the home position message contains additional information about the approach vector). Using the HOME_POSITION message is a much better approach for dronekit because it doesn't require the download of (potentially large) missions in order to get the home position - and the consequently different behaviour for users when accessing this information. Of course it won't be supported in every autopilot build and we need to compensate for that. We also need to support requesting the value and setting the value, not just reading it.

Therefore code will need to be added to:

  • determine if there is support and conditionally disable the waypoint_0 based information
  • add the approach vector information, setting values of attributes to None if this cannot be collected.
  • send the getter message appropriately (I don't know when that should be, depends on whether autopilot sends it automatically)
  • Change our setter method to set the value appropriately (assuming supported) rather than by writing to waypoint_0
  • Update our documentation :-)

@ghost
Copy link

ghost commented Jul 14, 2016

Thanks for the review @hamishwillee. Does any of that block this PR?

@hamishwillee
Copy link
Contributor

@nick3dr It shouldn't, provided the autopilot(s) don't stop populating waypoint_0 along with this change.

@georgehines
Copy link
Contributor Author

No functional changes to APM were made along with this change. APM sends
the HOME_POSITION message when it sets its internal home position in the
AHRS (at least in arducopter), so this listener merely picks up that
broadcast.

On Thu, Jul 14, 2016, 5:04 PM Hamish Willee [email protected]
wrote:

@nick3dr https://github.com/nick3dr It shouldn't, provided the
autopilot(s) don't stop populating waypoint_0 along with this change.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#646 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APV4SsOuqW6H4edGO9SE97elMM-FbTlXks5qVs55gaJpZM4JMzPC
.

@hamishwillee
Copy link
Contributor

@georgehines Yes. My small concerns are that:

  • someone will in future remove waypoint_0 storage of the value before proper checks are made on versioning ... on either ArduPilot or PX4 stacks.
  • The job will never be done through to completion

It's probably not worth delaying this being included.

@mrpollo
Copy link
Member

mrpollo commented Jul 15, 2016

as @hamishwillee mentions theres no need to delay merge.

@peterbarker I'm going ahead and merging, but could you chime in on this thread with @hamishwillee and myself on what the proper way to handle backwards compat with APM/PX4 on issues like this is?, we might need a plan soon

@mrpollo mrpollo merged commit 51d64c8 into dronekit:master Jul 15, 2016
@hamishwillee
Copy link
Contributor

@mrpollo @peterbarker The extra work required can be discussed in #649. It should be pretty easy.

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

Successfully merging this pull request may close these issues.

3 participants