-
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
Add MS5837 high-resolution pressure and temperature sensor driver #942
Conversation
The MS5611 is very similar, except it's SPI. I did notice a possible error on the MS5611: the datasheet ask for SENS = SENST1 + TCS * dT = C1 * 2 15 + (C3 * dT ) / 28 It is implemented like that in the MS5837. Bests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot judge the functional correctness of the algorithms, but the rest is fine.
I would remove the logger and instead return error codes. Depending on the printf implementation can be annoying on very small µCs.
If you still want to report a failure, you can use modm_assert_continue_ignore_debug(false, "ms5837.read", "readProm failed!", this)
to report an error in a more lightweight manner.
I did implement the modifications, I don't have the HW at hand for the moment, so, I am pushing it whenever it is tested ;) (probably next week) |
Did you have time to test your changes @lukh? |
Yes, pushed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! (I only added a commit to convert the spaces to tabs). Please squash and rebase the commits on the latest develop, when you have the time.
@lukh I have added a commit with some clean-up, especially of the CRC code copied from the datasheet. Just squash it into your changes. I hope I didn't break anything 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukh Could you add an example for the driver? Because it's all templates the CI can't otherwise check if the driver really compiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, awesome! Please rebase and squash into two commits for adding the driver and example each.
My first squash ! Hooray ! |
eb166cc
to
8179e6b
Compare
(I just fixed the out-of-sync documentation) |
Implementation of the MS5837 high-resolution pressure and temperature sensor
heavily based on the MS5711 by Rasmus Kleist Hørlyck Sørensen