drivers: add support for LIS2DH12 acceleromter (SPI mode) by haukepetersen · Pull Request #8381 · RIOT-OS/RIOT

haukepetersen · 2018-01-18T14:45:04Z

Contribution description

This PR adds a device driver for STM lis2dh12 accelerometers. In the current state, the driver supports only to interface the device via SPI (as used on the ruuvitag). I2C support will be added in a follow up (as used on the thingy52), but the structure of the driver is already prepared for this...

In the current state, the driver is very rudimentary, as it supports only a very basic operating mode with continuous sampling. Events, interrupts, and low power modes are not yet supported.

Issues/PRs references

one step towards full support of the ruuvitag (#8366)

haukepetersen · 2018-01-18T14:50:50Z

rebased

haukepetersen · 2018-01-18T15:05:08Z

made lis2dh12_spi default in the test for now.

kaspar030

Only minor issues.

kaspar030 · 2018-01-18T15:08:06Z

drivers/include/lis2dh12.h

+extern "C" {
+#endif
+
+/* So for I2C support is not implemented, so throw an error when selected */


drop So for

kaspar030 · 2018-01-18T15:10:20Z

drivers/lis2dh12/lis2dh12.c

+ * @brief       LIS2DH12 accelerometer driver implementation
+ *
+ * @author      Hauke Petersen <hauke.petersen@fu-berlin.de>
+ *


remove line

kaspar030 · 2018-01-18T15:11:22Z

drivers/lis2dh12/lis2dh12.c

+/* flag to enable address auto incrementation on read or write */
+#define AUTOINC             (0x40)
+
+static inline int acquire(const lis2dh12_t *dev)


here and below: no need to inline when not in *.c file

kaspar030 · 2018-01-18T15:12:13Z

drivers/lis2dh12/lis2dh12.c

+    spi_release(BUS);
+}
+
+static inline uint8_t read(const lis2dh12_t *dev, uint8_t reg)


while this might work, IMO name clashes with standard functions should be avoided. _[a-z] is guaranteed to be free. ;)

kaspar030 · 2018-01-18T15:12:46Z

drivers/lis2dh12/lis2dh12.c

+    assert(dev && params);
+
+    dev->p = params;
+    dev->comp = (1000 * (0x02 << (dev->p->scale >> 4)));


kaspar030 · 2018-01-18T15:13:38Z

drivers/lis2dh12/lis2dh12.c

+    spi_transfer_regs(BUS, CS, (READ | AUTOINC | reg), NULL, data, len);
+}
+
+static inline void write(const lis2dh12_t *dev, uint8_t reg, uint8_t data)


also name clashing with posix function

kaspar030 · 2018-01-18T15:17:37Z

tests/driver_lis2dh12/main.c

+#include "lis2dh12_params.h"
+
+
+#define DELAY       (100 * US_PER_MS)


kaspar030 · 2018-01-18T15:19:58Z

tests/driver_lis2dh12/main.c

+
+int main(void)
+{
+    xtimer_ticks32_t last_wakeup = xtimer_now();


I propose moving this in front of the while queue so the following puts() are not part of the first interval...

kaspar030 · 2018-01-18T15:21:54Z

tests/driver_lis2dh12/main.c

+    }
+
+    while (1) {
+        xtimer_periodic_wakeup(&last_wakeup, DELAY);


maybe move to the end of the loop?

haukepetersen · 2018-01-18T15:39:57Z

addressed comments

kaspar030 · 2018-01-18T15:41:58Z

please squash!

haukepetersen · 2018-01-18T15:43:58Z

squashed.

kaspar030

Tested on ruuvitag, works like a charm. ACK.
Nice PR!

aabadie

Found some typo and possible wording improvements. Among other minor things.

aabadie · 2018-01-18T15:57:40Z

drivers/include/lis2dh12.h

+ *
+ * This device driver provides a minimal interface to LIS2DH12 devices. As of
+ * now, it only provides very basic access to the device. The driver configures
+ * the device into continuously reading the acceleration data with statically


s/continuously reading/continuous reading of/ ?

continuously is an adverb of reading?

Yeah, that looked weird at first sight but ok, let's keep this one as it is

aabadie · 2018-01-18T15:59:27Z

drivers/include/lis2dh12.h

+/**
+ * @brief   Available sampling rates
+ *
+ * @note    The device does also support some additional rates for specific low-


s/does also support/also supports/

in this case, I like the existing wording better...

aabadie · 2018-01-18T16:02:14Z

drivers/lis2dh12/lis2dh12.c

+    _read_burst(dev, REG_OUT_X_L, raw, 6);
+    _release(dev);
+
+    for (int i = 0; i < 3; i++) {


Why not use a defined constant instead of 3 ?

aabadie · 2018-01-18T16:02:30Z

drivers/lis2dh12/lis2dh12.c

+{
+    assert(dev && data);
+
+    uint8_t raw[6];


aabadie · 2018-01-18T16:03:49Z

sys/auto_init/saul/auto_init_lis2dh12.c

+
+void auto_init_lis2dh12(void)
+{
+    for (unsigned int i = 0; i < LIS2DH12_NUM; i++) {


Maybe add an assert between LIS2DH12_NUM and a LIS2DH12_INFO_NUM, see #7937 for examples.

aabadie · 2018-01-18T16:05:42Z

tests/driver_lis2dh12/main.c

+
+#define DELAY       (100UL * US_PER_MS)
+
+/* allocate some memory for holding the formated sensor output */


s/formated/formatted/

aabadie · 2018-01-18T16:05:52Z

tests/driver_lis2dh12/main.c

+#define DELAY       (100UL * US_PER_MS)
+
+/* allocate some memory for holding the formated sensor output */
+static char str_out[3][8];


What are those magic numbers? Maybe describe the "formatted sensor output" ?

haukepetersen · 2018-01-18T16:36:25Z

@aabadie addressed comments, thanks for the feedback

aabadie · 2018-01-18T16:53:54Z

tests/driver_lis2dh12/README.md

+This is a manual test application for the LIS2DH12 accelerometer driver.
+
+# Usage
+This test application will initialize the accelerometer with the its default


there's an extra 'the' before 'its'

aabadie

ACK when the comment above is fixed. Please squash.

haukepetersen · 2018-01-18T17:38:21Z

fixed readme

haukepetersen · 2018-01-18T17:40:06Z

and squashed

haukepetersen · 2018-01-18T18:45:29Z

ACKed and all green -> go.

Thanks for the quick reviews!

haukepetersen added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 18, 2018

haukepetersen assigned aabadie and kaspar030 Jan 18, 2018

haukepetersen force-pushed the add_driver_lis2dh12 branch from 96c3f50 to 3d91529 Compare January 18, 2018 14:46

haukepetersen force-pushed the add_driver_lis2dh12 branch from 3d91529 to 38b9365 Compare January 18, 2018 14:50

kaspar030 requested changes Jan 18, 2018

View reviewed changes

haukepetersen force-pushed the add_driver_lis2dh12 branch from fdec3b2 to 8575701 Compare January 18, 2018 15:43

kaspar030 approved these changes Jan 18, 2018

View reviewed changes

aabadie requested changes Jan 18, 2018

View reviewed changes

aabadie reviewed Jan 18, 2018

View reviewed changes

aabadie approved these changes Jan 18, 2018

View reviewed changes

haukepetersen added 6 commits January 18, 2018 18:39

boards/ruuvitag: init and set CS pins in board_init

14842f9

drivers: add support for STM lis2dh12 accelerometer

09dab1a

drivers/lis2dh12: add SAUL mapping

ba8ab6c

sys/auto_init: add SAUL initialization for lis2dh12

bb8ba48

tests: add test for lis2dh12 device driver

e31532b

boards/ruuvitag: enable lis2dh12 for saul_default

0c8b131

haukepetersen force-pushed the add_driver_lis2dh12 branch from 96128f2 to 0c8b131 Compare January 18, 2018 17:40

haukepetersen merged commit a896d95 into RIOT-OS:master Jan 18, 2018

haukepetersen deleted the add_driver_lis2dh12 branch January 18, 2018 18:45

haukepetersen mentioned this pull request Jan 23, 2018

drivers/lis2dh12: add I2C mode #8419

Closed


		#define DELAY (100UL * US_PER_MS)

		/* allocate some memory for holding the formated sensor output */

Conversation

haukepetersen commented Jan 18, 2018

Contribution description

Issues/PRs references

Uh oh!

haukepetersen commented Jan 18, 2018

Uh oh!

haukepetersen commented Jan 18, 2018

Uh oh!

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

haukepetersen commented Jan 18, 2018

Uh oh!

kaspar030 commented Jan 18, 2018

Uh oh!

haukepetersen commented Jan 18, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

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!

haukepetersen commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jan 18, 2018

Uh oh!

haukepetersen commented Jan 18, 2018

Uh oh!

haukepetersen commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone