drivers: sensor: ms5611 barometric pressure sensor driver support #67292
Conversation
zephyrbot
added
area: Devicetree Binding
|
Hello @AarC10, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
avisconti,
galak,
MaureenHelm,
teburd,
tristan-google and
yperess
AarC10
force-pushed
the
ms5611_driver
branch
3 times, most recently
from
adbc87f to
bb2269e
Compare
AarC10
force-pushed
the
ms5611_driver
branch
3 times, most recently
from
81a49da to
10f6629
Compare
|
Hi @MaureenHelm Could I please get a review of this PR to gain a better understanding of what to fix and how to improve so this could get merged for my org. Thanks! |
AarC10
changed the title
There was a problem hiding this comment.
It looks incredibly similar to the ms5607. If I understand correctly, they are virtually the same sensor, only MS5607 has half the resolution of the MS5611.
Please look at making MS5607 driver more generic as it doesn't seem like there is justification for duplicating so much code. What's more, the ms5607 driver has support for both I2C and SPI, which is nice.
Yea, I've worked with the MS5607 previously, before switching to the MS5611 for a new board revision. It seems that the only difference would be the coefficients being used during calculation. I agree with your suggestion and I can doing something similar with how the TMP116 driver also supports TMP117 functionality. |
|
@AarC10 thanks for the update. Does it make sense to leave this PR open or do you want to create another one for the update to the MS5607 driver? It's fine either way. |
I think it makes more sense to just leave this PR open. I will try to make changes, test and squash soon when I can find the time (hopefully this week). Thanks! Follow up question actually. Should we rename the ms5607 driver to ms56xx if this is to be a shared driver? |
Excellent!
Yep! |
|
Alright merged the drivers into one now. Sorry for the delay. Most of my time was spent figuring out macro fun for initializing those coefficients. Pretty much everything is renamed, added a field to the config for storing coefficients and replacing the hardcoded values in the compensate function to use those stored coefficients instead. Also made a comparison table of the values. Tested both drivers as well and the results are below. MS5607 on top and MS5611 on the bottom
|
AarC10
force-pushed
the
ms5611_driver
branch
2 times, most recently
from
f515bd3 to
cc469a2
Compare
| compatible: "meas,ms5607" | ||
| compatible: "meas,ms56xx" |
There was a problem hiding this comment.
as you are renaming an existing compatible, it would require an entry in the migration guide to tell people what they need to change in their code that will now break.
Another option would be to keep yaml files for the "old" ms5607 and have them include/inherit the new generic binding, to allow for backward compatibility
drivers/sensor/ms56xx/Kconfig
Outdated
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| menuconfig MS56XX | ||
| bool "MS56XX pressure and temperature sensor" |
There was a problem hiding this comment.
not sure if you want to be more consistent: sometimes the driver is referred as being for "MS5607 and MS5611 ... " (ex. in dts bindings), and sometimes as "MS56xx". Don't have a strong preference but it would be good to be consistent.
| default: 7 | ||
| description: | | ||
| Specifies which chip is used to determine calculation coefficients |
There was a problem hiding this comment.
default value needs to be explained/justified
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#rules-for-default-values
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Sounds good! Sorry. Was planning on getting to it, just a lot of stuff going on these past few months. Will handle ASAP |
|
Hey @kartben Made changes to those suggestions. Also super sorry, but I wasn't thinking when I hit sync fork and it automatically closed this PR. Had a new development machine setup too and had to reclone everything, so couldn't really force push. Don't know if its ok that I had to open a new PR, or if I should just mess with my git a little more to try and get it here if possible. Also, can't reply to your conversations in review mode, because everything got blasted :/ Anyways, I added to the migration guide. I think we should be explicit that it isn't just MS5607 only that is supported. I remember when I first started using Zephyr there was some sensor I was using that didn't make it explicit, so that's sort of why I want to go in that direction if that was ok. Also updated the KConfig as requested to be a little more consistent. And removed making it a default value and instead making it required to choose between either one. My original intent was to sort of backwards support by not needing to modify devicetree, but I guess that doesn't make sense if the driver itself is renamed anyways. Please let me know if there is anything else you need or anything that might help! Thank you for your time. |
Following up on #48758 since it was closed more than a year ago. Double checked the driver continues to work and added comments for CRC4 calculation, as well as replace <zephyr/zephyr.h> with <zephyr/kernel.h>
Signed-off-by: Aaron Chan [email protected]