drivers/bmp180: during init retry getting ID because it may fail

[keestux]

This solves a problem I had when testing a RIOT port to a new board (SODAQ Autonomo) in combination with I2C. I connected a SODAQ TPH board.

Somehow the BMP is not ready directly after a reset. I couldn't find anything documented in the BMP datasheet. But retrying it (getting the device ID) seems a harmless solution.

[PeterKietzmann]

Hi @keestux thanks for the PR! Would you try something for me, probably it is interesting in an other context:
a)

i2c_write_byte(dev->i2c_dev, BMP180_ADDR, BMP180_REGISTER_ID);
xtimer_usleep(500);
i2c_read_reg(dev->i2c_dev, BMP180_ADDR, BMP180_REGISTER_ID, &checkid);

b) original code but without compiler optimization (just put CFLAGS_OPT = -O0 in the Makefile of your application)

I'm referring to issue #5460

[keestux]

Interesting, issue #5460. If I had known :-) I blamed my porting work for the Autonomo. It took me three days to get to this point. Read on, it gets interesting.

I've tried 4 cases. With and without -O0, with and without the usleep between the write and the read.

1. -O0 and usleep => FAIL
2. -O0 without usleep => FAIL
3. -Os and usleep => OK
4. -Os without usleep => OK

In other words, it is the -O0 that makes it fail! And the usleep doesn't make a difference. Yesterday I happen to have tried this myself, also with a 500us sleep. However, if the sleep is done AFTER the read it does help and the problem goes away.

I also found out that the BMP device could end up in a failure state, and it takes extra effort to get it out of it. Usually it is enough to keep the reset button pressed for a few seconds.

BTW. I was using -O0 to debug the code. That's why I was running into the problem.

[keestux]

One thing I need to mention is that I am working with a SAMD21 and with a newer set of include files from Atmel (ASF 3.30). That could have influence, but in this case I don't think it does.

[PeterKietzmann]

@keestux I'm a bit confused about your results. -0s is set by default for cortex M platforms (see here). That means case 4) should be exactly the issue you want to fix with this PR but now it works? In #5460 the impression was that code size optimization (-Os) lead to problems. However, compiler optimizations seem to fix a bug at your site, I think that is an indicator for something else going wrong :/

[keestux]

@PeterKietzmann I understand that you are confused. But it is exactly as I described.
And I'm also afraid that something else is wrong.

[PeterKietzmann]

@keestux would you please try the following: add to your Makefile

# Change this to 0 show compiler invocation lines by default:
QUIET ?= 0

And compile the application with
(i) CFLAGS_OPT = -O0
(ii) CFLAGS_OPT = -Os
(iii) no extra information about the optimization level

Have a look at the output of dedicated modules, probably paste them here, and search for the optimization level that was actually used.

[keestux]

@PeterKietzmann just so you know, I'm using these make commands all the time

make QUIET=0 BOARD=sodaq-autonomo clean all
make QUIET=0 CFLAGS_OPT=-O0 BOARD=sodaq-autonomo clean all
make QUIET=0 CFLAGS_OPT=-Os BOARD=sodaq-autonomo clean all

Notice that it is not necessary to change the Makefile for a different QUIET setting, nor for CFLAGS_OPT. The ?= is made for that.

I use that make command because I want to see compiler flags, optimization level, include dirs, etc). That is also why I know that -Os is the default. So, options (ii) and (iii) are identical.

Now to come to your request. I take it that you didn't believe me, and you want me to try this again :-). I understand your suspicion. That why I checked it once again (and again).

Guess what? At first, I could get it to fail anymore. WTF? Then I remembered I also enabled ENABLE_DEBUG in auto_init.c. In my setup this seems to be an important factor! Timing?

(i) CFLAGS_OPT=-O0, fails (I checked the presence of -O0 on the compiler command).
(ii) CFLAGS_OPT=-Os, succeeds (checked presence of -Os)
(ii) no CFLAGS_OPT, succeeds (checked presence of -Os)

BTW, after it failed with -O0 I had to physically disconnect the BMP and reconnect to make it work again for -Os.

Believe me, I went and retried this several times to be sure. Conclusion, to make it fail I need:

• CFLAGS_OPT=-O0
• enable ENABLE_DEBUG in auto_init.c
  If you ask me, it is just some weird timing issue of the BMP. (Not documented in the datasheet.)
  A loop count of 1 in this PR seems to be good enough. Even a xtimer_usleep(1) !!

[PeterKietzmann]

@keestux sorry, I didn't mean to take you for a fool. Just the behaviour you describe seems so weird :-). One last thing that came to my mind: Could you try to connect external pull-up resistors to the bus lines? Something about 10kOhm. I once notices that the internal pull-ups on the samr21-xplained pro were not enough. I could imagine the there are some signals/flanks on the bus during initialisation which the sensor interprets as (invalid) command.
How to proceed with this PR? Let's wait for your results with extra resistors. I will check the sensor collection of a colleague to reproduce with the xplained board. However, I ordered one from China. If we had to wait for the chinese-ware, we can merge the PR as is (for now). But generally I'd like to find out the reason for the "bug" or whatever it is.

BTW: Have you seen the green lights for restructuring samr21 and samd21?!

[keestux]

@PeterKietzmann Don't worry. I'm not easily offended :-) I was just trying to be funny.
For me there is no rush. I am continuing with my own branch. It's a weird bug. Most likely a timing issue.

I'm not in a position to do hardware changes. The setup (Autonomo + our TPH board) is used extensively by our (SODAQ) customers. And we use it too. All with Arduino. We haven't seen this bug before.

[aabadie]

For the record, I made several tests with the sensor and it works ok in any cases (with/without ENABLE_DEBUG, with/without the optimisation flag).
I used the samr21-xpro board (which also have a SAMD21 ;) ).

[aabadie]

during my tests, I also noticed that this line is not needed one can directly access the ID register with the next line.

[aabadie]

I created #5679. Can you give it a try ?

[keestux]

For me it does not make a difference. It still fails when building with -O0.

But your changes seems to be correct. With the default -Os it works for me.

BTW. Why didn't you remove

i2c_write_byte(dev->i2c_dev, BMP180_ADDR, (char)BMP180_CALIBRATION_AC1);

It seems useless too.

[keestux]

Sorry, forget about my last BTW (about leaving out the write_byte). It turns out it is necessary.

[keestux]

Again.
The above mentioned i2c_write_byte still does not make sense. Why is it needed? I don't think it is. But if I leave it out I'm getting all zeroes for the calibration.

To solve that I need a delay between the BMP180_REGISTER_ID and the BMP180_CALIBRATION_AC1.

Here is a question to you. Can you try the following?

diff --git a/drivers/bmp180/bmp180.c b/drivers/bmp180/bmp180.c
index e772c3a..ad771cc 100644
--- a/drivers/bmp180/bmp180.c
+++ b/drivers/bmp180/bmp180.c
@@ -73,10 +73,10 @@ int bmp180_init(bmp180_t *dev, i2c_t i2c, uint8_t mode)
         i2c_release(dev->i2c_dev);
         return -1;
     }
+    xtimer_usleep(3);

     char buffer[22] = {0};
     /* Read calibration values, using contiguous register addresses */
-    i2c_write_byte(dev->i2c_dev, BMP180_ADDR, (char)BMP180_CALIBRATION_AC1);
     if (i2c_read_regs(dev->i2c_dev, BMP180_ADDR, BMP180_CALIBRATION_AC1, buffer, 22) < 0) {
         DEBUG("[Error] Cannot read calibration registers.\n");
         i2c_release(dev->i2c_dev);

[aabadie]

@keestux, I tried in #5679 and it indeed works better. Let me know if it's the same for you. We could then merge #5679 as a bug fix.

[PeterKietzmann]

@keestux would you report your results in #5679 please? If the delay fixes your problem (and #5679 does not), I would rather merge a solution with a small delay (plus comment) instead of retrying. If @aabadie is nice, he could implement the fix in #5679, you could test it and give your ACK. This would be a bug fixing PR and we could merge it despite of the current feature freeze :-)

[aabadie]

@keestux, change applied in #5679. Can you give a try and confirm that it works ?

[keestux]

Yes, I confirm that it works.
I've tested my particular error case (ENABLE_DEBUG in auto_init.c and build with -O0). This works.
And the normal case (-Os, without ENABLE_DEBUG in auto_init.c) works too.

[PeterKietzmann]

Closing in favour #5679. Thanks @keestux!