drivers: add driver for FXOS8700 3-axis accelerometer/magnetometer by Hyungsin · Pull Request #8978 · RIOT-OS/RIOT

Hyungsin · 2018-04-18T21:47:43Z

This PR provides a driver for the FXOS8700 sensor, which reads 3-axis acceleration and magnetic field.
Driver implementation style is similar to HDC1000 driver.

jnohlgard · 2018-04-19T04:39:47Z

Nice! This sensor can be found on the frdm-kw41z board.

jnohlgard

Some comments below. I will test on the frdm-kw41z board. I can provide a board config for that particular setup when I get it running.

jnohlgard · 2018-04-19T04:42:20Z

drivers/fxos8700/include/fxos8700_params.h

+ */
+static const saul_reg_info_t fxos8700_saul_info[] =
+{
+    HDC1000_SAUL_INFO


jnohlgard · 2018-04-19T04:46:36Z

drivers/fxos8700/fxos8700.c

+        DEBUG("[fxos8700] Could not read WHOAMI (%d)\n", rv);
+        return FXOS8700_NOBUS;
+    }
+    dev->whoami = config;


Is there any use case for saving this value? It seems like it is never used by the implementation.

jnohlgard · 2018-04-19T04:48:48Z

drivers/include/fxos8700.h

+} fxos8700_params_t;
+
+/**
+  * @brief   Device descriptor for a AT30TSE75x device


Copy pasta, and weird indentation

jnohlgard · 2018-04-19T04:49:15Z

drivers/include/fxos8700.h

+} fxos8700_t;
+
+/**
+  * @brief   Individual hybrid measurement


Weird indent

jnohlgard · 2018-04-19T04:51:23Z

drivers/include/fxos8700.h

+        uint8_t  ydr:1;       /*!< bit:      0  Start Conversion Event In          */
+        uint8_t  xdr:1;       /*!< bit:      1  Synchronization Event In           */
+    } bit;                    /*!< Structure used for bit  access                  */
+    uint8_t reg;              /*!< Type      used for register access              */


Here and everywhere else: use /** (and of course /**<) for doxygen comments in RIOT.

jnohlgard · 2018-04-19T04:56:56Z

drivers/include/fxos8700.h

+        uint8_t  xdr:1;       /*!< bit:      1  Synchronization Event In           */
+    } bit;                    /*!< Structure used for bit  access                  */
+    uint8_t reg;              /*!< Type      used for register access              */
+} FXOS8700_STATUS_Type;


This type, and the other bit fields below, are not actually used by the implementation.
Additionally, it is not required by a compiler to pack the bit fields in the struct, so this bit representation will not be guaranteed to match the hardware register bits. It is more robust to use bit mask macros or other bit operations in the implementation. The compiler should be smart enough to optimize the accesses in a good way when one of the operands is a numeric literal or static constant.

jnohlgard · 2018-04-19T05:11:45Z

The cache does not handle multiple instances.
I understand that your projects use the caching to reduce the power consumption while reading multiple successive reads, but I wonder if we should generalize the cache into its own module and let it wrap any sensor driver, with configurable cache time to live. Most sensors have the same kind of read function API where you pass a driver instance as the first argument and an output buffer as the next argument with error codes as the return value, that would be easily wrapped by a generic cache implementation.

jnohlgard · 2018-04-19T05:13:22Z

Otherwise we will likely end up with various cache implementations in each sensor driver, with plenty of opportunity for mistakes and difficulties testing them in a reliable way.

jnohlgard · 2018-04-19T05:44:13Z

I think it is a good verification to read the whoami register during init, to check that the device is responding and that the connected device is actually a FXOS8700 sensor by comparing the actual value against the expected value. Just that it does not need to be saved in the context struct.

Hyungsin · 2018-04-19T05:56:19Z

@gebart. Thank you for the quick review! I resolved you comments.

WHOAMI comparison is still here but its value is not saved now.

Regarding the cache, I don't know how many sensors in RIOT are relevant (one sensor should provide multiple types of information). If this cache functionality is useful for many other sensors, we can think of generalizing it. But I think this should be another PR.

jnohlgard

Minor code style issues.

jnohlgard · 2018-04-19T08:41:54Z

drivers/fxos8700/fxos8700.c

+        int32_t acc_raw_z = (int32_t) ((data[4]<<6) | (data[5]>>2));
+        acc->x = (int16_t)(acc_raw_x*244/1000);
+        acc->y = (int16_t)(acc_raw_y*244/1000);
+        acc->z = (int16_t)(acc_raw_z*244/1000);


Code style: add spaces around operators

Also, for clarity it may be better to have parentheses around (raw * 244) / 1000, though it changes nothing in the generated code since the associativity is left to right for the multiplication and division operators.

jnohlgard · 2018-04-19T08:52:01Z

drivers/fxos8700/fxos8700.c

+    if (mag) {
+        mag->x = (int16_t) ((data[6] <<8) | data[7]);
+        mag->y = (int16_t) ((data[8] <<8) | data[9]);
+        mag->z = (int16_t) ((data[10]<<8) | data[11]);


Spaces around bit shift operators

jnohlgard · 2018-04-19T08:58:13Z

drivers/fxos8700/fxos8700.c

+
+int fxos8700_read_cached(const fxos8700_t *dev, fxos8700_measurement_t* acc, fxos8700_measurement_t* mag)
+{
+    uint32_t now = xtimer_now_usec();


Optional: It may be less computationally expensive to use xtimer_now and convert the renew interval to ticks instead, since you can precompute that us to tick conversion at compilation time rather than do a conversion from ticks to us in every read. See this as an optional comment, and maybe just remember for future drivers, I assume this function will be called somewhat infrequently since the renew interval is set to 1 second.

Good point! I will remember for the future driver implementations which require frequent xtimer access 👍

jnohlgard · 2018-04-19T09:01:58Z

Are you using this sensor externally or integrated on any dev board? Do you have any updated board configurations you want to add here?

aabadie

Rough pass of review. I found problems with doxygen and style and have minor comments in the driver implementation (return code, defines, etc)

aabadie · 2018-04-19T09:10:36Z

drivers/fxos8700/fxos8700_saul.c

+ */
+
+/**
+ * @ingroup     driver_fxos8700


should be drivers_fxos8700

aabadie · 2018-04-19T09:11:11Z

drivers/fxos8700/fxos8700.c

+static int fxos8700_read_regs(fxos8700_t* dev, uint8_t reg, uint8_t* data, size_t len)
+{
+    i2c_acquire(dev->p.i2c);
+    if(i2c_read_regs(dev->p.i2c, dev->p.addr, reg, (char*) data, len) <= 0) {


missing space after if here and in other places below

aabadie · 2018-04-19T09:11:46Z

drivers/fxos8700/fxos8700.c

+        return FXOS8700_BUSERR;
+    }
+
+    while(!(ready & 0x08)) {


missing space after while here and below. Maybe unscrustify the file ?

aabadie · 2018-04-19T09:12:13Z

drivers/fxos8700/fxos8700.c

@@ -0,0 +1,205 @@
+/*
+ * Copyright (C) 2017 UC Berkeley


Shouldn't it be 2018 ?

aabadie · 2018-04-19T09:12:37Z

drivers/fxos8700/fxos8700_saul.c

@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2017 Hyung-Sin Kim


Not UC Berkeley ?

aabadie · 2018-04-19T09:18:13Z

drivers/include/fxos8700.h

+}
+#endif
+
+/** @} */


This line should be after #endif below

aabadie · 2018-04-19T09:19:20Z

drivers/fxos8700/fxos8700.c

+    if (config != FXOS8700_WHO_AM_I_VAL) {
+        i2c_release(dev->p.i2c);
+        DEBUG("[fxos8700] WHOAMI is wrong (%2x)\n", config);
+        return FXOS8700_NOBUS;


Maybe add another return status for this case ? Something line FXOS8700_NODEV ?

aabadie · 2018-04-19T09:20:09Z

drivers/fxos8700/fxos8700.c

+    /* Configure the ODR to maximum (400Hz in hybrid mode) */
+    config = 0x00;
+    if (fxos8700_write_regs(dev, FXOS8700_REG_CTRL_REG1, &config, 1) != FXOS8700_OK) {
+        return FXOS8700_NOBUS;


Should it be FXOS8700_BUSERR instead ? Here and in other places below.

aabadie · 2018-04-19T09:20:55Z

drivers/fxos8700/fxos8700.c

+#define FXOS8700_RENEW_INTERVAL 1000000ul
+#endif
+
+#define ACCEL_ONLY_MODE         0x00


Maybe put parenthesis around these hexadecimal defines

aabadie · 2018-04-19T09:22:31Z

drivers/fxos8700/fxos8700.c

+    i2c_acquire(dev->p.i2c);
+    if(i2c_read_regs(dev->p.i2c, dev->p.addr, reg, (char*) data, len) <= 0) {
+        DEBUG("[fxos8700] Can't read register 0x%x\n", reg);
+        i2c_release(dev->p.i2c);


You could use for example:

#define DEV_I2C (dev->p.i2c)

at the beginning of this file. Same with addr attribute and maybe others

I followed the style of the current HDC1000 implementation. Is this style a strict requirement?

No, that was more a suggestion for future maintenance, technically it doesn't change anything.

I see. Thanks!

Hyungsin · 2018-04-19T17:26:50Z

@gebart @aabadie
Thank you for the reviews and I resolved comments again. 👍

@gebart, I'm using FXOS8700 on the hamilton board, which I plan to include in RIOT.
Basically, all sensor/clock/radio drivers I have modified/added so far are related to the hamilton.

aabadie · 2018-04-19T21:04:40Z

drivers/fxos8700/fxos8700.c

+#define MAG_ONLY_MODE           (0x01)
+#define HYBRID_MODE             (0x03)
+
+static fxos8700_measurement_t acc_cached, mag_cached;


Instead of those static variable; why no having them as attributes of fxos8700_t ?
Because otherwise this will be a problem with multiple instances of this device as already reported by @gebart here.That could be a best temporary solution until we have a module for managing cache.
Maybe saul could do it ?

I agree and updated this PR reflecting this comment. Thanks!

aabadie · 2018-04-19T22:07:28Z

drivers/include/fxos8700.h

+ * @brief   Device descriptor for a FXOS8700 device
+ */
+typedef struct {
+    fxos8700_measurement_t acc_cached;


The attributes of this struct are missing documentation, same with the fxos8700_measurement_t struct

aabadie · 2018-04-19T22:08:35Z

Could you also add a test application ?

Hyungsin · 2018-04-20T17:05:50Z

@aabadie, @gebart, I addressed your comments.
and @gebart, let me test this with my device also and I will let you know. It worked before but there could be some bugs while rearranging codes.

Hyungsin · 2018-04-20T20:03:17Z

@gebart, I checked that it works at least in my case (hamilton), as below.

Dev: fxos8700 Type: SENSE_ACCEL
Data: [0] 15mg
[1] 1005mg
[2] 156mg

Can you retry?

jnohlgard · 2018-04-20T20:13:56Z

I will check again tomorrow

jnohlgard · 2018-04-22T15:45:16Z

My frdm-kw41z is giving strange values, I will try again with a frdm-k22f board tomorrow

jnohlgard

I found the source of my problems. Negative values of acceleration are not handled correctly, see my comment below regarding the bit shift

jnohlgard · 2018-04-23T10:06:36Z

drivers/fxos8700/fxos8700.c

+    }
+
+    /* Read all data at once */
+    if (fxos8700_read_regs(dev, FXOS8700_REG_OUT_X_MSB, &data[0], 12)) {


change magic 12 into sizeof something or a macro definition.

jnohlgard · 2018-04-23T10:08:12Z

drivers/fxos8700/fxos8700.c

+    if (acc) {
+        int32_t acc_raw_x = (int32_t) ((data[0] << 6) | (data[1] >> 2));
+        int32_t acc_raw_y = (int32_t) ((data[2] << 6) | (data[3] >> 2));
+        int32_t acc_raw_z = (int32_t) ((data[4] << 6) | (data[5] >> 2));


the shift of 6 loses the sign bit

This gives reasonable results for negative numbers:

int32_t acc_raw_x = (int16_t) ((data[0] << 8) | data[1]) >> 2; int32_t acc_raw_y = (int16_t) ((data[2] << 8) | data[3]) >> 2; int32_t acc_raw_z = (int16_t) ((data[4] << 8) | data[5]) >> 2; acc->x = (int16_t) ((acc_raw_x * 244) / 1000); acc->y = (int16_t) ((acc_raw_y * 244) / 1000); acc->z = (int16_t) ((acc_raw_z * 244) / 1000);

Very good point. Thanks!!

jnohlgard · 2018-04-23T10:20:25Z

drivers/fxos8700/fxos8700.c

+        int32_t acc_raw_z = (int32_t) ((data[4] << 6) | (data[5] >> 2));
+        acc->x = (int16_t) ((acc_raw_x * 244) / 1000);
+        acc->y = (int16_t) ((acc_raw_y * 244) / 1000);
+        acc->z = (int16_t) ((acc_raw_z * 244) / 1000);


You are really discarding quite a lot of bits of precision with this division. You could just as well configure the sensor to +/- 8 g range and use x * 976 / 1000 instead and get the benefit of an increased sensor range without any difference in accuracy compared to the current implementation

Hyungsin · 2018-04-23T20:21:12Z

@gebart, thank you for improving this driver!

I made accelerator's scale configurable and replaced some magic numbers with macros.

Hyungsin · 2018-05-12T01:19:08Z

@gebart @aabadie, any comment? good to be merged?

aabadie

Codewise it looks good now.

jnohlgard

My comment regarding the lost bits when using 2 or 4 g full scale range still applies, but I don't intend to block this PR just for that.

jnohlgard · 2018-05-12T19:24:57Z

Optionally, change the division from 1000 to 100 and change the SAUL scale from -3 to -4 when +/- 2 G range is selected

Hyungsin · 2018-05-13T00:54:40Z

@gebart, I updated, but I think this driver does not lose any bit regardless of resolution setting since it provides "FXOS8700_USE_ACC_RAW_VALUES" mode.

jnohlgard · 2018-05-13T04:49:53Z

👍

jnohlgard · 2018-05-13T07:47:41Z

@Hyungsin in future PRs, please create fixup/squash commits for changes you add after the initial review to make the review process easier. You can amend typos or other minor corrections, but for anything a little bigger, such as when you added the different scaling factor configurations in this PR, use a separate commit. Then squash the commits when the reviewer requests it. It makes it easier for the reviewer to know what has changed between iterations.

Hyungsin · 2018-05-14T01:16:09Z

@gebart, good point. I should keep this in mind!

aabadie · 2018-05-14T06:45:54Z

All green here. Let's merge.

Hyungsin force-pushed the forupstream_fxos8700 branch 3 times, most recently from 9dda5b2 to caffc42 Compare April 18, 2018 22:19

jnohlgard requested changes Apr 19, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_fxos8700 branch from b22891e to 495a94a Compare April 19, 2018 05:40

Hyungsin force-pushed the forupstream_fxos8700 branch from 495a94a to 921de7a Compare April 19, 2018 05:53

Hyungsin force-pushed the forupstream_fxos8700 branch 2 times, most recently from 52a33f5 to 933f2dd Compare April 19, 2018 06:03

jnohlgard reviewed Apr 19, 2018

View reviewed changes

aabadie added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Apr 19, 2018

aabadie requested changes Apr 19, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_fxos8700 branch from 72211b9 to b7484c4 Compare April 19, 2018 17:16

aabadie reviewed Apr 19, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_fxos8700 branch 4 times, most recently from ef2c5c5 to 7e388d3 Compare April 19, 2018 21:49

aabadie reviewed Apr 19, 2018

View reviewed changes

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 19, 2018

Hyungsin force-pushed the forupstream_fxos8700 branch 2 times, most recently from ab7d496 to 64b1963 Compare April 19, 2018 23:44

Hyungsin force-pushed the forupstream_fxos8700 branch from 4b747d3 to 2e6a557 Compare April 20, 2018 17:04

Hyungsin force-pushed the forupstream_fxos8700 branch 2 times, most recently from 1098e15 to 9195661 Compare April 20, 2018 17:11

jnohlgard requested changes Apr 23, 2018

View reviewed changes

jnohlgard reviewed Apr 23, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_fxos8700 branch 3 times, most recently from 80b99b8 to 48d3bd9 Compare April 23, 2018 20:12

Hyungsin mentioned this pull request Apr 24, 2018

board: add hamilton board support #9013

Merged

Hyungsin force-pushed the forupstream_fxos8700 branch from 48d3bd9 to ba6ef8b Compare April 24, 2018 19:37

aabadie approved these changes May 12, 2018

View reviewed changes

jnohlgard approved these changes May 12, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_fxos8700 branch from ba6ef8b to 67673b2 Compare May 13, 2018 00:51

drivers: add driver for FXOS8700 3-axis accelerometer/magnetometer

beaf08b

Hyungsin force-pushed the forupstream_fxos8700 branch from 67673b2 to beaf08b Compare May 13, 2018 00:52

aabadie merged commit 372aadd into RIOT-OS:master May 14, 2018

Hyungsin mentioned this pull request Jun 20, 2018

drivers/hdc1000: change renew_interval to a parameter #9386

Merged

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

Hyungsin commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Apr 19, 2018

Uh oh!

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

jnohlgard commented Apr 19, 2018

Uh oh!

jnohlgard commented Apr 19, 2018

Uh oh!

jnohlgard commented Apr 19, 2018

Uh oh!

Hyungsin commented Apr 19, 2018

Uh oh!

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

jnohlgard commented Apr 19, 2018

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!

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 19, 2018 • 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!

Hyungsin commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Hyungsin commented Apr 18, 2018 •

edited

Loading

aabadie Apr 19, 2018 •

edited

Loading

Hyungsin commented Apr 20, 2018 •

edited

Loading

Hyungsin commented Apr 23, 2018 •

edited

Loading