drivers: add lsm6dsl accelerometer/gyroscope device driver by vincent-d · Pull Request #6835 · RIOT-OS/RIOT

vincent-d · 2017-03-31T14:10:26Z

This PR add a driver for ST LSM6DSL accelerometer/gyroscope + saul support.
It's available on the x-nucleo-iks01a2 shield.

It's a basic driver which just read accelerometer and gyroscope data.

aabadie

looks good, will test next week (I have the hardware). a few style comments though.

aabadie · 2017-03-31T14:19:10Z

drivers/lsm6dsl/Makefile

@@ -0,0 +1,2 @@
+


empty line, can be removed

aabadie · 2017-03-31T14:19:53Z

drivers/lsm6dsl/include/lsm6dsl-internal.h

+ */
+
+/**
+ * @addtogroup  drivers_lsm6dsl


should it be @InGroup ?

Looks like @InGroup is used everywhere, indeed.

aabadie · 2017-03-31T14:21:34Z

drivers/lsm6dsl/lsm6dsl.c

+                        LSM6DSL_REG_OUTX_H_XL, &tmp);
+    data->x |= tmp << 8;
+    res += i2c_read_reg(dev->params.i2c, dev->params.addr,
+                       LSM6DSL_REG_OUTY_L_XL, &tmp);


misaligned ;)

aabadie · 2017-03-31T14:22:06Z

drivers/lsm6dsl/lsm6dsl.c

+                        LSM6DSL_REG_OUTX_H_G, &tmp);
+    data->x |= tmp << 8;
+    res += i2c_read_reg(dev->params.i2c, dev->params.addr,
+                       LSM6DSL_REG_OUTY_L_G, &tmp);


align, and in other places

vincent-d · 2017-03-31T15:23:43Z

@aabadie comments addressed

aabadie · 2017-03-31T15:30:16Z

Just had a look at the producs on ST website. There are several versions of this sensor: LSM6DS0 in X-NUCLEO-IKS01A1 and LSM6DSL in X-NUCLEO-IKS01A2
There's also a similar PR #3263 opened for LSM6DS3.

They seem to be very similar, do you think it would make sense to make this driver compatible with both ?

aabadie · 2017-03-31T15:32:32Z

drivers/include/lsm6dsl.h

+ */
+
+/**
+ * @defgroup    drivers_lsm6dsl LSM36DSL 3D accelerometer/gyroscope


LSM36DSL => LSM6DSL

and in other places below

aabadie · 2017-03-31T15:32:54Z

drivers/lsm6dsl/include/lsm6dsl-internal.h

+ *
+ * @{
+ * @file
+ * @brief       Internal configuration for LSM36DSL devices


here as well

aabadie · 2017-03-31T15:33:18Z

drivers/lsm6dsl/include/lsm6dsl_params.h

+ *
+ * @{
+ * @file
+ * @brief       Default configuration for LSM36DSL devices


vincent-d · 2017-03-31T15:35:43Z

Copy/paste error fixed ;)

vincent-d · 2017-03-31T15:38:04Z

As usual, there is no unique datasheet, so we need to compare to be sure the interface is similar. But if it's the case, I would say it make sense to factorize the code and have a unique driver.

aabadie

a few nitpicks + some remaining typo. I'll test this driver on LSM6DS0

aabadie · 2017-04-05T15:37:31Z

drivers/include/lsm6dsl.h

+#endif
+
+#endif /* LSM6DSL_H */
+


extra line could be removed

aabadie · 2017-04-05T15:37:44Z

drivers/lsm6dsl/include/lsm6dsl-internal.h

+ * @name LSM6DSL registers
+ * @{
+ */
+#define LSM6DSL_REG_FUNC_CFG_ACCESS  (0x01)


aabadie · 2017-04-05T15:38:20Z

drivers/lsm6dsl/include/lsm6dsl-internal.h

+/** @} */
+
+/** WHO_AM_I value */
+#define LSM6DSL_WHO_AM_I  (0b01101010)


Can you align with the other values above, here and below

aabadie · 2017-04-05T15:38:32Z

drivers/lsm6dsl/include/lsm6dsl-internal.h

+#endif
+
+#endif /* LSM6DS_LINTERNAL_H */
+


aabadie · 2017-04-05T15:38:48Z

drivers/lsm6dsl/include/lsm6dsl_params.h

+#endif
+
+#endif /* LSM6DSL_PARAMS_H */
+


aabadie · 2017-04-05T15:42:39Z

sys/auto_init/saul/auto_init_lsm6dsl.c

+ * @{
+ *
+ * @file
+ * @brief       Auto initialization of LSM6DSL pressure sensors


s/pressure/accelerometer../

aabadie · 2017-04-05T15:43:46Z

sys/auto_init/saul/auto_init_lsm6dsl.c

+void auto_init_lsm6dsl(void)
+{
+    for (unsigned int i = 0; i < LSM6DSL_NUM; i++) {
+


extra space that can be removed

aabadie · 2017-04-05T15:44:08Z

sys/auto_init/saul/auto_init_lsm6dsl.c

+
+        int res = lsm6dsl_init(&lsm6dsl_devs[i], &lsm6dsl_params[i]);
+        if (res < 0) {
+            LOG_ERROR("[auto_init_saul] error initializing lsm6dsl #%u\n", i);


I would prefer using a continue after and no else

aabadie · 2017-04-05T15:53:54Z

drivers/lsm6dsl/include/lsm6dsl-internal.h

@@ -0,0 +1,158 @@
+/*


Can you rename this file to lsm6dsl_internals.h ?

Done, but a lot of other drivers have a dash in the xxx-internals.h file name.

vincent-d · 2017-04-05T16:09:10Z

@aabadie comments addressed

aabadie · 2017-04-05T16:10:16Z

Tested on a LSM6DS0 with small adaptations. The gyroscope and accelerometer seems to work but I don't have the temperature. Looking at the datasheet the registers addresses are quite different even if some have similar names... So it might not be that simple to unify the drivers.

aabadie · 2017-04-05T16:13:25Z

drivers/lsm6dsl/lsm6dsl.c

+                        ((dev->params.acc_odr << LSM6DSL_CTRL_ODR_SHIFT) |
+                         (dev->params.acc_fs << LSM6DSL_CTRL_FS_SHIFT)));
+    /* Set gyro odr / full scale */
+    res = i2c_write_reg(dev->params.i2c, dev->params.addr, LSM6DSL_REG_CTRL2_G,


this line makes static-test fails on my laptop:
drivers/lsm6dsl/lsm6dsl.c:55: style (redundantAssignment): Variable 'res' is reassigned a value before the old one has been used

aabadie · 2017-04-05T16:14:35Z

drivers/lsm6dsl/lsm6dsl.c

+    res = i2c_write_reg(dev->params.i2c, dev->params.addr, LSM6DSL_REG_FIFO_CTRL5,
+                        (fifo_odr << LSM6DSL_FIFO_CTRL5_FIFO_ODR_SHIFT) |
+                        LSM6DSL_FIFO_CTRL5_CONTINUOUS_MODE);
+    res = i2c_write_reg(dev->params.i2c, dev->params.addr, LSM6DSL_REG_FIFO_CTRL3,


same with this one because of another reason:
drivers/lsm6dsl/lsm6dsl.c:64: style (unreadVariable): Variable 'res' is assigned a value that is never used

The make static-test command returns interesting results :)

aabadie · 2017-04-05T16:16:19Z

drivers/lsm6dsl/lsm6dsl.c

+    xtimer_usleep(5000);
+
+    i2c_read_reg(dev->params.i2c, dev->params.addr, LSM6DSL_REG_WHO_AM_I, &res);
+    if (res != LSM6DSL_WHO_AM_I) {


This is only place where you actually use the res value. I would drop res = in the other res = i2c_write_reg... lines

vincent-d · 2017-04-06T07:42:29Z

Improved init function

aabadie · 2017-04-06T08:06:08Z

drivers/lsm6dsl/lsm6dsl.c

+                        ((dev->params.acc_odr << LSM6DSL_CTRL_ODR_SHIFT) |
+                         (dev->params.acc_fs << LSM6DSL_CTRL_FS_SHIFT)));
+    /* Set gyro odr / full scale */
+    res += i2c_write_reg(dev->params.i2c, dev->params.addr, LSM6DSL_REG_CTRL2_G,


even better

aabadie

ACK. Thanks @vincent-d !

aabadie · 2017-04-06T08:07:52Z

Please squash

aabadie · 2017-04-06T08:29:44Z

Just curious, do you plan to write drivers for other sensors on this shield ? (HTS221, LPS22HB, LSM303AGR)

vincent-d · 2017-04-06T09:00:57Z

Squashed

vincent-d · 2017-04-06T09:02:41Z

Just curious, do you plan to write drivers for other sensors on this shield ? (HTS221, LPS22HB, LSM303AGR)

No, but I can help testing and reviewing if you plan it ;)

aabadie · 2017-04-06T09:17:46Z

HTS221 is interesting, I'll let you know if one day I start having a look at this one. Otherwise there's already #4257 opened that should be quite similar to LPS22HB.

Let's go when Murdock is green.

vincent-d added the Area: drivers Area: Device drivers label Mar 31, 2017

aabadie self-assigned this Mar 31, 2017

aabadie added this to the Release 2017.04 milestone Mar 31, 2017

aabadie added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Mar 31, 2017

aabadie requested changes Mar 31, 2017

View reviewed changes

aabadie reviewed Mar 31, 2017

View reviewed changes

aabadie requested changes Mar 31, 2017

View reviewed changes

aabadie requested changes Apr 5, 2017

View reviewed changes

aabadie reviewed Apr 5, 2017

View reviewed changes

aabadie reviewed Apr 6, 2017

View reviewed changes

aabadie approved these changes Apr 6, 2017

View reviewed changes

Vincent Dupont added 2 commits April 6, 2017 10:58

drivers: add lsm6dsl imu driver

73d8149

tests/driver_lsm6dsl: add lsm6dsl driver test app

f5adafd

vincent-d force-pushed the pr/lsm6dsl branch from 8f1f49d to f5adafd Compare April 6, 2017 09:00

vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 6, 2017

aabadie merged commit c65e57b into RIOT-OS:master Apr 6, 2017

vincent-d deleted the pr/lsm6dsl branch April 7, 2017 07:32

aabadie mentioned this pull request Jun 4, 2017

Usage and interpretation of I2C addresses #7133

Closed

smlng mentioned this pull request Jun 19, 2017

LSM6DS3 driver implementation for LimiFrog V1 #3263

Closed

smlng mentioned this pull request Jun 30, 2017

Support for ST X-NUCLEO IKS01A2 #7282

Closed

		#endif

		#endif /* LSM6DSL_H */

		#endif

		#endif /* LSM6DS_LINTERNAL_H */

		#endif

		#endif /* LSM6DSL_PARAMS_H */

Conversation

vincent-d commented Mar 31, 2017

Uh oh!

aabadie 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!

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!

vincent-d commented Mar 31, 2017

Uh oh!

aabadie commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vincent-d commented Mar 31, 2017

Uh oh!

vincent-d commented Mar 31, 2017

Uh oh!

aabadie 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!

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!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Apr 5, 2017

Uh oh!

aabadie commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 5, 2017 • 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!

aabadie Apr 5, 2017 • 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!

vincent-d commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Apr 6, 2017

Uh oh!

aabadie commented Apr 6, 2017

Uh oh!

vincent-d commented Apr 6, 2017

aabadie commented Mar 31, 2017 •

edited

Loading

aabadie Apr 5, 2017 •

edited

Loading

aabadie Apr 5, 2017 •

edited

Loading