Use return INFINITY for FixedwingLandDetector::_get_max_altitude().

[mcsauder]

Describe problem solved by the proposed pull request
This PR adds a return value to the LandDetector _get_max_altitude() method declaration so that implementations can be deleted from the FixedWingLandDetector and RoverLandDetector classes. (Updated by #11874)

This PR simplifies the FixedwingLandDetector _get_max_altitude() logic by using the LandDetector base class logic return default. A few #includes in the land_detector module that can be deleted are also removed.

Please let me know if you have any questions on this PR. Thanks!

-Mark

[dagar]

I believe the reason it was done this way was so that the land detector publications would be throttled. On publishing on change or every second. https://github.com/PX4/Firmware/blob/02b92674aeba3a76f7e3e852182a9efe50fba40b/src/modules/land_detector/LandDetector.cpp#L105

[mcsauder]

Hi @dagar, I've submitted this because it reflects work accomplished in your vehicle_imu PR, #9756. How would you like me to proceed?

[mcsauder]

Hi @dagar , I will attempt to outline the direct impacts of this PR to help evaluate if it should be used or not:

FLT_EPSILON 1.1920929e-07F

No impacts in the case of Rover/LandDetector.
const float alt_max = 0.0 > 0.0f ? _get_max_altitude() : INFINITY; = INFINITY
if (fabsf(_land_detected.alt_max - alt_max) > FLT_EPSILON)) = FALSE

Taking into account previous _land_detected.alt_max value could be set; so if previously set = INFINITY - INFINITY = 0 = FALSE

If any of the other criteria are satisfied, the result is unchanged with this PR:
_land_detected.alt_max = alt_max; = INFINITY

Impacts in the case of FixedWing/LandDetector:
const float alt_max = roundf(-_vehicle_local_position.z + 10000) > 0.0f ? _get_max_altitude() : INFINITY;
if (fabsf(_land_detected.alt_max - alt_max) > FLT_EPSILON))

We can place real numbers into the code for examining. Assume that the current local position is a small value, using -1m (negative Z direction for positive altitude) is sufficient for illustration:
const float alt_max = 10001 > 0.0f ? _get_max_altitude() : INFINITY; = 10001
if (fabsf(_land_detected.alt_max - 10001) > FLT_EPSILON)) = FALSE until the aircraft descends below the previous value, (positive Z displacement).

If any of the other criteria are satisfied,
_land_detected.alt_max = 10001; = 10001

Iterate after positive Z displacement has occurred:
const float alt_max = 9999 > 0.0f ? _get_max_altitude() : INFINITY; = 9999
if (fabsf(10000 - 9999) > FLT_EPSILON)) = TRUE
_land_detected.alt_max = 9999; = 9999

For all practical purposes, the value will always stick somewhere close to 10,000.
If this PR is accepted, the value will get pushed to infinity.
const float alt_max = INFINITY > 0.0f ? _get_max_altitude() : INFINITY; = INFINITY
if (fabsf(INFINITY) > FLT_EPSILON)) = TRUE
_land_detected.alt_max = INFINITY; = INFINITY

Hopefully this is useful.

[julianoes]

@mcsauder

INFINITY - INFINITY = 0 = FALSE

Really? Not NaN? Did you test it? I'm mostly curious.

[mcsauder]

Hi @julianoes , thanks!

@mcsauder

INFINITY - INFINITY = 0 = FALSE

Really? Not NaN? Did you test it? I'm mostly curious.

You are right about that! (Luckily, the logic impact is the same.) Here is supporting data and test code for convenience:

#include <stdio.h>
#include <math.h>
#include <stdint.h>
#include <inttypes.h>
#include <string.h>

int main()
{
	float result = INFINITY - INFINITY;
	uint64_t result_uint64 = 0;
	memcpy(&result_uint64, &result, sizeof result);
	printf("result:   %f %" PRIx64 "\n", result, result_uint64);
}

result: nan 7fc00000

[mcsauder]

Rebased on current master.

[julianoes]

@mcsauder thanks for laying that all out!

Ah, so Inf - Inf is NaN and NaN > Epsilon is False and thus no publication. I think this is sane then but so abstract!

[mcsauder]

Thanks @julianoes !