Added autodetect of baro and compass on external bus #7803

night-ghost · 2018-02-28T07:12:43Z

fixes this

WickedShell · 2018-02-28T14:10:54Z

One thing to be worrisome to me is that if anyone is trying to build this for plane that the MS5525 differential sensor is indistinguishable from the MS5611 which could lead to airspeeds being treated as barometers (which does not work out well at all)

night-ghost · 2018-02-28T15:56:38Z

That's why my HAL don't allows to create a 2nd device with the same address on same bus, so if MS5525 will be detected first there will not be a problem.

WickedShell · 2018-02-28T16:05:35Z

We detect baro's first at the moment rather then airspeed sensors. This is somewhat a larger issue as the two sensors will always be easily conflated with each other. (It's why the airspeed library allows you to specify which of I2C address the sensor is on, but I don't think the baro allows you that sort of control).

night-ghost · 2018-02-28T16:42:20Z

We detect baro's first at the moment rather then airspeed sensors

But who prevents to change order?

OXINARF · 2018-02-28T18:26:18Z

libraries/AP_Baro/AP_Baro.cpp

 #endif
    }

+#if CONFIG_HAL_BOARD != HAL_BOARD_F4LIGHT // most boards requires external baro


I have no comment on removing the need for a baro, but the comment is wrong. The code below doesn't require an external baro nor any board that I know of requires it; it simply requires that at least one baro is connected.

User should can to use board without any baro. Arming check will prevent him from flight anyway. So comment is about that most boards for F4light don't has builtin baro

it would be nice to support boards with no baro in limited flight modes (eg. ACRO mode in copter). It would probably be quite an intrusive change though.

I thought about it and tried to do it, but it took too long

OXINARF · 2018-02-28T18:30:13Z

libraries/AP_Baro/AP_Baro.cpp

@@ -535,6 +535,21 @@ void AP_Baro::init(void)
                                      std::move(hal.spi->get_device(HAL_BARO_LPS22H_NAME))));
 #endif

+#ifdef HAL_BARO_MS5611_I2C_BUS


Why are all these lines below needed instead of each board simply defining HAL_BARO_DEFAULT? Unless I missed something, this code is repeated above, except it checks HAL_BARO_DEFAULT value.

because most boards don't have a baro so user solders it itself, so we don't know what baro will be

OXINARF · 2018-02-28T18:34:52Z

libraries/AP_Baro/AP_Baro.cpp

@@ -546,12 +561,23 @@ void AP_Baro::init(void)
 #else
        ADD_BACKEND(AP_Baro_MS56XX::probe(*this,
                                          std::move(hal.i2c_mgr->get_device(_ext_bus, HAL_BARO_MS5611_I2C_ADDR))));
+
+ #if CONFIG_HAL_BOARD == HAL_BOARD_F4LIGHT // we don't know which baro user will solder


Maybe not needed for now, but we should add an external driver enable mask parameter here; the issue is that people might not want to enable the MS56XX driver as it conflicts with other device types (as @WickedShell said). Also these baros below might be used in other boards as well.

I don't affect other boards, I need to fix user's issue

OXINARF · 2018-02-28T18:41:25Z

libraries/AP_Compass/AP_Compass.cpp

@@ -834,6 +834,10 @@ void Compass::_detect_backends(void)
    ADD_BACKEND(DRIVER_HMC5883, AP_Compass_HMC5843::probe(*this, hal.i2c_mgr->get_device(HAL_COMPASS_HMC5843_I2C_BUS, HAL_COMPASS_HMC5843_I2C_ADDR),
                                                          false, HAL_COMPASS_HMC5843_ROTATION),
                AP_Compass_HMC5843::name, false);
+ #ifdef HAL_COMPASS_HMC5843_I2C_EXT_BUS
+    ADD_BACKEND(DRIVER_HMC5883, AP_Compass_HMC5843::probe(*this, hal.i2c_mgr->get_device(HAL_COMPASS_HMC5843_I2C_EXT_BUS, HAL_COMPASS_HMC5843_I2C_ADDR),true


This line seems to be missing some code at the end

Thanks, fixed

OXINARF · 2018-02-28T18:49:36Z

libraries/AP_Compass/AP_Compass.cpp

@@ -877,6 +881,41 @@ void Compass::_detect_backends(void)
    #error Unrecognised HAL_COMPASS_TYPE setting
 #endif

+
+#ifdef BOARD_I2C_BUS_EXT


I get the reason, but two points:

the external compasses should probably be detected before the internal compass

we should this correctly, read HAL - i2c detect mask #6748 and HAL: define method to get I2C buses to probe #6265 (there are probably other comments elsewhere); with that implemented, there would be no reason to not probe all drivers for external buses for all boards, which would greatly reduce the code duplication.

should probably be detected before the internal compass

but why? They are completely independent

read #6748 and #6265

those PRs are hangs for half of year, while my users lost the ability to update right now, so I want to restore full functionality

but why? They are completely independent

not quite, the EKF uses the first one in preference, and external compass is usually preferred (otherwise why did the user bother to attach it?)

why did the user bother to attach it

because most boards don't contains internal one. With per-motor compensation builtin compass gives very good results.

IIRC user can select the main compass in MP?

night-ghost · 2018-03-01T03:20:08Z

should probably be detected before the internal compass

but why? They are completely independent

read #6748 and #6265

those PRs are hangs for half of year, while my users lost the ability to update right now, so I want to restore full functionality

tridge · 2018-03-01T05:30:26Z

That's why my HAL don't allows to create a 2nd device with the same address on same bus, so if MS5525 will be detected first there will not be a problem.

unfortunately we need to have two device handles for one device in some cases. For example, the lsm303d sensor is both a mag and an accel on the same bus/address, separated by register range. The way our driver structure works we attach to the device twice. It's safe as we use thread per bus, plus of course the semaphore.
Perhaps we should disallow attaching twice with the same bus/address except if you make some API call to allow it?

night-ghost · 2018-03-01T05:33:48Z

Perhaps we should disallow attaching twice with the same bus/address

We can create one I2c device and reuse in two drivers, so as for compass via MPU just to get device for compass from IMU driver.

UPD. LSM303D is on a SPI bus, not on I2C

tridge · 2018-03-01T10:07:19Z

UPD. LSM303D is on a SPI bus, not on I2C

it can be on either - just that nobody has been silly enough to put it on I2C yet
The problem with doing this as a general rule without an override is it will bite someone badly in the future. There are a lot of crazy sensors out there.

night-ghost · 2018-03-01T10:14:02Z

Autodetect is very useful feature so I think that wil be better to alter API

night-ghost · 2018-03-02T19:38:45Z

There are a lot of crazy sensors out there.

OK. I do HAL for normal sensors and don't want to know about crazy ones. So let my HAL will have autodetect, pleeeeeeeeeeeeeeeeeeeeease!

hiro2233 · 2018-03-04T13:40:32Z

@night-ghost

night-ghost · 2018-03-04T15:17:51Z

Sorry I don't understand you

night-ghost · 2018-03-06T14:30:15Z

What issues still present in this PR?

tridge

The remaining issue with this PR is that it may break things for boards that don't use HAL_F4Light.
In particular, the addition of the probe for the MS5611, the BMP280 and the BMP085 without a check for HAL type could break some HAL_PX4 boards. I suspect it won't, but for this sort of change we'd need to do testing on quite a wide range of boards. We support a wide combination of sensors on the various px4-v3 boards, and we've carefully tested that it comes out right. If you put that part inside a check for HAL_F4Light then it would be fine.
The changes to AP_Compass look safe to me, and can be applied as-is.
I also think the change to not give sensor_config_error() when a baro isn't found is not a good idea. If we could support flight without a baro then it would be OK, but right now we don't. The sensor_config_error() at least gives the user a clear error message as to what is wrong. Without that check the user will get a more confusing error in startup, and probably won't get a mavlink connection, so they won't have the option to look at their board parameters to fix it.

tridge · 2018-03-06T22:34:41Z

btw, I think our whole system of #ifdefs for compass, baro and INS drivers needs an overhaul. It really is quite messy and hard to maintain. We need to live with it for now, but I am thinking of trying a different approach as we start to support a wider range of boards

night-ghost · 2018-03-07T04:51:43Z

If we could support flight without a baro then it would be OK, but right now we don't. The sensor_config_error() at least gives the user a clear error message as to what is wrong. Without that check the user will get a more confusing error in startup

No, this was checked many times! User got "baro not healthy" with working MAVlink with refuse to arm. Just try it yourself.
BTW, in the near past absense of baro was causing a panic()

our whole system of #ifdefs for compass, baro and INS drivers needs an overhaul

Sure. Now all things checked by board's name while the best way to check by feature, defined in board's config.

this PR is that it may break things for boards that don't use HAL_F4Light

changed baro driver to autotest only if BOARD_I2C_BUS_EXT defined (which no one existing HAL does except HAL_F4Light)

sh83 · 2018-04-03T10:19:53Z

Inav/betaflight had sensor autodetection on external bus a years ago. Current hardcoded sensor detection is simply awful. This pr (partially) fixes it.

tridge · 2018-04-11T04:16:48Z

I've rebased on master and changed the #ifdefs so its clearer this is just F4Light (while the define was specific to F4Light, that wasn't obvious to someone reading the code)
I've pushed to the PR branch and once this passes CI we can push to master

night-ghost · 2018-04-11T04:34:46Z

This PR is not intended only fo F4Light, it will be useful for any HAL working on racer's boards without built-in baro. So may be it will be better to define something like BOARD_BARO_AUTODETECT in HAL and check it in baro driver?

all fixed

tridge · 2018-04-11T05:00:14Z

@night-ghost I'm planning on taking a new approach for driver listing in the future for the ChibiOS build. but for now I'll take the cautious approach and do it just for F4Light so I'm sure it doesn't break anything

night-ghost · 2018-04-11T05:43:07Z

@tridge you removed checks like '#ifdef HAL_BARO_MS5611_I2C_BUS' so now all baros tries to detect regardless of board. It's wrong!

night-ghost requested review from tridge and rmackay9 February 28, 2018 07:13

night-ghost force-pushed the fix4 branch from c19b320 to d01330b Compare February 28, 2018 07:17

OXINARF added the Library label Feb 28, 2018

OXINARF reviewed Feb 28, 2018

View reviewed changes

night-ghost force-pushed the fix4 branch from d01330b to 0f09394 Compare March 1, 2018 04:20

night-ghost force-pushed the fix4 branch from 0f09394 to f40246e Compare March 1, 2018 10:26

night-ghost closed this Mar 3, 2018

night-ghost reopened this Mar 4, 2018

tridge previously requested changes Mar 6, 2018

View reviewed changes

night-ghost force-pushed the fix4 branch from f40246e to 16f266a Compare March 7, 2018 04:49

night-ghost added 2 commits April 11, 2018 14:12

AP_Baro: added autodetection of popular baro types on external bus

39f916e

AP_Compass: added autodetect of all known compasses on external bus

85c6307

tridge force-pushed the fix4 branch from 16f266a to 85c6307 Compare April 11, 2018 04:15

tridge merged commit 677f2be into ArduPilot:master Apr 11, 2018

night-ghost deleted the fix4 branch April 11, 2018 06:23

Added autodetect of baro and compass on external bus #7803

Added autodetect of baro and compass on external bus #7803

Conversation

night-ghost commented Feb 28, 2018

WickedShell commented Feb 28, 2018

night-ghost commented Feb 28, 2018

WickedShell commented Feb 28, 2018

night-ghost commented Feb 28, 2018

Choose a reason for hiding this comment

night-ghost Mar 1, 2018 • edited Loading

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

night-ghost Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

night-ghost commented Mar 1, 2018 • edited Loading

tridge commented Mar 1, 2018

night-ghost commented Mar 1, 2018 • edited Loading

tridge commented Mar 1, 2018

night-ghost commented Mar 1, 2018

night-ghost commented Mar 2, 2018

hiro2233 commented Mar 4, 2018

night-ghost commented Mar 4, 2018

night-ghost commented Mar 6, 2018

tridge left a comment

Choose a reason for hiding this comment

tridge commented Mar 6, 2018

night-ghost commented Mar 7, 2018 • edited Loading

sh83 commented Apr 3, 2018

tridge commented Apr 11, 2018

night-ghost commented Apr 11, 2018

tridge commented Apr 11, 2018

night-ghost commented Apr 11, 2018

night-ghost Mar 1, 2018 •

edited

Loading

night-ghost Mar 1, 2018 •

edited

Loading

night-ghost commented Mar 1, 2018 •

edited

Loading

night-ghost commented Mar 1, 2018 •

edited

Loading

night-ghost commented Mar 7, 2018 •

edited

Loading