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.xml: MISSION_CURRENT progress notification #2029

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hamishwillee
Copy link
Collaborator

This attempts to design a solution #2023 for reporting the progress in individual waypoints, in particular for loiter progress.

It adds MISSION_CURRENT.progress and .progress_final which are uint_16s that represent the progress and final progress to be reached. The units of these values depend on the mission item - I have added some possible values to waypoint and loiter waypoints.

  • WAYPOINT - start distance, final distance in metres
    • Do we need this? The GCS can infer this easily from telemetry data for current position relative to initial/final position.
  • MAV_CMD_NAV_LOITER_UNLIM - progress and progress_final fields to 0.
    • I don't think we need anything here. The GCS knows it is loitering forever from the type. It knows when it is travelling to the waypoint and can infer when it has reached it from the type of vehicle and radius etc. We can measure the time taken so far, and there is no end point.
    • The only thing we might provide is number of turns already done - but since that isn't useful for progress, IMO not better to set to zero.
  • MAV_CMD_NAV_LOITER_TIME - progress and progress_final fields to 0.
    • Do we need progress for this? I've said no for same reason as previous case - a GCS can work it out from knowledge of the mission item and current vehicle position.
    • The only exception is if you connect when the vehicle has already been waiting.
  • MAV_CMD_NAV_LOITER_TURNS - number of turns completed, and total number of turns.
    • It is harder for GCS to estimate complete turns but it can actually work this out.
    • Again, only a problem if you connect after starting mission or lose telemetry.

@julianoes @sypaq-nexton So from above, for travel between points you can work out progress from knowledge of the mission and knowledge of current position of the vehicle.
The exceptions are loiter time and loiter turns, which you can work out from GCS if you are always connected, but not if you lose telemetry at some point.

The values can be zero'd if we expect the info to come from the GCS so no particular additional cost.

Anyway, have a think - is this "good". Are there other things that would need non-zero values, or be better if were non-zero values?

@AndreasAntener
Copy link
Contributor

What about number of repetitions from MAV_CMD_DO_JUMP?

@hamishwillee
Copy link
Collaborator Author

What about number of repetitions from MAV_CMD_DO_JUMP?

It is true that this isn't exposed anywhere, and if you aren't always connected to the vehicle there is no way of guessing where you are at in the jump cycle without this kind of information.

I can kind of imagine how you might use this to show a repeating route - ie mark it in some kind of separate colour and have a counter display near the start or end. A generalization of tracking turns. Maybe. Thanks.

@AndreasAntener
Copy link
Contributor

AndreasAntener commented Aug 17, 2023

We would have wished for this a few times during testing, in endurance flights after lap 15 nobody knows the count anymore for sure ;)

But I realise now that, even tough your proposal is well generalised, the issue with the jump count is that you are not staying at the same waypoint. So the MISSION_CURRENT message might not even be sent out for it.

@AndreasAntener
Copy link
Contributor

After thinking more about this, the jump repeat count is more a mission level (or "lap") state, not a waypoint state. It would be misuse of this new waypoint progress indicator. The better approach for repeat updates might be to provide a partial mission update to the GCS when the count changes.

@julianoes
Copy link
Collaborator

After thinking more about this, the jump repeat count is more a mission level (or "lap") state, not a waypoint state.

I'm pretty sure when we first implemented goto on the PX4 side we made the mission count down, so if you were to download the mission from the vehicle, the count would go down accordingly. It's not pretty but it seemed a way to transmit that information. I'm not sure that's still how it works though, I'd have to test it. And we should probably note/spec it either way.

@nexton-winjeel
Copy link
Contributor

nexton-winjeel commented Aug 18, 2023

Thanks for this @hamishwillee and @julianoes.

I would prefer if the Vehicle was the source of truth for this data, rather than having the GCS infer it, for two reasons:

  • it greatly simplifies the implementation of the GCS; and
  • as @hamishwillee mentioned above, it means this information remains correct regardless of when the GCS connects. I can see this becoming important as gimbals/cameras become more prevalent, as I can see a use case where one operator is controlling the Vehicle and another operator is connecting periodically to monitor/control the camera.

I also think we should have an enum that defines the type for these progress fields. The specific use case I'm thinking of is MAV_CMD_NAV_WAYPOINT, where we have both distance to the waypoint and a hold time once it reaches the waypoint. With the current design, there is no way to report both of these.

And I think the following also need a progress:

  • MAV_CMD_NAV_DELAY: time remaining and total time (in seconds)
  • MAV_CMD_CONDITION_DELAY: time remaining and total time (in seconds)

and possibly also:

  • MAV_CMD_NAV_RETURN_TO_LAUNCH: this could be either time remaining or distance remaining...

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Aug 24, 2023

@julianoes I think you are saying that you get the mission lap by downloading the whole mission, looking for MAV_CMD_DO_JUMP instances. Each time a jump iterates, you count the repeat number down?

That feels pretty heavyweight. We could use items for this and force a jump item to emit a MISSION_CURRENT with a count. Otherwise likely you'd miss them (I assume MISSION_CURRENT may emit non waypoint mission items, such as camera triggers, but generally it won't because those all get executed very quickly).

And I think the following also need a progress: MAV_CMD_NAV_DELAY, MAV_CMD_CONDITION_DELAY

Yes. no way to infer them from telemetry.

MAV_CMD_NAV_RETURN_TO_LAUNCH: this could be either time remaining or distance remaining...

Maybe. You can kind of see this on the map already - its not like delays and turns for which it is hard to work out what is going on from telemetry.

The specific use case I'm thinking of is MAV_CMD_NAV_WAYPOINT, where we have both distance to the waypoint and a hold time once it reaches the waypoint. With the current design, there is no way to report both of these.

Maybe. Or you could say that progress is not reported for anything except the hold time. Of course you'd still want the type in this case (and I think that does make things a little easier, for the cost of just one byte).

</description>
<field type="uint16_t" name="seq">Sequence</field>
<extensions/>
<field type="uint16_t" name="total" invalid="UINT16_MAX">Total number of mission items on vehicle (on last item, sequence == total). If the autopilot stores its home location as part of the mission this will be excluded from the total. 0: Not supported, UINT16_MAX if no mission is present on the vehicle.</field>
<field type="uint8_t" name="mission_state" enum="MISSION_STATE" invalid="0">Mission state machine state. MISSION_STATE_UNKNOWN if state reporting not supported.</field>
<field type="uint8_t" name="mission_mode" invalid="0">Vehicle is in a mode that can execute mission items or suspended. 0: Unknown, 1: In mission mode, 2: Suspended (not in mission mode).</field>
<field type="uint8_t" name="item_progress_units">0: not used. 1: distance (m), 2: time (seconds), 3: count (laps)</field>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note, to replace with enum if whole strategy agreed.

@hamishwillee
Copy link
Collaborator Author

OK, updated in line with discussion so far. Does not consider jumps in mission.

message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Aug 30, 2023

@sypaq-nexton We discussed in mav call last night:

  • Use case is valid and having in MISSION_CURRENT makes sense.
  • Might as well provide a percentage as well.
  • Count down is a little more useful than count up. In theory you could just have count down in the message since the recipient should know how many items there are - but even if they didn't it would tell them the useful information "how long have you got". But we figure it's easier for the recipient not to have to look for the info in the mission.
  • EDIT - but then count up allows open ended counting for an unlimited loiter where we have no end point. IMO we don't allow that - the time in loiter or turns for an unlimited operation does not matter. What matters is say battery life so should not affect message.
  • There was a thought that most people aren't as interested in counts etc as "how long until the mission moves to the next item" - so waypoint travel time + wait time. That is probably correct, but could be fairly hard to estimate.
  • This is intended for things that can't be inferred, such as hold and loiter times remaining. I think we should probably restrict use to those cases - i.e. no expectation and perhaps actively discourage publishing the travel time to a waypoint. If we did that then we could allow a message to perhaps emit time rather than counts for a turns loiter.

hamishwillee and others added 3 commits August 31, 2023 08:51
Co-authored-by: Nick Exton <NExton@sypaq.com.au>
Co-authored-by: Nick Exton <NExton@sypaq.com.au>
Comment on lines +5329 to +5331
<field type="uint8_t" name="item_progress_units" enum="MISSION_ITEM_PROGRESS_UNITS">Units for remaining/original progress fields. MISSION_ITEM_PROGRESS_UNITS_NOT_USED (0) if not reporting progress. Units depend on particular mission item.</field>
<field type="uint8_t" name="item_progress_percentage" units="%" invalid="UINT8_MAX">0-100: Percentage remaining (countdown). 0 if progress not being reported (item_progress_units is MISSION_ITEM_PROGRESS_UNITS_NOT_USED).</field>
<field type="uint16_t" name="item_progress_remaining" invalid="0">Remaining time/distance/count to complete current mission item (units in item_progress_units). 0: progress not reported or item complete.</field>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sypaq-nexton @julianoes @auturgy (FYI @MaEtUgR ) I've just updated the docs in line with our discussion on the dev call plus some minor mods based on what is actually needed.

This reports both percentage and remaining progress as a countdown, and omits the "initial count". We're really interested in how long we have to wait or our percentage through - we don't care so much what we started with - and if we do, then we can get it from the mission item.

We only report this for things that you can't infer - such as loiter/delay.

We don't report this for travel time in waypoints, which can be inferred. We don't report it for unlimited delays - they have no "progress'.

I am quite happy with this, with one minor caveat. It is harder to report "turns=3/5" as you would have to estimate by dividing by the percentage and rounding to a whole number (note, you do not have to worry about this for time based delays since remaining time or % are how you would report those). I think the saving of 2 bytes and the simplicity of what gets reported is worth it.

Does this work for everyone?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can work with this. Thanks!

@nexton-winjeel
Copy link
Contributor

@hamishwillee wrote:

EDIT - but then count up allows open ended counting for an unlimited loiter where we have no end point.

If you really wanted to allow counting up, you could potentially capture it in the MISSION_ITEM_PROGRESS_UNITS, e.g.. you could have both:

  • MISSION_ITEM_PROGRESS_UNITS_[UNIT]_ELAPSED and
  • MISSION_ITEM_PROGRESS_UNITS_[UNIT]_REMAINING.

But I'm not sure the value justifies the extra complexity.

@hamishwillee
Copy link
Collaborator Author

But I'm not sure the value justifies the extra complexity.

Yes. It doesn't to me.

@hamishwillee
Copy link
Collaborator Author

@auturgy @julianoes You OK with this too? Or are we waiting on a @nexton-winjeel to create an implementation and then we merge (this has to be on common.xml, so if we approve, it's live).

@hamishwillee
Copy link
Collaborator Author

@nexton-winjeel We discussed this in the call last night. This has "general approval" as a workable system.

Because we don't like merging things into common.xml until there is some testing/prototyping we need to see this appearing in a flight stack/GCS before we merge. Do you have a plan/timeline for that implementation?

FYI that there is another PR for mission_current related to detecting mission changes, so the actual position of fields might change depending on which gets prototyped/accepted first.

@nexton-winjeel
Copy link
Contributor

@hamishwillee: It's on my roadmap. Planned for sometime this year, but I don't have a firmer date than that.

@hamishwillee
Copy link
Collaborator Author

Thanks @NaterGator - much appreciated.

@nexton-winjeel
Copy link
Contributor

@hamishwillee, @auturgy, @julianoes:

Initial implementation for QGC and ArduPilot available at:

(Note that neither of these will build because they don't include the MAVLink changes in this PR).

But with the changes in this PR, it looks like the following:
qgc-item-progress-distance
qgc-item-progress-count
qgc-item-progress-time

I'll update those branches and raise PRs once this PR is merged.

@hamishwillee
Copy link
Collaborator Author

@nexton-winjeel So in a loiter you're reporting distance to destination then number of turns remaining? Didn't we agree we only reported the things you couldn't work out from GCS. I'm happy to discuss/modify, we just shouldn't do stuff we decided against. See

We don't report this for travel time in waypoints, which can be inferred. We don't report it for unlimited delays - they have no "progress'.

@nexton-winjeel
Copy link
Contributor

@hamishwillee: These QGC and ArduPilot changes don't have to be the final solution. It was done this way to demonstrate that:

  • Progress can be reported for various mission item types;
  • Each item progress unit type can be correctly reported and displayed; and
  • Mission Items can report different unit types throughout execution.

@nexton-winjeel
Copy link
Contributor

@hamishwillee:

Because we don't like merging things into common.xml until there is some testing/prototyping we need to see this appearing in a flight stack/GCS before we merge.

Does this mean you need a proof-of-concept like I've done, or that you need this functionality merged into a flight stack/GCS?

@hamishwillee
Copy link
Collaborator Author

These QGC and ArduPilot changes don't have to be the final solution. It was done this way to demonstrate that ..

I'm not saying I don't like this, I'm saying that the implementation must be compliant with the specification. That specification currently says:

  • The progress units, such as distance, time, count (for example, of "remaining turns"), must be defined in the mission item.
  • MISSION_CURRENT should set the item_progress_remaining field to the remaining loiter time (s) once loitering.
  • Nothing is mentioned about distance, but in this discussion we explicitly said "only report on things that you can't infer".

Don't get me wrong, I like what you've done and I don't think it breaks anything, as long as there is no expectation in the GCS that this information will be provided. Just making sure that when someone does an implementation it is compliant, and if we do choose to provide distance in an implementation, the docs say that is OK.

In summary "the spec should be something all flight stacks can develop to and likely end up with the behaviour".

Because we don't like merging things into common.xml until there is some testing/prototyping we need to see this appearing in a flight stack/GCS before we merge.

Does this mean you need a proof-of-concept like I've done, or that you need this functionality merged into a flight stack/GCS?

Usually it would need broad commitment from the flight stack to take your changes. If you have that, what you have done is probably enough. We'd discuss that in the dev call next week if you think you have that agreement? I.e. has Peter Barker or JamesP looked at this?

@hamishwillee
Copy link
Collaborator Author

@nexton-winjeel We discussed in the call. What is needed is a PR in ArduPilot (or PX4) so that we can see discussion and assess ArduPilot's commitment to taking this. We think it's "good" but we're wary of creating orphan fixes that never end up in a flight stack.

@nexton-winjeel
Copy link
Contributor

@hamishwillee: ArduPilot PR here: ArduPilot/ardupilot#25249

I'll discuss it with them in the next dev call.

@hamishwillee
Copy link
Collaborator Author

Thanks! Good luck. I would like this as part of the standard.

Copy link
Member

@tridge tridge left a comment

Choose a reason for hiding this comment

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

This only works in AUTO, but the same information also makes sense in RTL, GUIDED, QRTL and many other modes. How will they be handled?
maybe this should be a new message, which is requested via message intervals by the GCS?
this proposal also doesn't work for ship landing, doesn't work for precland doesn't work for follow, in each case the destination is not in the mission. Also doesn't work for rally points, again missing the destination
also, how does the flight controller know if the user wants time or distance?
in a new msg it could have float time and float distance, with NaN for unknown

@nexton-winjeel
Copy link
Contributor

Following discussion with @tridge and the ArduPilot devs, we probably want a new message that looks something like:

    <message id="11045" name="NAV_CONTROLLER_PROGRESS">
      <description>Progress of the current Nav Controller item.</description>
      <field type="uint8_t" name="percentage_complete">Percentage complete. Set to 0 if unknown.</field>
      <field type="float" name="distance_remaining" units="m" invalid="NaN">Distance remaining (in metres) until the current Nav Controller item is complete. Set to NaN if unknown, or not applicable.</field>
      <field type="float" name="time_remaining" units="s" invalid="NaN">Time remaining (in seconds) until the current Nav Controller item is complete. Set to NaN if unknown, or not applicable.</field>
      <field type="float" name="count_remaining" invalid="NaN">Count remaining until the current Nav Controller item is complete. Set to NaN if unknown, or not applicable.</field>
      <!-- MAV_MISSION_TYPE field here so we can reference Rally points? -->
      <field type="uint16_t" name="seq">Sequence (mission item number). Set to 0 if not a mission item.</field>
      <field type="int32_t" name="x" invalid="INT32_MIN">X pos/Latitude of nav target. Set to INT32_MIN if no target, or to use the location of the corresponding mission item.</field>
      <field type="int32_t" name="y" invalid="INT32_MIN">Y pos/Longitude of nav target. Set to INT32_MIN if no target, or to use the location of the corresponding mission item.</field>
      <field type="float" name="alt" units="m" invalid="NaN">Altitude of nav target. Set to NaN if no target, or to use the location of the corresponding mission item.</field>
      <field type="uint8_t" name="frame" enum="MAV_FRAME">Coordinate frame of target.</field>
    </message>

@hamishwillee
Copy link
Collaborator Author

All excellent points. There are many other situations where the GCS can't know what the vehicle thinks it is doing so this is much better.

I don't know enough about ship landing or precland to know what you need there. Can you outline what the GCS would want to present for progress and what the vehicle would need to supply?

Ditto for Follow? Isn't that an unbounded operation - we decided that you would only report for times, distances that you could know. Follow is more like an unlimited orbit. Or to put it another way "what is the progress" in this case?

Also doesn't work for rally points, again missing the destination

So is the point here that you want to include a destination so the GCS always knows where the vehicle thinking it needs to go in cases such as rally points, where the vehicle makes a decision and does not communicate it? Nice

also, how does the flight controller know if the user wants time or distance?

We've been saying that it isn't up to the user. Are we OK to leave this up to the flight stack? Do we want to allow both as proposed

@peterbarker One reason I was thinking in the mission context was to avoid new messages/additional flash memory cost. What is ArduPilot's appetite for a message as proposed above.

@auturgy
Copy link
Collaborator

auturgy commented Nov 22, 2023

Discussed at Mavlink dev call again - all supportive of proposed changes/new message, waiting for update

@hamishwillee
Copy link
Collaborator Author

Note, above message should be created in a new PR in development.xml. As above, happy enough with the message proposed but would like to expand on what we expect it to be for various cases such as precland and ship landing etc so that we can be sure all the information needed is present.

@nexton-winjeel
Copy link
Contributor

This is on my backlog, but unlikely to get to it this side of Christmas.

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.

None yet

6 participants