Skip to content
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

Merged
merged 2 commits into from
Feb 5, 2023

Conversation

lukh
Copy link
Contributor

@lukh lukh commented Jan 13, 2023

Implementation of the MS5837 high-resolution pressure and temperature sensor

heavily based on the MS5711 by Rasmus Kleist Hørlyck Sørensen

@lukh
Copy link
Contributor Author

lukh commented Jan 13, 2023

The MS5611 is very similar, except it's SPI.
internal registers are similar.

I did notice a possible error on the MS5611:
L54 of file ms5611_data_impl.hpp:
int64_t sens = (int64_t(prom.data[1]) << 15) + ((int64_t(prom.data[5]) * int64_t(dT)) >> 8);

the datasheet ask for SENS = SENST1 + TCS * dT = C1 * 2 15 + (C3 * dT ) / 28
which would translate to:
int64_t sens = (int64_t(prom.data[1]) << 15) + ((int64_t(prom.data[3]) * int64_t(dT)) >> 8);

It is implemented like that in the MS5837.

Bests

Copy link
Member

@salkinium salkinium left a 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.

src/modm/driver/pressure/ms5837.lb Outdated Show resolved Hide resolved
src/modm/driver/pressure/ms5837_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/pressure/ms5837_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/pressure/ms5837_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/pressure/ms5837_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/pressure/ms5837_impl.hpp Outdated Show resolved Hide resolved
@lukh
Copy link
Contributor Author

lukh commented Jan 22, 2023

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)

@salkinium salkinium changed the title Feature/ms5837 Add MS5837 high-resolution pressure and temperature sensor driver Jan 25, 2023
@salkinium
Copy link
Member

Did you have time to test your changes @lukh?

@lukh
Copy link
Contributor Author

lukh commented Jan 31, 2023

Yes, pushed :)
Il take care of the rebase on Thursday

Copy link
Member

@salkinium salkinium left a 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.

@chris-durand
Copy link
Member

@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 😄

Copy link
Member

@chris-durand chris-durand left a 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.

Copy link
Member

@salkinium salkinium left a 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.

@lukh
Copy link
Contributor Author

lukh commented Feb 5, 2023

My first squash ! Hooray !

@salkinium
Copy link
Member

(I just fixed the out-of-sync documentation)

@salkinium salkinium merged commit 8179e6b into modm-io:develop Feb 5, 2023
@salkinium salkinium added this to the 2023q1 milestone Feb 5, 2023
@lukh lukh deleted the feature/ms5837 branch February 23, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants