AP_RangeFinder: Benewake sends max-range+1m when out of range

[rmackay9]

This PR corrects two issues discovered during Copter-3.6.1-rc1 testing (see discussion here):

• The Benewake lidar driver returns the RNGFND_MAX_CM + 1m if the lidar goes out of range. Previously it would ignore these readings but this would lead to the lidar's state becoming "NoData" when really it's "OutOfRangeHigh". The 1m addition to the defined maximum range is a bit arbitrary but I wanted to ensure that it's significantly beyond the maximum so that no rounding errors could lead to it being considered within range. Below is a screen shot of a test where the RNGFND_MAX_CM was set to 1500 so the lidar reports 16m when out-of-range.

• Copter's extended system status message to the GCS reports the lidar as healthy as long as it's returning data. Previously the lidar was only reported healthy if the lidar range was within the MIN and MAX. In my view, "healthy" means the lidar is operating as expected and no action is required from the user. It's a normal and expected thing in most situations that the lidar will go out of range of the ground so I don't think it's good to report the lidar as "unhealthy" in these situations.

[OXINARF]

Second part is OK.

First part I would prefer to see it use the signal_ok, after all, that's precisely why it was there. It seems it was misused in the update method; in my opinion we only to change:

• get_reading to initialize signal_ok to false
• update to:

  bool signal_ok = false;
  if (get_reading(state.distance_cm, signal_ok)) {
    state.last_reading_ms = AP_HAL::millis();
    update_status();
  } else if (signal_ok) {
    set_status(RangeFinder::RangeFinder_OutOfRangeHigh);
  } else if (AP_HAL::millis() - state.last_reading_ms > 200) {
    set_status(RangeFinder::RangeFinder_NoData);
  }
  

[rmackay9]

@OXINARF, thanks very much for the review!

[rmackay9]

@OXINARF, thinking about this a bit more.. the proposed code won't quite work because last_reading_ms won't be updated so the sensor's health will go bad. I.e. it's status will soon be set to NoData..

So I'm still thinking about how to handle this..

[rmackay9]

@OXINARF, how do you feel about this change? Instead of making more use of "signal_ok" I've removed it and in cases where we were using it, we now just set the distance to out-of-range. I also made a couple of other minor simplifications.

Here's a graph of the RFND.Dist1 during a test after making this change.

[OXINARF]

@rmackay9 How does it set the out-of-range status? It looks like the value would be used as valid, unless the user explicitly sets the max range which triggers the out-of-range status; we rely on that on some drivers, but I think that in drivers where we know it is out-of-range we shouldn't require it.

[rmackay9]

@OXINARF, once the update_status() call is made it will check that the new distance reading vs the "max_distance_cm" (aka RNGFND_MAX_CM) and because the distance was set to "max_distance_cm + 1m" it will set the status to OutOfRangeHigh.

The max distance is defaulted to 700cm and it always needs to be defined. It's not one of those parameters where setting it to zero removes the limit.

[OXINARF]

Ok, makes sense. Can we add the following in the constructor?

state.max_distance_cm.set_default(BENEWAKE_DIST_MAX_CM);

This way if the user doesn't set it, we set the default to a more sane value.

By the way, squashing third commit with first might be a good idea.

thanks for the review!

[rmackay9]

Thanks I've squashed and created an issue to better default the max range for all lidar.

I'll merge this when travis and semaphore say it's OK.