Clarify FollowMe::Config::min_height_m and follow_dist_m

[hamishwillee]

This states:

float min_height_m = 8.0f - Min follow height, in meters.

My guess would be that this is actually not minimum follow height, but actual/desired follow height. So when we specify a target position (e.g. 0 AMSL) the height that the drone will follow will be 8m above that - correct?

If so, then this should be renamed follow_height_m.

1. Can we set negative values? i.e. to follow from below/under a target?

We should also rename follow_dist_m to follow_distance_m for consistency.

[shakthi-prashanth-m]

My guess would be that this is actually not minimum follow height, but actual/desired follow height. So when we specify a target position (e.g. 0 AMSL) the height that the drone will follow will be 8m above that - correct?

Yes. I understood so from PX4 Follow Me guide.

Can we set negative values? i.e. to follow from below/under a target?

As per PX4 Full Parameter Reference for Follow target, NAV_MIN_FT_HT is of type float; and we can't set negative values: Min: 8.0 meters.

If so, then this should be renamed follow_height_m.

We should also rename follow_dist_m to follow_distance_m for consistency.

Sure we can. I wanted to keep it consistent with PX4 parameter names as people may be familiar with them.

[hamishwillee]

@shakthi-prashanth-m @julianoes Thank you. Now I explore this in detail, I think our implementation of the minimums and config is wrong.

What we do now is have hard coded minimum values for the various follow parameters. These match whatever the documented default follow-me parameter values are in PX4. So for example, the minimum follow height is set, by default, to 8 metres. We allow a user to set whatever height they like in the config but they will be constrained to the minimum.

The problems with this are:

• Anyone can change the value of these parameters on PX4 at any time. A user might then be unable to reach a value that is supported on PX4, or might request a value that is below the minimum, but we won't get warning
• The value is hard coded so changing requires a recompile.

What I think we should do is fetch these minimum values from the PX4 parameters rather than hard coding.
We might also allow the value to be set through the API, though that is optional.
Last of all, the config value should be changed to reflect that it is the desired height, not a minimum height.

Make sense?

[shakthi-prashanth-m]

@hamishwillee I agree a user can change follow-me config from QGroundControl. When user sets value out-of-range, he/she will be warned as below.

Anyone can change the value of these parameters on PX4 at any time. A user might then be unable to reach a value that is supported on PX4, or might request a value that is below the minimum, but we won't get warning

What we fetch from PX4 may not be minimum values. But may be changed to different values outside (for. eg: from QGroundControl).

What I think we should do is fetch these minimum values from the PX4 parameters rather than hard coding.

We ensure that we start with default Follow Me configuration by applying default parameters during Follow Me startup. I believe that it is safer approach than fetching current PX4 configuration. We can choose what is right way here.

[hamishwillee]

We ensure that we start with default Follow Me configuration by applying default parameters during Follow Me startup.

That is a valid alternative too. In that case though, we should make them something that is set as part of Config.
I don't have a strong opinion about which approach is better. Maybe @julianoes does.

[hamishwillee]

@shakthi-prashanth-m @julianoes Did you guys agree what needs to be done with this?

[julianoes]

I think DroneCore would have to use the same xml file for parameter limits in order to know these values. Right now that's not the case so I would vote to have a safe default and make sure this is tested in CI soon.

I vote to change names to human readable such as distance since we don't have the same 16 char parameter length constraints.

[hamishwillee]

I think DroneCore would have to use the same xml file for parameter limits in order to know these values. Right now that's not the case so I would vote to have a safe default and make sure this is tested in CI soon.

It can never know them even with xml file, because they can be changed via GCS. I'm OK with having a safe default as long as we actually set it - as it is now you can get an inconsistency between the vehicle set value and dronecode. If we're going to set it though, then why not make it a configurable default?

I vote to change names to human readable such as distance since we don't have the same 16 char parameter length constraints.

+1

[shakthi-prashanth-m]

If we're going to set it though, then why not make it a configurable default?

@hamishwillee sorry I didn't get your point about making default configuration. As said earlier, we already have default configuration set, regardless of current PX4 configuration.

[shakthi-prashanth-m]

I vote to change names to human readable such as distance since we don't have the same 16 char parameter length constraints.

I shall make changes.

[hamishwillee]

If we're going to set it though, then why not make it a configurable default?

@hamishwillee sorry I didn't get your point about making default configuration. As said earlier, we already have default configuration set, regardless of current PX4 configuration.

At the moment we just set some value DroneCode side to control what we send to the vehicle. We don't care what PX4 has set. @julianoes suggested having a sensible minimum - and I hope he means that we should write that minimum to the vehicle (?).
What I am saying is that if we're going to be writing a parameter to the vehicle anyway, lets make it something that can be set through the API. We can't know what are sensible minimums for all use cases.

[shakthi-prashanth-m]

OK, Yes. Although we set default PX4 Follow Me configuration to start with; users can always override existing configuration before starting Follow Me (optional).

[hamishwillee]

Yes. Does this override actually change the min height on the vehicle?

[shakthi-prashanth-m]

@hamishwillee Yes it does.

[hamishwillee]

Awesome. I'll go disappear then :-)