drivers/adxl345: cleanup/enhancements by dylad · Pull Request #6799 · RIOT-OS/RIOT

dylad · 2017-03-27T20:40:48Z

@haukepetersen If you have some time, could you take a look at it please ?
I need some advises about the set state functions. I have to add a clear_state function for each mode. But I don't know how could I put it into one clear and efficient function.

haukepetersen · 2017-03-28T12:16:53Z

drivers/adxl345/adxl345.c


-int adxl345_init(adxl345_t *dev, const adxl345_params_t* params)
+int adxl345_init(adxl345_t *dev, const adxl345_params_t* params, uint8_t range,
+                 uint8_t rate, uint8_t full_res, float scale_factor)


As stated in https://github.com/RIOT-OS/RIOT/wiki/Guide:-Writing-a-device-driver-in-RIOT, the init function should have the dev and params struct as only parameters...

Sorry I didn't remember that part ... I'll change that but I saw others drivers with more than dev and params as arguments like drivers/isl29020

haukepetersen · 2017-03-28T12:22:08Z

drivers/include/adxl345.h

-    uint8_t range;                 /**< Sensitivity configuration */
-    uint8_t rate;                  /**< Configured sample rate for accel  */
-    uint8_t full_res;              /**< Resolution bit */
-    uint8_t scale_factor;          /**< Scale factor for converting value to mg */


I don't know the device in detail, but it seems to me, like some of these fields are static configuration values, while others can be deducted from them, right?
interrupt -> dynamic param set at runtime -> belongs into adx1345_t
offset -> static params, part of adx1345_params_t
range -> static param, part of adx1345_params_t
rate -> static param, part of adx1345_params_t
full_res -> deducted from range?! -> goes into device descriptor
scale_factor -> deducted from range -> goes into device descriptor

full_res is not deducted from range so it will belong to adxl345_params_t
I will move each part to right struct

haukepetersen · 2017-03-28T12:24:10Z

drivers/adxl345/adxl345.c

@@ -43,6 +44,12 @@ int adxl345_init(adxl345_t *dev, const adxl345_params_t* params)
    /* get device descriptor */
    dev->params= *params;


this seems wrong, assigment of adx1345_params_t ** to adx1345_params_t? If you do it like this, I think what you want to do is

memcpy(&dev->params, params, sizeof(adx1345_params_t));

But under further consideration, think of only copying the 'often used' fields (e.g. i2c, addr) into RAM and leave the 'use once' fields (e.g. offset) in ROM...

memcpy(&dev->params, params, sizeof(adx1345_params_t)); was the initial implementation in the previous PR but @aabadie made me change this line. I think copying all parameters with memcpy is more clear.

actually using dev->params = *params also works and it's also used in other places.

ture, didn't notice the * instead of &, so never mind...

But still the second consideration holds: IMHO we don't need to copy all config params into RAM...

@haukepetersen How am I suppose to put some members in RAM and others in ROM ? Should I remove them from the struct and call the #define params in adxl345_init function ?

noo, actually what I meant is quite simple: instead of having the device descriptor look like

typedef struct { adx1345_params_t params; ... } adx1345_t;

you could do something like

typedef struct { i2c_t bus, uint8_t addr, adx1345_params_t *p, .... } adx1345_t;

In this case, you would only copy the 'often/periodically' used values into RAM for faster access, while you access the rest of the config params indirectly from ROM (we assume the configuration to be placed in ROM...).

haukepetersen · 2017-03-28T12:25:56Z

drivers/adxl345/include/adxl345_params.h

-                                  .scale_factor = ADXL345_PARAM_SCALE_FACTOR}
+#define ADXL345_PARAMS          { .i2c    = ADXL345_PARAM_I2C,   \
+                                  .addr   = ADXL345_PARAM_ADDR,  \
+                                  .offset = ADXL345_PARAM_OFFSET }


why don't you write the offset here inline as in .offset = { 0, 0, 0 } } here? Saves some of the verbose lines below...

Because I copy/paste this from another driver without even thinking ! I'll change it.

dylad · 2017-03-28T15:04:53Z

@haukepetersen I add some commits
I didn't change the line dev->params= *params; yet. Should I switch to memcpy or leave it 'as is' ?

I will squash when we're done.

haukepetersen · 2017-03-30T07:52:44Z

I didn't change the line dev->params= *params; yet. Should I switch to memcpy or leave it 'as is' ?

nope, no need. But see my comment above about only copying some params into the dev descriptor, then you would have to change this line anyhow...

dylad · 2017-03-30T19:48:37Z

Thanks for your help @haukepetersen . I really tought you wanted something far more complicated...
I pushed lastest changes. I hope it is fine !

dylad · 2017-03-31T20:29:04Z

@haukepetersen
I'm facing a weird issue
I did some tests and notice some output data error when result is negative
I tested :

    data->x = (int16_t)(dev->scale_factor * ((float)((result[3] << 8)+result[2])));
    data->y = (int16_t)((float)((result[3] << 8)+result[2])* 3.9);
    data->z = (((result[3] << 8)+result[2]) );

data->x and data->y are supposed to be equals
and data->z * 3.9 should be equals to data->x in that case
The output result is :
X axis:-6558 Y axis:-6558 Z axis:-1

dev->scale_factor is a float equals to 3.9
data->x,y,z are all int16_t variables.
Why multiple -1 by 3.9 give me -6558 ?
I'm pretty sure the reason is obvious...

dylad · 2017-04-03T20:54:53Z

I fixed my problem. It works now.
What do you think ?

aabadie

A few nitpicks. Otherwise looks good.

aabadie · 2017-04-06T08:14:55Z

drivers/include/adxl345.h

-    adxl345_params_t params;      /**< Device configuration */
+    i2c_t i2c;                     /**< I2C device which is used */
+    uint8_t addr;                  /**< I2C address */
+    adxl345_params_t *params;       /**< Device configuration */


please align

aabadie · 2017-04-06T08:15:12Z

tests/driver_adxl345/main.c

        adxl345_read(&dev, &data);
        printf("Acceleration [in mg]: X axis:%d Y axis:%d Z axis:%d\n",
-                data.x, data.y, data.z);
+                (int)data.x, (int)data.y, (int)data.z);


aabadie · 2017-04-06T08:15:40Z

drivers/adxl345/include/adxl345_params.h

 #define ADXL345_PARAM_FULL_RES     (1)
 #endif
+#ifndef ADXL345_PARAM_OFFSET
+#define ADXL345_PARAM_OFFSET    { 0, 0, 0 }


please align defines in this file

aabadie · 2017-04-06T08:16:35Z

drivers/adxl345/adxl345.c

 #include <stdint.h>
 #include <stdbool.h>
 #include <string.h>
+#include <math.h>


is this really needed ?

dylad · 2017-04-06T17:38:01Z

@aabadie thanks for your review, I'll fix it.
Regarding #include <math.h> I have a question :

data->x = (((result[1] << 8)+result[0]) * dev->scale_factor);
data->y = (((result[3] << 8)+result[2]) * dev->scale_factor);
data->z = (((result[5] << 8)+result[4]) * dev->scale_factor);

Here I multiply a integer by a real number so the result is a float and I store it into a int16_t
Is it better to use the floor function to retrieve the integer or should I leave as it ?
Any thought ?

aabadie · 2017-04-06T20:04:46Z

Is it better to use the floor function to retrieve the integer or should I leave as it ?

I would say leave it as it is without the #include <math.h>.

aabadie

ACK, please squash

dylad · 2017-04-10T18:18:47Z

Squashed.

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

aabadie · 2017-04-11T08:34:13Z

Murdock approves, go !

dylad · 2017-04-11T08:36:01Z

many thanks to both of you @aabadie @haukepetersen !

haukepetersen · 2017-04-11T11:07:52Z

@aabadie: there were still un-addressed comments (see my comments above...). The state you merged is broken by design: what if you want to configure two of these devices? Now it won't work... Please make sure you look for open comments before you merge things...

aabadie · 2017-04-11T11:18:21Z

there were still un-addressed comments (see my comments above...)

I'm sorry, I don't which one you are talking about. But apologizes, I should have for your confirmation first.

The state you merged is broken by design: what if you want to configure two of these devices? Now it won't work

Can you be more explicit ?

dylad · 2017-04-11T11:22:29Z

@haukepetersen I really thought all your comments were addressed. Which one did I miss ?

aabadie · 2017-04-11T12:47:58Z

tests/driver_adxl345/main.c

+           dev.i2c);

-    if (adxl345_init(&dev, adxl345_params) == ADXL345_OK) {
+    if (adxl345_init(&dev, (adxl345_params_t*)adxl345_params) == ADXL345_OK) {


@haukepetersen, were you mentioning this regarding the fact only one sensor can be initialized at a time ? Then yes, I admit the line should be:

adxl345_init(&dev, &adxl345_params[0]) == ADXL345_OK

haukepetersen

my fault, seems like my browser did not complete the connection to github when i submitted this review...

haukepetersen · 2017-03-31T06:52:19Z

drivers/adxl345/adxl345.c

+#define ADDR                (dev->addr)

-int adxl345_init(adxl345_t *dev, const adxl345_params_t* params)
+int adxl345_init(adxl345_t *dev, adxl345_params_t* params)


the const should stay here...

haukepetersen · 2017-03-31T06:53:40Z

drivers/adxl345/adxl345.c


    /* get device descriptor */
-    dev->params= *params;
+    dev->params = (adxl345_params_t*)params;


but what happened to the i2c bus and the addr?

haukepetersen · 2017-03-31T06:54:02Z

drivers/include/adxl345.h

+    ADXL345_STANDBY_MODE,
+    ADXL345_SLEEP_MODE,
+    ADXL345_AUTOSLEEP_MODE,
 };


documentation missing

(even though obvious what the enum members mean, doxygen will certainly complain about it...)

haukepetersen · 2017-03-31T06:54:51Z

tests/driver_adxl345/main.c

    adxl345_data_t data;

+    dev.i2c = ADXL345_PARAM_I2C;
+    dev.addr = ADXL345_PARAM_ADDR;


remove these two...

haukepetersen · 2017-03-31T06:55:22Z

drivers/include/adxl345.h

+    uint8_t range;            /**< Sensitivity configuration */
+    uint8_t rate;             /**< Configured sample rate for accel  */
+    uint8_t full_res;         /**< Resolution bit */
 } adxl345_params_t;


i2c and addr must stay in the params struct...

haukepetersen · 2017-04-25T13:18:34Z

@aabadie, @dylad: sorry for my comments about open review comments, something went wrong on my side when I tried to send my review comments... Hope they help to shed some light on the open points that I had in mind...

dylad · 2017-04-25T13:23:25Z

@haukepetersen
Thanks for your review. Since this one is merged, I'll submit another PR soon (in May).

haukepetersen · 2017-04-25T13:33:15Z

perfect, thanks!

haukepetersen reviewed Mar 28, 2017

View reviewed changes

miri64 assigned aabadie and haukepetersen Apr 6, 2017

aabadie reviewed Apr 6, 2017

View reviewed changes

aabadie approved these changes Apr 10, 2017

View reviewed changes

dylad force-pushed the adxl345_cleanup branch from 1ed0eed to 530a2ef Compare April 10, 2017 18:17

drivers/adxl345: cleanup/enhancements

530a2ef

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation 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 Apr 10, 2017

aabadie added this to the Release 2017.04 milestone Apr 10, 2017

aabadie changed the title ~~drivers/adxl345: cleanup/enhancements [WIP]~~ drivers/adxl345: cleanup/enhancements Apr 10, 2017

aabadie merged commit dffbc70 into RIOT-OS:master Apr 11, 2017

aabadie reviewed Apr 11, 2017

View reviewed changes

haukepetersen reviewed Apr 25, 2017

View reviewed changes

dylad deleted the adxl345_cleanup branch May 7, 2017 09:05

dylad mentioned this pull request May 16, 2017

drivers/adxl345: fixes & cleanup #7067

Merged

		@@ -43,6 +44,12 @@ int adxl345_init(adxl345_t dev, const adxl345_params_t params)
		/* get device descriptor */
		dev->params= *params;

Conversation

dylad commented Mar 27, 2017

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!

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!

dylad commented Mar 28, 2017

Uh oh!

haukepetersen commented Mar 30, 2017

Uh oh!

dylad commented Mar 30, 2017

Uh oh!

dylad commented Mar 31, 2017

Uh oh!

dylad commented Apr 3, 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!

dylad commented Apr 6, 2017

Uh oh!

aabadie commented Apr 6, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

dylad commented Apr 10, 2017

Uh oh!

aabadie commented Apr 11, 2017

Uh oh!

dylad commented Apr 11, 2017

Uh oh!

haukepetersen commented Apr 11, 2017

Uh oh!

aabadie commented Apr 11, 2017

Uh oh!

dylad commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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