drivers/bmp180: fix potential use of uninitialized variable

[aabadie]

Contribution description

This PR is small cleanup in the bmp180 driver implementation. It fixes the use of a potentially uninitialized value in the driver. It was raised by scan-build.

Testing procedure

• Run

  TOOLCHAIN=llvm make -C tests/driver_bmp180/ scan-build
  

  on master, you get this warning: warning: 2nd function call argument is an uninitialized value _compute_b5(dev, ut, &b5);. With this PR, it is fixed.

• One could also verify that the driver still works

Issues/PRs references

Tick one item in #11852

[maribu]

OK. I looked at the context and think it should be fixed differently. The first access to ut is _read_ut(), which will initialize ut unless an communication error on the I2C bus occurs. As the return value of _read_ut() is ignored, the code will continue to compute the temperature based on the random content of ut, if the read failed.

The proper solution would be do error handling in case the I2C communication fails.

[gschorcht]

It is quite interesting that ut is claimed to be used uninitialized while b5 ist not. Both variables are used as reference parameters that are filled by a function. The only difference is that ut is used as parameter for a subsequent function call while b5 is used in a calculation.

There are some extra costs of 16 bytes on ATmega and 24 bytes on Cortex-M platforms to fix this warning. So the question is, whether we could work around this with defines.

BTW, when executing the test command, I get dozens of

/data/riotbuild/cpu/sam0_common/include/periph_cpu_common.h:390:46: warning: The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'unsigned long'

Have you seen them too?

[maribu]

It is quite interesting that ut is claimed to be used uninitialized while b5 ist not. Both variables are used as reference parameters that are filled by a function. The only difference is that ut is used as parameter for a subsequent function call while b5 is used in a calculation.

The result of the analyzer is 100% correct: _compute_b5() will always write to b5, while _read_ut() will only write to ut if the I2C transfer succeeded.

[aabadie]

Have you seen them too?

Yes but this is another problem.

I think the proposed fix of this PR is ok for now. Looking at the code, I agree we should do a proper error handling: public functions should return an error code instead of the measurement value. This one should be passed as output parameter. That would be better IMHO but would also be an API change.

[gschorcht]

Ups, we have taken look in parallel on that.

The proper solution would be do error handling in case the I2C communication fails.

This is a very old topic. 95% of our drivers (or even more) do not check the return values ​​of i2c_ * functions. The understandable justification was always that checking the return value would increase the code size. It's always a matter of tradeoff between fault tolerance and code size.

[aabadie]

Maybe, I can also remove the I2C checks in the read_ut/read_up functions. What do you think ?

[gschorcht]

Looking at the code, I agree we should do a proper error handling: public functions should return an error code instead of the measurement value.

I totally agree, I am a friend of fault tolerance. But this was also discussed already a number of times.

[aabadie]

I totally agree, I am a friend of fault tolerance

Ok, let's go with the API change then :)

[gschorcht]

Maybe, I can also remove the I2C checks in the read_ut/read_up functions. What do you think ?

What is better, having an error handling or having wrong values? I'm not sure.

The error handling in _read_ut and _read_up isn't consistent anyway. The return value from i2c_write_bytes isn't checked while the return value from i2c_read_regs is.

What would be the effect, if error handling is removed completely? The user would get sensor values that make no sense. With a consequent error handling, he would get 0 for sensor data that would at least indicate for pressure that it can't be.

[maribu]

The understandable justification was always that checking the return value would increase the code size. It's always a matter of tradeoff between fault tolerance and code size.

Sure? My understanding was that this was a bit more involved:

• Things that can only be cause by bugs in the code should be handled by assert(), so that no ROM size is being paid in production code
• Things that either consistently fail (like spi_acquire() with the very same arguments) should be checked on driver initialization, but not again afterwards
• Things that can fail and are outside of the control of the code (like communication errors) should be handled

But I cannot remember where I got this.

[gschorcht]

The understandable justification was always that checking the return value would increase the code size. It's always a matter of tradeoff between fault tolerance and code size.

Sure? My understanding was that this was a bit more involved:

• Things that can only be cause by bugs in the code should be handled by assert(), so that no ROM size is being paid in production code
• Things that either consistently fail (like spi_acquire() with the very same arguments) should be checked on driver initialization, but not again afterwards
• Things that can fail and are outside of the control of the code (like communication errors) should be handled

But I cannot remember where I got this.

Probably, you got this from the discussion in issue #10673

[gschorcht]

Changing the API would be a long term process and is not part of this PR. IMO we could go with this PR and accept the small increase of the code size if can make scan-build happy.

I think it is not necessary to test since it doesn't change anything critical. But if you want, I can do a short test on ESP8266, I have the BMP180 at hand.

[aabadie]

Changing the API would be a long term process and is not part of this PR

Ah, too late, sorry. But I can revert and provide the refactoring in another PR.

[aabadie]

BTW, the new driver version increases the code size by 128 bytes.

[gschorcht]

Changing the API would be a long term process and is not part of this PR

Ah, too late, sorry. But I can revert and provide the refactoring in another PR.

Oh, I'm really sorry. I hate to put work into something that is not used in the end.

@maribu What do you think?

Two concerns of mine are

1. Changing the API always requires changes in existing applications at RIOT developers side and should be decided very carefully.
2. I'm not sure whether the design rules in the Guide for writing a device driver in RIOT sections Return values and Sensors are still met. Maybe the guide needs a revision or at least some clarification.

On the other hand, similar sensors like SHT1x, SH3x already implement an API in that way with the only difference that they return temperature and humidity in one call. This would be a further possibility.

If we decide to change it now, we should also have a look to bmx280 which is a very similar sensor and should have a similar API.

[maribu]

First: I'm OK with both the suggested alternatives (ignoring the return value of the I2C transfer altogether or adding error handling).

However, I'm personally a huge fan of carefully handling things that could go wrong and are not code related. I spend enormous amounts of time trying to debug code where the issue was a broken wire. Error handling for communication errors would prevent this.

I think that adding an int bmp180_read(const bmp180_t *dev, int16_t *temperature, uint32_t *pressure) (that allows to only read one by setting either temperature or pressure to NULL) could be a way to add the error handling. The current functions could be added as static inline wrappers that return a temperature of e.g. -2742 (= -274.2°C = -1K, so physically impossible) and a pressure of -1 on error. In addition the current function could be marked as deprecated to motivate existing users to migrate to the new function, which makes error handling easier than testing for -2742 (or whatever special temperature is considered as error). However, no user would need to move to the new API if they don't want to.

[aabadie]

@gschorcht, I reverted to the initial version of this PR. I backed up the refactoring with error checking in a separate branch, so it's not lost :)

[gschorcht]

@gschorcht, I reverted to the initial version of this PR. I backed up the refactoring with error checking in a separate branch, so it's not lost :)

OK, let's continue with the little change first. Should we open a tracking issue so we do not forget the pending API change?

[gschorcht]

Small change that makes "scan-build" happy at a very low price of extra bytes in code size.