-
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
drivers: sensors: add ST lis2dux12 accelerometer support #65259
drivers: sensors: add ST lis2dux12 accelerometer support #65259
Conversation
Hello @kdunn926, and thank you very much for your first pull request to the Zephyr project! |
d07595b
to
fd64e81
Compare
fd64e81
to
4bb4096
Compare
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.
consider using year 2023, and probably just use .clang-format
to format all these new files
4bb4096
to
c8c0ed8
Compare
c8c0ed8
to
e243de1
Compare
e243de1
to
55d5e05
Compare
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.
It's been a long way but we are finally there...
dcf6590
to
6e2fbc4
Compare
Please rebase |
6e2fbc4
to
8d69de7
Compare
Ok I just did. Sorry for the delay. |
@ycsin - Have all the changes you requested been addressed? |
@fabiobaltieri if/when you have time, can you possibly take a look at this PR? Thanks |
drivers/sensor/lis2dux12/lis2dux12.c
Outdated
|
||
static int lis2dux12_set_range(const struct device *dev, uint8_t range) | ||
{ | ||
int err = 0; |
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.
drop the initializer, not needed
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.
Alright
drivers/sensor/lis2dux12/lis2dux12.c
Outdated
|
||
err = lis2dux12_mode_set(ctx, &val); | ||
|
||
if (!err) { |
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.
invert the logic here:
if (err) {
return err;
}
and just return 0;
at the end of the function, keep the normal execution flow inline, easier to follow the code intended path
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.
That makes sense. I made the change.
drivers/sensor/lis2dux12/lis2dux12.c
Outdated
|
||
if (!err) { | ||
switch (range) { | ||
default: |
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.
sure you don't want to error out here? silently falling back to default does not strike me as a great idea, maybe add a LOG_ERR
and return -EINVAL
?
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.
Good point, I changed it to be like this:
default:
LOG_ERR("range [%d] not supported.", range);
return -EINVAL;
drivers/sensor/lis2dux12/lis2dux12.c
Outdated
#define FOREACH_ODR_ENUM(ODR_VAL) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_OFF, 0.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_1Hz_ULP, 1.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_3Hz_ULP, 3.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_25Hz_ULP, 25.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_6Hz, 6.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_12Hz5, 12.50f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_25Hz, 25.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_50Hz, 50.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_100Hz, 100.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_200Hz, 200.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_400Hz, 400.0f) \ | ||
ODR_VAL(LIS2DUX12_DT_ODR_800Hz, 800.0f) |
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.
nit: you can probably pull those \
a bit behind, no need to make lines unnecessarily wide
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.
Alright, I shifted these over quite a bit in the latest commit.
8d69de7
to
32fbe01
Compare
Adds support for the STMicroelectronics LIS2DUX12 3-axis accelerometer. Signed-off-by: Kyle Dunn <[email protected]>
32fbe01
to
50bef4a
Compare
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 good
Hi @kdunn926! 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 driver was based off the existing lis2dw12 implementation.
This was physically tested via SPI at 1MHz using a nrf52840 development kit. Both the Zephyr
sensor
and directspi_write
APIs were successfully used. Interrupts and runtime configuration changes (e.g. ODR, range) were not tested.sensor
API sample:spi_write
API sample: