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: Add new MAV_SYS_STATUS_TERRAIN_FOLLOW flag #346

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Davidsastresas
Copy link

This address the current issue of not having good mavlink feedback about terrain following

@peterbarker
Copy link

@hamishwillee I asked David to create this so we could discuss this at our DevCall to see if it would be fit for our purpose. It's supposed to be analagous to the "attitude stabilization" flags in here - i.e. is the vehicle currently following terrain.

We were also thinking of adding one for "am I currently actively avoiding something and modifying my trajectory because of that". @Davidsastresas we should probably discuss both at once, I think. There's the open question of whether that flag would always be set when running Dijkstra's.

@Davidsastresas
Copy link
Author

Davidsastresas commented Dec 29, 2023

I understand what you mean. So we have present, enabled and health flags.

The question is, we want to indicate to the GCS when any kind of avoidance is "active", or we want to indicate just when the particular avoidance type is actually taking control of the aircraft?

Of course ideally we should have both. As we can deal with standard proximity integrating properly the MAV_SYS_STATUS_SENSOR_PROXIMITY in Ardupilot, I suggest we do the same for OA avoidance:

  • present: OA_TYPE is set to other than 0
  • enabled: OA is actually taking control of the aircraft
  • health: any kind of trouble with OA

Or maybe we can unify both, standard avoidance and more complex OA, and deal with more details on OA later, on a different set of messages. This would be pretty cool, as right now all the setups working with this use custom mavlink to report in detail the status of OA. Maybe at some point it is worth to open this discussion.

To be honest I didn't thought at all about OA, but you are totally right, now that we are opening this discussion we should give this a thought.

@hamishwillee
Copy link

hamishwillee commented Dec 29, 2023

As you say MAV_SYS_STATUS_* has bit arrays for present, enabled, health. I guess that made sense for the sensors it was designed for - a sensor might be present but not enabled, but if enabled and healthy is always in use. That's not the case for other systems that this is used for, where they can be in use but not actively and obviously doing anything.

I think what you are proposing here it that "present" is reimagined as "present and enabled in some mode", and enabled means "active" - right? So you're dropping the idea of being able to know from the message that it is "present but not enabled" - so you can't tell as a GCS that this is an option - right?

Personally I'd prefer not to mess with the current meaning - because it complicates the message to suggest you have to read the description to understand what "present" or "enabled" mean.

If we actually want to know that something is actively doing its thing we could add another array for "active" at the cost of wasting most of the array for all the sensors. That's a pretty big array.

Or we could have specific messages - in the same way that proximity has COLLISION.

Where to start with this is to look at the use cases that might need to output "actively doing stuff". If there are just one or two then specific messages make sense.

If there are many then maybe the extra array or maybe something added to COMPONENT_INFORMATION_BASIC - i.e. you get the message so you know it is present, but you could have flags for enabled and healthy or active, or whatever. Of course that only works for things that have a mavlink presence. Not advocating, just thinking about options.

So what are the cases we might need this for?

@Davidsastresas
Copy link
Author

Davidsastresas commented Jan 5, 2024

Thanks for the answer @hamishwillee . You are correct, that is exactly what I meant. But after reading your thoughts, I agree with you, it doesn't seem totally right.

So, about the features we need better mavlink support, it is:

  • avoidance: meaning having actual avoidance system actively "monitoring", and being ready to take control anytime. Of course it would be ideal if besides that, we could also know when the avoidance system is actually in control of the vehicle to avoid an obstacle. On top of this, now that we are probably rethinking all of this, it would be great to have a mavlink command to enable/disable this. We could take advantage of the extra fields to also specify at the same time useful settings, like distance to objects, etc.

  • terrain following: meaning in the current flight mode, or current mission, the vehicle is tracking the ground altitude, actively following it.

From my experience, there are particular use cases that could really benefit a lot of better mavlink support for this, like agricultural sprayers. They alternate non terrain following for getting to the places to spray, and then they do terrain following during operation, because the altitude to the ground is critical for how the product arrives to the crops.

So it would be great to have feedback about when vehicle is following terrain or not, and also a dedicated mavlink command to enable/disable this would be great.

What do you think? Do you think it is worth it to make dedicated messages and commands for these 2 features? Or we should find a workaround in Ardupilot?

Thank you very much!

@hamishwillee
Copy link

Hi @Davidsastresas
I will have to think on this a little more. Just a few thoughts today while on another job, and before wandering off to pub.

  1. I was going to say terrain following works with the existing present, enabled, failed status bits because while enabled it is following terrain. But I think I'm probably wrong - you can have terrain following enabled but be too high for it to work right? Certainly this is true on PX4 when using a range sensor.

  2. On PX4 obstacle avoidance, collision prevention, and terrain following, are enabled using parameters. They can be used in different modes (which is a pity, because otherwise you might argue they be implemented and started as unique modes, and you could tra

    My personal view is that it makes sense for these cases to be enabled/disabled using a MAVLink mechanism. If there is support for this in ArduPilot I am happy to also pursue in PX4.

  3. I am not sure what mechanism. speaking probably utter drivelOff the top of my head

    • a command to enabled these, which takes as a parameter MAV_SYS_STATUS_SENSOR indicating the sensor or "all sensors", an id in case there are multiples, and a last parameter to indicate if it is to be enabled or disabled. We would need to think about risks here. Again, stream of conciousness.
    • A new message that outputs the active status of features in that array.
    • OR for both a new command for enabling and a new message for publishing enabled status.

I'll think some more. In an ideal world we want something easy to grow, and lightweight.

Copy link

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

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

This seems good to me.

@davidbuzz
Copy link

looks good to me.

@peterbarker
Copy link

UTC1100 - https://github.com/ArduPilot/mavlink/pull/346
Sys status terrain follow bit
Set when the vehicle is terrain following
Should be set when using laser rangefinder or the terrain database
no objections on call to the flag
Peter will read comments from David and Hamish and bring it back to another DevCall

@hamishwillee
Copy link

UTC1100 - https://github.com/ArduPilot/mavlink/pull/346
Sys status terrain follow bit
Set when the vehicle is terrain following
Should be set when using laser rangefinder or the terrain database
no objections on call to the flag

To be clear, this makes sense as described, which interprets the bit in-line with all the other status information. In other words, you can use it to determine if the vehicle has a terrain-following system, if that system is enabled, and if that system is healthy.

  • You can't use it to tell if terrain following is actually being used or engaged.
  • If you were using a rangefinder and were above it's supported range, you would fall back to using a baro or similar - in this case it would still report present, active, healthy even though you are not terrain following at that precise time.

@hamishwillee
Copy link

hamishwillee commented Jan 9, 2024

@Davidsastresas PS I don't really have any further thoughts on #346 (comment) whether we need separate starter commands, separate notification of terrain/avoidance systems being used, or if need to work out some kind of generic system.

My gut feeling though is that separate starter/stop commands and monitoring messages is the simplest solution. But it really comes down to whether @peterbarker thinks the flash cost of new separate messages is worth the benefit.

@Davidsastresas
Copy link
Author

Thanks for the insight!

I agree, separate commands and messages make the most sense, but I understand the implications of flash cost.

Just to recap, In the worst case scenario, if flash cost is really a problem, my understanding is that the additional sys_status flag for terrain following is approved, and the mav_cmd definitions don't consume flash, correct? So we would only have the problem of feedback when it is actually on, actually "controlling vehicle".

So the current discussion of adding or not new mavlink definitions would only affect feedback about the systems ( avoidance, terrain following ) actually "taking control" over the vehicle.

Considering this, for Avoidance we really could use COLLISION message for this matter, as it has everything we need, only we would need to add a new field to MAV_COLLISION_SRC for onboard sensors, something like MAV_COLLISION_SRC_PROXIMITY_SENSORS as right now only adsb and mavlink gps are present.

So in short, the above decision will only affect feedback about terrain following actually doing terrain following, correct?

@hamishwillee
Copy link

@Davidsastresas

  1. This PR change needs to be done upstream in the mavlink/mavlink repo. But I have no doubt that as described it will be approved - it is no different than other systems we have in that enum.
  2. As you say, this PR will tell us if the system is present and active, but not if terrain is actually being followed in a particular instance.

Yes, the rest of the discussion is about:

  • Enabling features using a standard mechanism rather than using parameters (as currently done by both ArduPilot and PX4). In particular terrain following, collision prevention, obstacle avoidance.
  • Publishing telemetry about the state of a particular service, in this case we're interested in whether the service is "actively controlling" as opposed to "active".

I know that adding a new message is more costly than extending a message. I think that is because there are framework costs of supporting a new message. Supporting a new enum value or command should also have some cost, but I suspect that it is likely to be relatively much lower because most of the framework cost is paid. I'm interested if this is indeed a correct set of assumptions @peterbarker.

So in summary ...

Yes, I think adding the mav_cmd definitions is going to be relatively cheap. You'll be writing a handler for the incoming message in the autopilot that internally sets the associated parameters or rejects if the command, if for example you don't have a range sensor.
One reason I think dedicated commands work here is that you might have both terrain map and sensor support, so having a dedicated message makes it more obvious what the options are. I am assuming an autopilot can work out whether it has support for various types of terrain following enabled.

Yes, adding new messages is more expensive. Extending COLLISION is possible - create a PR upstream and we can discuss.

So in short, the above decision will only affect feedback about terrain following actually doing terrain following, correct?

Yes.

@Davidsastresas
Copy link
Author

Thank you very much @hamishwillee. Let's have some more feedback from Ardupilot dev team to see how we can proceed. Thanks!

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.

5 participants