-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature ms5611 #851
Feature ms5611 #851
Conversation
There is also the following statement in ms5611_impl.hpp:173:
I see
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. |
The use of 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 My recommendation in this case would be to replace the whole expression with a boring bit-shift operation like |
Thanks a lot for the comments I really appreciate your nice feedback! I will make the necessary changes during Saturday |
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 |
The program crashes in case you remove the |
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. |
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 |
Debugging your resumable functions would be an interesting task for the future. However, as for now I am fine with merging. |
Ok, rebase, squash into two commits? Something like [driver] Add MS5611 driver + [example] Add MS5611 example. |
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 |
b06c1dd
to
d3c8a8a
Compare
Co-authored-by: Niklas Hauser <niklas.hauser@gmail.com>
d3c8a8a
to
ab9bcee
Compare
I have had this lying around for a while now. There are some points I am not quite happy with yet. More precisely;
reinterpret_cast<uint16_t *>(&data.calibration)
gives a compiler warningreadProm()
preventing indices not in range 0..7rawTemperatureTouched
andrawPressureTouched
since it only makes sense to measure both at onceI would like to hear your thoughts?