drivers: Lsm9ds1 driver #73141

miggazElquez · 2024-05-22T10:04:45Z

This patch add a driver for the st lsm9ds1 sensor (https://www.st.com/en/mems-and-sensors/lsm9ds1.html).

It also fix a bug in the accel_polling sample.

github-actions · 2024-05-22T10:05:24Z

Hello @miggazElquez, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

decsny · 2024-05-23T18:12:37Z

dts/bindings/sensor/st,lsm9ds1_mag.yaml

missing justification

@miggazElquez see https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#rules-for-default-values

Thanks, I added the justification

jeppenodgaard

Almost got through the lsm9ds1 part. Can the lsm9ds1_mac part be split into another PR?

jeppenodgaard · 2024-05-29T20:18:42Z

drivers/sensor/st/lsm9ds1/lsm9ds1.c

Suggested change

#if defined(CONFIG_LSM9DS1_ENABLE_TEMP)

#if IS_ENABLED(CONFIG_LSM9DS1_ENABLE_TEMP)

drivers/sensor/st/lsm9ds1/lsm9ds1.c

jeppenodgaard · 2024-05-29T20:28:20Z

drivers/sensor/st/lsm9ds1/lsm9ds1.c

Should the 0 be 4?

The raw odr value (index of the array) can only be 0,1 or 3. I added a comment to explain it, but maybe I should put another value than 0 ?

jeppenodgaard · 2024-05-29T20:40:01Z

drivers/sensor/st/lsm9ds1/lsm9ds1.c

Probably LOG_ERR since accel and gyro probably have non-matching values.

True, the sensor will still work but the code will break.

jeppenodgaard · 2024-05-29T20:43:44Z

drivers/sensor/st/lsm9ds1/lsm9ds1.c

What happens if 15 is chosen here when accel seems to want 10? Would it make sense to check for equal values before changing?

When the gyroscope is on, the frequencies of the gyroscope are the one that matters.
So if the gyroscope is off, accel can be at 10 Hz, if the gyroscope is on, accel will be at 15 Hz (or another frequency on the gyroscope list), and can't be at 10 Hz as long as the gyroscope is on.
In fact, if the gyroscope is on, the value of the odr register of the accel is ignored, but the documentation and the HAL wants to have the same value in both registers, so I set it to the right value.

jeppenodgaard · 2024-05-29T20:44:18Z

drivers/sensor/st/lsm9ds1/lsm9ds1.c

0x7 probably needs a define.

drivers/sensor/st/lsm9ds1/lsm9ds1.c

miggazElquez · 2024-05-30T14:58:38Z

Almost got through the lsm9ds1 part. Can the lsm9ds1_mac part be split into another PR?

Done, the mag part is now on #73530

tests/drivers/build_all/sensor/i2c.dtsi

jeppenodgaard

The header could benefit from clang-format and probably also the source.

jeppenodgaard · 2024-06-01T18:34:12Z

samples/sensor/accel_polling/src/main.c

Would it make sense to return if device return -ENOTSUP?

In a lot of drivers the "attr_get" function is not implemented, so if we return here the sample will not work for these sensors. My idea was that in the case we don't get an answer, we just assume that the frequency isn't correct, and try to set one. If the frequency was set, and we change it to another frequency than the default one, it's not really a problem, as we just need to fetch samples every second.

miggazElquez · 2024-06-10T11:56:29Z

The header could benefit from clang-format and probably also the source.

I didn't know this tool, thanks for the recommendation.

MaureenHelm

Need to rebase and resolve conflicts

MaureenHelm · 2024-06-11T21:42:31Z

drivers/sensor/st/lsm9ds1/lsm9ds1.c

Can we sleep instead?

It seems like it work. I did a busy wait because I saw that other similar drivers did the same, and I didn't checked.

drivers/sensor/st/lsm9ds1/lsm9ds1.c

This commit adds a description for the lsm9ds1 sensor, with a .h file containing all configuration options. Signed-off-by: Miguel Gazquez <[email protected]>

avisconti

LGTM, but maybe it's better to add few comments in the commit message:

no trigger support
only I2C
no mag support

drivers/sensor/st/lsm9ds1/Kconfig

drivers/sensor/st/lsm9ds1/lsm9ds1.c

avisconti

Nice job.

miggazElquez · 2024-06-24T07:14:10Z

@MaureenHelm does this look good to you now ?

The LSM9DS1 is a system-in-package featuring a 3D digital linear acceleration sensor, a 3D digital angular rate sensor and a 3D digital magnetic sensor. This driver implements only the linear acceleration sensor and the angular rate sensor, on the I2C bus. This driver is without trigger support. The driver is based on the stmemsc HAL. link: https://www.st.com/resource/en/datasheet/lsm9ds1.pdf Signed-off-by: Miguel Gazquez <[email protected]>

Describe the lsm9ds1 sensor available in the Arduino Nano 33 BLE through I2C. Set the accel0 alias. Signed-off-by: Miguel Gazquez <[email protected]>

The accel_polling sample uses various sensor, but doesn't set a sampling rate. But some sensors (like st,lsm6dso) have a default sampling frequency of 0. So, depending on the sensor, the sample may not always work. There are two ways to fix this: either all drivers must set a valid sampling rate, or the sample shall at least try to set a value if there is none. We propose here the second approach, wich should allow the sample to work on more sensors out of the box. Signed-off-by: Miguel Gazquez <[email protected]>

miggazElquez · 2024-06-27T07:33:16Z

I just fixed some typos I saw in comments, the driver didn't change otherwise.

miggazElquez requested a review from avisconti as a code owner May 22, 2024 10:04

zephyrbot added area: Samples Samples area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors labels May 22, 2024

zephyrbot requested review from MaureenHelm, decsny, galak, kartben, nashif, teburd, tristan-google, ubieda and yperess May 22, 2024 10:05

zephyrbot assigned MaureenHelm May 22, 2024

decsny reviewed May 23, 2024

View reviewed changes

miggazElquez force-pushed the lsm9ds1_driver branch 3 times, most recently from fc337be to 1601cfc Compare May 29, 2024 08:08

jeppenodgaard reviewed May 29, 2024

View reviewed changes

miggazElquez force-pushed the lsm9ds1_driver branch 2 times, most recently from 3fe7f13 to c1c17a6 Compare May 30, 2024 14:28

miggazElquez mentioned this pull request May 30, 2024

drivers: Lsm9ds1 mag driver #73530

Merged

miggazElquez changed the title ~~Lsm9ds1 driver~~ drivers: Lsm9ds1 driver May 31, 2024

MaureenHelm requested changes May 31, 2024

View reviewed changes

tests/drivers/build_all/sensor/i2c.dtsi Outdated Show resolved Hide resolved

MaureenHelm assigned MaureenHelm and avisconti and unassigned MaureenHelm May 31, 2024

jeppenodgaard reviewed Jun 1, 2024

View reviewed changes

jeppenodgaard mentioned this pull request Jun 4, 2024

Contributor self nominate jeppenodgaard #73716

Closed

miggazElquez force-pushed the lsm9ds1_driver branch 2 times, most recently from 52c8808 to 71ffa5f Compare June 10, 2024 12:25

MaureenHelm reviewed Jun 11, 2024

View reviewed changes

miggazElquez force-pushed the lsm9ds1_driver branch from 71ffa5f to 6f15e19 Compare June 12, 2024 11:52

jeppenodgaard approved these changes Jun 13, 2024

View reviewed changes

dts: bindings: add DT binding for lsm9ds1

1f7c33f

This commit adds a description for the lsm9ds1 sensor, with a .h file containing all configuration options. Signed-off-by: Miguel Gazquez <[email protected]>

miggazElquez force-pushed the lsm9ds1_driver branch 2 times, most recently from 76a2890 to 3041044 Compare June 17, 2024 11:53

avisconti requested changes Jun 19, 2024

View reviewed changes

drivers/sensor/st/lsm9ds1/Kconfig Outdated Show resolved Hide resolved

drivers/sensor/st/lsm9ds1/lsm9ds1.c Outdated Show resolved Hide resolved

drivers/sensor/st/lsm9ds1/lsm9ds1.c Outdated Show resolved Hide resolved

drivers/sensor/st/lsm9ds1/lsm9ds1.c Outdated Show resolved Hide resolved

miggazElquez force-pushed the lsm9ds1_driver branch from 3041044 to d71bab4 Compare June 20, 2024 09:58

avisconti previously approved these changes Jun 20, 2024

View reviewed changes

miggazElquez added 3 commits June 27, 2024 09:31

boards: arduino_nano_33_ble: add lsm9ds1 sensor to device tree

7daa992

Describe the lsm9ds1 sensor available in the Arduino Nano 33 BLE through I2C. Set the accel0 alias. Signed-off-by: Miguel Gazquez <[email protected]>

miggazElquez dismissed avisconti’s stale review via 5d50211 June 27, 2024 07:32

miggazElquez force-pushed the lsm9ds1_driver branch from d71bab4 to 5d50211 Compare June 27, 2024 07:32

avisconti approved these changes Jun 27, 2024

View reviewed changes

jeppenodgaard approved these changes Jul 2, 2024

View reviewed changes

MaureenHelm approved these changes Jul 9, 2024

View reviewed changes

MaureenHelm added this to the v4.0.0 milestone Jul 9, 2024

aescolar merged commit f6f0386 into zephyrproject-rtos:main Jul 29, 2024

	#if defined(CONFIG_LSM9DS1_ENABLE_TEMP)
	#if IS_ENABLED(CONFIG_LSM9DS1_ENABLE_TEMP)

drivers: Lsm9ds1 driver #73141

drivers: Lsm9ds1 driver #73141

Uh oh!

Conversation

miggazElquez commented May 22, 2024

Uh oh!

github-actions bot commented May 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartben May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeppenodgaard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miggazElquez commented May 30, 2024

Uh oh!

Uh oh!

jeppenodgaard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miggazElquez commented Jun 10, 2024

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avisconti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avisconti left a comment

Choose a reason for hiding this comment

Uh oh!

miggazElquez commented Jun 24, 2024

Uh oh!

miggazElquez commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kartben May 23, 2024 •

edited

Loading