Use AP::airspeed() to get airspeed sensor, rather than going via AP_AHRS #15108

peterbarker · 2020-08-20T03:18:26Z

Setting the pointer in the AHRS predates our use of singletons.

Some of the users of this should probably be failling back to using a synthetic airspeed estimate instead of giving up if we don't have a true airspeed sensor.

Tools/Replay/Replay.cpp

magicrub · 2020-08-20T04:40:39Z

libraries/AP_AHRS/AP_AHRS_DCM.cpp

@@ -697,7 +697,7 @@ AP_AHRS_DCM::drift_correction(float deltat)
        }
        float airspeed;
        if (airspeed_sensor_enabled()) {
-            airspeed = _airspeed->get_airspeed();
+            airspeed = AP::airspeed()->get_airspeed();


Needs a null check. Not all vehicles instantiate airspeed

airspeed_sensor_enabled() does that. Further, we're already dereferencing a pointer :-)

I do want to rename airspeed_sensor_enabled() to using_airspeed_sensor().

I meant AP_::airspeed() could be nullptr

No, it can't; airspeed_sensor_enabled checks for nullptr. It already checks _airspeed for nullptr, and has been changed to check the singleton instead.

This is the same argument that was discussed on yesterday's dev call. The function should return the result when true, in this case a non-null ptr. Having a function check something, and then at a later time it is assumed true. That's not thread safe.

Where the check is done is irrelevant. If you want to do a nullptr check before calling a method on it then you need some sort of lock on that pointer or critical section of code.

magicrub

needs nullptr checking

magicrub · 2020-11-24T22:19:33Z

libraries/AP_AHRS/AP_AHRS.h

@@ -285,12 +277,14 @@ class AP_AHRS
    // return true if airspeed comes from an airspeed sensor, as
    // opposed to an IMU estimate
    bool airspeed_sensor_enabled(void) const {
+        const AP_Airspeed     * _airspeed = AP::airspeed();


unnecessary whitespace

magicrub · 2020-11-24T22:19:42Z

libraries/AP_AHRS/AP_AHRS.h

        return _airspeed != nullptr && _airspeed->use() && _airspeed->healthy();
    }

    // return true if airspeed comes from a specific airspeed sensor, as
    // opposed to an IMU estimate
    bool airspeed_sensor_enabled(uint8_t airspeed_index) const {
+        const AP_Airspeed     * _airspeed = AP::airspeed();


unnecessary whitespace

magicrub · 2020-11-24T22:21:46Z

libraries/AP_AHRS/AP_AHRS_DCM.cpp

@@ -697,7 +697,7 @@ AP_AHRS_DCM::drift_correction(float deltat)
        }
        float airspeed;
        if (airspeed_sensor_enabled()) {
-            airspeed = _airspeed->get_airspeed();
+            airspeed = AP::airspeed()->get_airspeed();


This is the same argument that was discussed on yesterday's dev call. The function should return the result when true, in this case a non-null ptr. Having a function check something, and then at a later time it is assumed true. That's not thread safe.

magicrub · 2020-11-24T22:23:51Z

libraries/AP_AHRS/AP_AHRS_DCM.cpp

@@ -1049,14 +1049,14 @@ bool AP_AHRS_DCM::get_position(struct Location &loc) const
 // return an airspeed estimate if available
 bool AP_AHRS_DCM::airspeed_estimate(float &airspeed_ret) const
 {
-    return airspeed_estimate(_airspeed?_airspeed->get_primary():0, airspeed_ret);
+    return airspeed_estimate(AP::airspeed()?AP::airspeed()->get_primary():0, airspeed_ret);


should not be checking if non-zero, should explicitly looking for nullptr

avoid the double call to singleton:
auto *airspeed = xxxxx;

magicrub · 2020-11-24T22:24:15Z

libraries/AP_AHRS/AP_AHRS_DCM.cpp

@@ -1049,14 +1049,14 @@ bool AP_AHRS_DCM::get_position(struct Location &loc) const
 // return an airspeed estimate if available
 bool AP_AHRS_DCM::airspeed_estimate(float &airspeed_ret) const
 {
-    return airspeed_estimate(_airspeed?_airspeed->get_primary():0, airspeed_ret);
+    return airspeed_estimate(AP::airspeed()?AP::airspeed()->get_primary():0, airspeed_ret);


Suggested change

return airspeed_estimate(AP::airspeed()?AP::airspeed()->get_primary():0, airspeed_ret);

return airspeed_estimate((AP::airspeed() != nullptr)?AP::airspeed()->get_primary():0, airspeed_ret);

magicrub · 2020-11-24T22:24:44Z

libraries/AP_AHRS/AP_AHRS_DCM.cpp

 }

 // return an airspeed estimate from a specific airspeed sensor instance if available
 bool AP_AHRS_DCM::airspeed_estimate(uint8_t airspeed_index, float &airspeed_ret) const
 {
    if (airspeed_sensor_enabled(airspeed_index)) {
-        airspeed_ret = _airspeed->get_airspeed(airspeed_index);
+        airspeed_ret = AP::airspeed()->get_airspeed(airspeed_index);


AP::airspeed() needs nullptr check

This is checked in airspeed_sensor_enabled.

I'm looking at refactoring a whole bunch of this stuff and would prefer to not make too many changes. Note that _airspeed here is already a pointer, so the nullptr check better work already!

magicrub · 2020-11-24T22:24:53Z

libraries/AP_AHRS/AP_AHRS_NavEKF.cpp

@@ -2231,6 +2231,7 @@ uint8_t AP_AHRS_NavEKF::get_active_airspeed_index() const
    }
 #endif
    // for the rest, let the primary airspeed sensor be used
+    const AP_Airspeed     * _airspeed = AP::airspeed();


unnecessary whitespace

magicrub · 2020-11-24T22:25:30Z

libraries/AP_Frsky_Telem/AP_Frsky_SPort_Passthrough.cpp

@@ -516,7 +516,7 @@ uint32_t AP_Frsky_SPort_Passthrough::calc_velandyaw(void)
    AP_AHRS &_ahrs = AP::ahrs();
    WITH_SEMAPHORE(_ahrs.get_semaphore());
    // horizontal velocity in dm/s (use airspeed if available and enabled - even if not used - otherwise use groundspeed)
-    const AP_Airspeed *aspeed = _ahrs.get_airspeed();
+    const AP_Airspeed *aspeed = AP::airspeed();
    if (aspeed && aspeed->enabled()) {


needs explicit nullptr check instead of non-null

It's modifying a line which isn't modified already in this PR.

I've just changed the way the pointer is fetched. This pointer will never be nullptr more often that the existing code!

Similarly in the other telem backends.

Those are all probably wrong anyway - they should be asking the AHRS for the estimate, not the sensor.

magicrub · 2020-11-24T22:26:02Z

libraries/AP_LTM_Telem/AP_LTM_Telem.cpp

@@ -121,7 +121,7 @@ void AP_LTM_Telem::send_Sframe(void)
    const uint16_t amp = (uint16_t) roundf(current * 100.0f);                          // current sensor (expects value in hundredth of A)

    // airspeed in m/s if available and enabled - even if not used - otherwise send 0
-    const AP_Airspeed *aspeed = AP::ahrs().get_airspeed();
+    const AP_Airspeed *aspeed = AP::airspeed();
    uint8_t airspeed = 0; // airspeed sensor (m/s)
    if (aspeed && aspeed->enabled()) {


needs explicit nullptr chk instead of nonzero

As elsewhere, unrelated to my PR. Not a bad change, just unrelated.

magicrub · 2020-11-24T22:26:10Z

libraries/AP_RCTelemetry/AP_Spektrum_Telem.cpp

@@ -435,7 +435,7 @@ void AP_Spektrum_Telem::calc_airspeed()
    AP_AHRS &ahrs = AP::ahrs();
    WITH_SEMAPHORE(ahrs.get_semaphore());

-    const AP_Airspeed *airspeed = ahrs.get_airspeed();
+    const AP_Airspeed *airspeed = AP::airspeed();
    float speed = 0.0f;
    if (airspeed && airspeed->healthy()) {


needs explicit nullptr chk instead of nonzero

As elsewhere, unrelated to my PR. Not a bad change, just unrelated.

tridge · 2020-11-25T07:44:27Z

libraries/AP_AHRS/AP_AHRS_DCM.cpp

@@ -1049,14 +1049,14 @@ bool AP_AHRS_DCM::get_position(struct Location &loc) const
 // return an airspeed estimate if available
 bool AP_AHRS_DCM::airspeed_estimate(float &airspeed_ret) const
 {
-    return airspeed_estimate(_airspeed?_airspeed->get_primary():0, airspeed_ret);
+    return airspeed_estimate(AP::airspeed()?AP::airspeed()->get_primary():0, airspeed_ret);


avoid the double call to singleton:
auto *airspeed = xxxxx;

peterbarker added the NeedsTesting label Aug 20, 2020

magicrub reviewed Aug 20, 2020

View reviewed changes

peterbarker force-pushed the pr/remove-ahrs-set-airspeed branch from d6d9d13 to 26f52c1 Compare August 28, 2020 02:32

peterbarker force-pushed the pr/remove-ahrs-set-airspeed branch 4 times, most recently from e579eb1 to 7e2843d Compare October 1, 2020 08:33

peterbarker force-pushed the pr/remove-ahrs-set-airspeed branch from 7e2843d to cd10d2c Compare November 11, 2020 04:11

peterbarker force-pushed the pr/remove-ahrs-set-airspeed branch from cd10d2c to 00b09f1 Compare November 24, 2020 11:15

peterbarker added DevCallEU and removed NeedsTesting labels Nov 24, 2020

magicrub requested changes Nov 24, 2020

View reviewed changes

tridge requested changes Nov 25, 2020

View reviewed changes

tridge removed the DevCallEU label Nov 25, 2020

peterbarker added 5 commits November 29, 2020 09:21

AP_AHRS: get airspeed sensor from singleton not AHRS object

8e0e7e0

AP_Frsky_Telem: get airspeed sensor from singleton not AHRS object

cccbc25

AP_LTM_Telem: get airspeed sensor from singleton not AHRS object

5fa9d18

AP_RCTelemetry: get airspeed sensor from singleton not AHRS object

888aa39

ArduPlane: let AHRS get the airspeed sensor from the singleton

018bbbc

peterbarker force-pushed the pr/remove-ahrs-set-airspeed branch 2 times, most recently from d56148f to 018bbbc Compare November 28, 2020 22:25

peterbarker added the DevCallTopic label Dec 4, 2020

tridge approved these changes Dec 7, 2020

View reviewed changes

CraigElder removed the DevCallTopic label Dec 7, 2020

magicrub approved these changes Dec 7, 2020

View reviewed changes

magicrub merged commit 5fae265 into ArduPilot:master Dec 7, 2020

peterbarker deleted the pr/remove-ahrs-set-airspeed branch December 8, 2020 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use AP::airspeed() to get airspeed sensor, rather than going via AP_AHRS #15108

Use AP::airspeed() to get airspeed sensor, rather than going via AP_AHRS #15108

peterbarker commented Aug 20, 2020

magicrub Aug 20, 2020

peterbarker Aug 20, 2020

magicrub Nov 24, 2020

peterbarker Nov 24, 2020

magicrub Nov 24, 2020

peterbarker Nov 27, 2020

magicrub left a comment

magicrub Nov 24, 2020

peterbarker Nov 27, 2020

magicrub Nov 24, 2020

peterbarker Nov 27, 2020

magicrub Nov 24, 2020

magicrub Nov 24, 2020

tridge Nov 25, 2020

peterbarker Nov 28, 2020

magicrub Nov 24, 2020

peterbarker Nov 28, 2020

magicrub Nov 24, 2020

peterbarker Nov 28, 2020

magicrub Nov 24, 2020

peterbarker Nov 28, 2020

magicrub Nov 24, 2020

peterbarker Nov 28, 2020

magicrub Nov 24, 2020

peterbarker Nov 28, 2020

magicrub Nov 24, 2020

peterbarker Nov 28, 2020

tridge Nov 25, 2020

	return airspeed_estimate(AP::airspeed()?AP::airspeed()->get_primary():0, airspeed_ret);
	return airspeed_estimate((AP::airspeed() != nullptr)?AP::airspeed()->get_primary():0, airspeed_ret);

Use AP::airspeed() to get airspeed sensor, rather than going via AP_AHRS #15108

Use AP::airspeed() to get airspeed sensor, rather than going via AP_AHRS #15108

Conversation

peterbarker commented Aug 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicrub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment