[WIP] use local coordinates for position control (Global to local)

[ChristophTobler]

We thought it makes sense to create this PR to get an overview of the current state

What has been done:

• mission_item will be handed over to navigator_item (which still has lat/lon/alt, but we wanted to get rid of it -> needs to be checked with fixed wing). That means mission_item is only being used for reading mission information from the dataman. Since the mission_item are being stored, they need to be small whearas the size of the navigator_item does not matter.
• mc_pos_controller uses local coordinates

What needs to be done:

• General flight testing (only SITL so far)
• Fixed wing @dagar can you maybe have a look at that and test?
• Offboard maybe @mhkabir or @Stifael?
• LPE needs more testing
• Check capability for long missions (remains the same so far for MC)
• Global coordinate functions to local

Anything else?

[TSC21]

@ChristophTobler is it possible to add the feature I referenced in #7368, meaning adding the possibility of the global origin being set and the local origin have an offset to it? It would be useful for multi-entity systems and it would follow the ROS REP 105 regarding the coordinate frames (earth<->map<->odom<->base_link). This means that adding this feature will mean setting the earth frame, which will have an offset WRT map/odom (local origin).
I can add a commit against this PR with that implementation if needed.

[Stifael]

@TSC21 I still don't completely understand how that is not already known. Currently the local origin is set at the moment the vehicle is powered on, and will remain there until you turn off the vehicle and restart it.
In contrast, the global origin (=home position) is set once you have armed, and will be reset to a new global origin anytime the vehicle disarms and arms again. The offset, however, is known at any time, and in fact is used to convert global setpoints into local setpoints. Am I missing something?

[TSC21]

@Stifael probably this will help: http://www.ros.org/reps/rep-0105.html#earth. This is the coordinate frame I'm referencing. For a single vehicle, the global origin is equal to the local origin. But for multi-entities, that is not true because they don't have the same home_position. In a translation to what happen in ROS, for a single vehicle/robot, you don't require earth, eventhough you can state it as being coincident with map (global is equal to the local frame). But for multiple systems, like a swarm for example, that will not apply, and you should define a global origin (earth) which is the same to all the entities, while having a different offset between the home_position and this global origin.
Is it clear to you? This is not a most but it is a neat feature which for example we already support on MAVROS side. Would be good if we could follow up the implementation here. Would be something very simple I suppose cause we just need to add support to the msgs and at the same time keep track of the earth origin and the local offset to it in the navigator.

[dagar]

In the short term do we need the position controllers to accept both global and local?

[ChristophTobler]

not sure for FW. Global is in there because of FW. MC uses local only

[dagar]

I was referring to the publishing side. I can make FW work with local only.

[TSC21]

@ChristophTobler do you accept the proposed idea? I can PR the code soon if it makes sense to all.

[ChristophTobler]

@TSC21 Sure

[TSC21]

@ChristophTobler perfect. Will do it ASAP. Thanks

[BRNKR]

why is the local origin not set before arm. why at start of the system? why is there no possibility to reset the local position via mavlink/mavros?

[mhkabir]

The local origin is never supposed to change after the estimator initializes. If you want, you can capture the value of the local position at arm time and subtract that from the estimate.

[Stifael]

I rebased it to the newest master. It compiles, but I encountered at least one issue, which I currently don't understand. For whatever reason, the update check for local position is always false https://github.com/PX4/Firmware/blob/d4ac87e5edd3e241012b5d95dd2dd347841e4051/src/modules/navigator/navigator_main.cpp#L358-L364
If I ignore the check, then it works normally.
@dagar, do you know why the update check always fails?

[dagar]

The navigator loop is polling local on master now. https://github.com/PX4/Firmware/blob/master/src/modules/navigator/navigator_main.cpp#L277

If you're immediately doing an orb_check on local there probably won't be new data.

[Stifael]

@dagar what is the state of the FW?
next step is to remove all lat-lon computation within navigator. The MC is in a state where such a transition is possible.

[dagar]

I'm not sure, but if you'd like to push this through let's get it rebased on master and I'll make sure FW is working completely. It'll be easier to handle local only in fw_pos rather than figure out if the lat/lon is valid.

[dagar]

I won't suggest we tackle at the same time, but this is making me question the overall design.
We have mavlink items that the mavlink module receives, converts to mission_items and dumps in navigator. Now navigator pulls the mission_items and converts to navigator items? Maybe we should be directly storing the original mission.

Secondly, I think it would be fine to continue calling using _mission_item everywhere in navigator. This drastically reduces the size of the diff and gives downstream consumers a chance of actually merging.

[Stifael]

@bkueng , can you give more info about mission_item?
i was told that mission_item is primarily used as a storage structure for dataman. Consequently, it is not meant to be passed around through the entire navigator module. the new navigator_item was introduced to replace it with the goal to reduce it farther such that it only contains local setpoints. the only reason why the navigator_item still contains lat, lon and other stuff is because the first step is to remove the dependency to the mission_item. Once that is established and we succeeded in making a transition to local, we then can easily reduce the navigator item farther.

independent to this Pr: the navigator interface and local variables are anyway not well defined. In my opinion there has to be an interface for getting new mission related things (mission_item) and one interface that publishes the desired setpoints (position triplets). But right now mission_item and position_triplets are used all over the place, which makes it difficult to read the code in general.

[TSC21]

@ChristophTobler @Stifael @dagar I do think we should revisit this soon. Is it a good idea to bring this subject on the next dev call?

[Stifael]

@TSC21 sure

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[TSC21]

Still relevant

[julianoes]

FYI @bresch

[stale]

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

[Pedro-Roque]

@TSC21 @dagar @julianoes is this still relevant?

[bresch]

@Pedro-Roque Position control uses local position now