Feature ms5611

[rasmuskleist]

I have had this lying around for a while now. There are some points I am not quite happy with yet. More precisely;

1. The reinterpret_cast<uint16_t *>(&data.calibration) gives a compiler warning
2. There is no guard on the readProm() preventing indices not in range 0..7
3. It may be redundant to have rawTemperatureTouched and rawPressureTouched since it only makes sense to measure both at once

I would like to hear your thoughts?

[rleh]

1. The reinterpret_cast<uint16_t *>(&data.calibration) gives a compiler warning

There is also the following statement in ms5611_impl.hpp:173: modm::fromBigEndian(*reinterpret_cast<uint16_t*>(buffer)).
The compiler is correct, because such a cast might lead to a bus fault on (e.g.) Cortex-M0 if buffer (uint8_t array) is not 16-bit aligned in the memory.
There is a helper to solve this: modm::asUnaligned<>(). It should be possible to just replace reinterpret_cast with modm::asUnaligned.

2. There is no guard on the readProm() preventing indices not in range 0..7

I see (address & 0b111) in the implementation, an input value >7 is harmless. Would be nice to add the allowed range the the documentation (ms5611.hpp:107).

3. It may be redundant to have rawTemperatureTouched and rawPressureTouched since it only makes sense to measure both at once

I don't know the sensor and the possible applications to be able to judge that. I have no problem with how it is currently implemented.

[chris-durand]

The compiler is correct, because such a cast might lead to a bus fault on (e.g.) Cortex-M0 if buffer (uint8_t array) is not 16-bit aligned in the memory.
There is a helper to solve this: modm::asUnaligned<>(). It should be possible to just replace reinterpret_cast with modm::asUnaligned.

The use of modm::asUnaligned is still undefined behaviour in terms of the language, independent of any architecture, even in C in that case. In C++ you may not generally cast a pointer of an object to an unrelated type and then dereference it (there are exceptional cases where it is allowed, e.g. for casts to char or std::byte to access the byte representation of an object). modm::asUnaligned is a no-op for anything that is not a Cortex-M0 and just dereferences the pointer.

Apart from the memory alignment issue there is another reason for that "strict aliasing rule". It allows the compiler to perform an optimization where it assumes objects of different types do not overlap in memory.

In a lot of cases the UB code will emit the expected instructions but a compliant C++ compiler may compile it to anything it wants. The rules are explained here: https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing

For GCC we might get away with it putting a __attribute((__may_alias__)) somewhere in asUnaligned but I honestly don't know really what it does and that would not work for clang and other future compilers.

My recommendation in this case would be to replace the whole expression with a boring bit-shift operation like (buffer[0] << 8) | buffer[1]. That is basically the same as fromBigEndian would do anyway but without reinterpret_cast and UB.

[rasmuskleist]

Thanks a lot for the comments I really appreciate your nice feedback! I will make the necessary changes during Saturday

[rasmuskleist]

Rewriting parts of the driver I have come across some strange behaviour which I have not been able to explain myself. If not either casting the result of readProm() or using modm::fromBigEndian() the contents of the prom is incorrect. I even experienced the program crashing consistently when not casting. Currently I have not experienced any problems with the implementation but I wonder why casting is so critical?

[chris-durand]

If not either casting the result of readProm() or using modm::fromBigEndian() the contents of the prom is incorrect. I even experienced the program crashing consistently when not casting.

The program crashes in case you remove the modm::fromBigEndian() there?

[rasmuskleist]

Yes that is right. Everything works swiftly with the ´modm::fromBigEndian´but removing it causes the program to crash as soon as the protothread is run the first time.

[salkinium]

I'm fine with your PR, unless you're willing to debug our resumable functions (which probably suffer from UB anyways), I would just merge it as is. cc @chris-durand

[rasmuskleist]

Debugging your resumable functions would be an interesting task for the future. However, as for now I am fine with merging.

[salkinium]

Ok, rebase, squash into two commits? Something like [driver] Add MS5611 driver + [example] Add MS5611 example.

[chris-durand]

I'd be fine with merging for now but I somehow have a bad feeling about it. Last time we debugged why adding another overload for operator delete() made the delay run faster we found that our linker script put the code into memory with messed up alignment and it skipped instructions ...
Let's hope it is only related to the resumable functions and we did not trigger another hidden bug.