-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
sensor: ms5837: fix compensate parameters for 30BA variant #69069
sensor: ms5837: fix compensate parameters for 30BA variant #69069
Conversation
Hello @EthanAussie, and thank you very much for your first pull request to the Zephyr project! |
c40cc42
to
b034284
Compare
b034284
to
4496563
Compare
please amend your commit so that your author first/last name matches the signed-off by entry. Thanks for the PR! |
4496563
to
499fa39
Compare
Hi Benjamin,
I just updated the Author and Signed-off by to same Ethan Lu.
Hope this time it works, :-)
This is the first time I added some work to Zephyr.
Not familiar with the whole procedure.
Sorry for those inconveniences.
Kind regards,
…On Thu, 29 Feb 2024 at 08:04, Benjamin Cabé ***@***.***> wrote:
please amend your commit so that your author first/last name matches the
signed-off by entry. Thanks for the PR!
—
Reply to this email directly, view it on GitHub
<#69069 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMDDK7QXV3AL6YE3A3M3NTDYV6LVPAVCNFSM6AAAAABDLV27USVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRZHEYTGOBSGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
499fa39
to
3c81de3
Compare
Not sure what is the status of this pull request. |
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.
@EthanAussie sorry that this felt through the cracks -- lgtm
@teburd @MaureenHelm @ubieda maybe you can help review too - thx!
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 for your contribution!
AFAIK - Just a small change-req, otherwise looks good!
drivers/sensor/meas/ms5837/ms5837.c
Outdated
Ti = (3ll * dT * dT) / (1ll << 23); | ||
OFFi = (3ll * temp_sq) / 1ll; | ||
Ti = (3ll * dT * dT) / (1ll << 33); | ||
OFFi = (3ll * temp_sq) / 1ll << 1; |
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.
We need to add parenthesis here, otherwise division will take precedence to the left-shift operation.
OFFi = (3ll * temp_sq) / 1ll << 1; | |
OFFi = (3ll * temp_sq) / (1ll << 1); |
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.
First time to do the pull request and change, etc.
I changed the original branch according to ubieda's suggestion.
It is great catch. Thanks.
The previous parameters seems wrong if we refer to: https://www.te.com/usa-en/product-CAT-BLPS0017.html Signed-off-by: Ethan Lu <[email protected]>
Hi @EthanAussie! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
The previous parameters seems wrong if we refer to:
https://www.te.com/usa-en/product-CAT-BLPS0017.html