-
Notifications
You must be signed in to change notification settings - Fork 585
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 LPS22HB device #2309
Add LPS22HB device #2309
Conversation
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.
Looks great! Thanks for this addition.
2.1 Digital low-pass filter The LPS22HB embeds an additional low-pass filter that can be applied on the pressure readout path when the device is in continuous mode. https://www.st.com/resource/en/application_note/an4833-measuring-pressure-data-from-sts-lps22hb-digital-pressure-sensor-stmicroelectronics.pdf
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.
Looks mostly good. Some suggestions.
src/devices/Lps22hb/README.md
Outdated
- [x] Continus mode (25Hz) | ||
- [x] BDU | ||
- [x] Enable Low-pass filter |
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.
Looks a bit too good to me, when these items are checked but none of them are really available in the API as far as I can tell. What is BDU anyway?
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.
I added more available configuration.
The BDU (Block Data Update) is used to inhibit the update of the output registers between the reading of upper, middle and lower register parts
p20 => https://www.st.com/resource/en/application_note/an4833-measuring-pressure-data-from-sts-lps22hb-digital-pressure-sensor-stmicroelectronics.pdf
src/devices/Lps22hb/Lps22hb.cs
Outdated
public void ResetDevice() | ||
{ | ||
WriteByte(Register.CTRL_REG2, 0b0100); | ||
while (true) |
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 should add a timeout otherwise, this is never ending or a cancellation token with a default value or a pattern with a number of addend and a delay, see
public static class TimeoutHelper |
or even something like this:
public static class TimeoutHelper |
Somehow your choice but you definitely need to addess the infinite loop problem.
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.
I arbitrarily added 100ms max, I've never seen a single loop I think it's enough.
Pipeline looks broken?
Yes, the mac build is currently broken. We're working on it. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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 all good and now build is repaired, we are go to merge!
Adding LPS22HB based on existing LPS25H
https://www.st.com/resource/en/datasheet/lps22hb.pdf
Microsoft Reviewers: Open in CodeFlow