drivers/adxl345: fixes & cleanup by dylad · Pull Request #7067 · RIOT-OS/RIOT

dylad · 2017-05-16T19:36:33Z

@haukepetersen As promised, this PR should address all your missing reviews from #6799
I hope I didn't forget something.

aabadie

Just a minor comment, otherwise look good. @haukepetersen what do you think ?

aabadie · 2017-06-23T08:36:42Z

tests/driver_adxl345/main.c

+           adxl345_params->i2c);

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


should be &adxl345_params[0], since ADXL_PARAMS macro may contain a list (defined by user).

@aabadie I'll address your comment after @haukepetersen's review

I am with @aabadie on this.

haukepetersen

Looks good, two minor things and this PR is good to go.

haukepetersen · 2017-08-25T12:37:08Z

tests/driver_adxl345/main.c

    puts("ADXL345 test application");
    printf("Initializing ADXL345 accelerometer at I2C_DEV(%i)... ",
-           dev.i2c);
+           adxl345_params->i2c);


this should be adx11345_params[0].i2c if I am not mistaken. Comes down to the same thing, but would semantically the right thing to do here.

haukepetersen · 2017-08-25T12:37:18Z

tests/driver_adxl345/main.c

+           adxl345_params->i2c);

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


I am with @aabadie on this.

haukepetersen · 2017-08-25T12:37:51Z

and sorry for the long reaction time...

dylad · 2017-08-28T19:22:26Z

@aabadie @haukepetersen
I addressed your comments and rebased.
Waiting for ACK :)

dylad · 2017-10-10T18:34:40Z

ping

dylad · 2017-11-10T21:06:54Z

Rebased on lastest master.
This PR is still looking for ACK so we can push the green button :)

aabadie · 2017-11-10T21:14:10Z

drivers/adxl345/adxl345.c


-#define BUS                 (dev->i2c)
-#define ADDR                (dev->addr)
+#define BUS                 (dev->params->i2c)


I think it should be dev->params.i2c instead of dev->params->i2c, same issue on the line below.

params is a pointer, hence params->i2c

aabadie · 2017-11-10T21:15:13Z

drivers/include/adxl345.h

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


no need to use a pointer for params (was already the case)

But it was requested by @haukepetersen in #6799 to save a bit of RAM.
The idea was the keep the often used data in RAM and let the rest in ROM.

I went through #6799 but couldn't find any reference of that. Looking at the device driver implementation (especially this paragraph), the params attribute shouldn't be a pointer (it's called p in the wiki)

I ACK if you change that @dylad ;)

@aabadie see #6799 (comment)
@haukepetersen what should we do ?

@haukepetersen, what's your opinion here ? (You wrote the wiki and gave the pointer advice)

I agree with using a pointer. This lets us put the configuration constants in ROM instead of in RAM. adxl345_params_t is significantly larger than a single pointer and I doubt the double dereference will cause any measurable performance delays when communicating with a I2C device.

kaspar030 · 2018-04-16T10:55:20Z

ping

dylad · 2018-04-16T12:37:41Z

@haukepetersen ping

kaspar030 · 2018-04-20T09:02:28Z

postponed

tcschmidt · 2018-05-29T20:18:32Z

@haukepetersen ping

dylad · 2018-06-21T16:44:54Z

I guess we will never have @haukepetersen 's response here. Should I close ?

tcschmidt · 2018-06-21T21:44:52Z

We can look for other reviewers and clear the request of @haukepetersen - what about your requests @aabadie ?

aabadie · 2018-06-22T16:28:16Z

The problem here is not about clearing @haukepetersen's review but rather having his opinion on this comment and the associated thread.

tcschmidt · 2018-06-26T10:50:27Z

@aabadie if @haukepetersen is not sharing his opinion we need someone else to evaluate the case. Unresponsiveness is a DoS to the process, and this has been almost a year by now.

aabadie · 2018-06-26T18:49:56Z

Unresponsiveness is a DoS to the process, and this has been almost a year by now.

Agreed @tcschmidt, let's ask @gebart who commented somewhere else about a similar problem.

no reply

jnohlgard

Looks good to me, untested ACK

jnohlgard · 2018-06-27T16:20:29Z

drivers/include/adxl345.h

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


I agree with using a pointer. This lets us put the configuration constants in ROM instead of in RAM. adxl345_params_t is significantly larger than a single pointer and I doubt the double dereference will cause any measurable performance delays when communicating with a I2C device.

jnohlgard · 2018-06-27T16:21:56Z

drivers/adxl345/adxl345.c


-#define BUS                 (dev->i2c)
-#define ADDR                (dev->addr)
+#define BUS                 (dev->params->i2c)


params is a pointer, hence params->i2c

tcschmidt · 2018-07-05T13:57:55Z

@aabadie are your concerns addressed ?

dylad · 2018-07-05T14:01:51Z

This PR is blocked as long as the I2C embargo remains so no need to hurry.

kaspar030 · 2018-07-05T14:06:57Z

This PR is blocked as long as the I2C embargo remains so no need to hurry.

Maybe we need a label for that?

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

dylad · 2018-08-03T09:44:58Z

I resolved a conflict introduces by the I2C rework, now the embargo is over we can handle this PR :)

tcschmidt · 2018-08-03T10:22:33Z

Yep - but now @aabadie is gone. Is it easy to resolve @PeterKietzmann or shall we wait for Alex?

dylad · 2018-08-03T10:50:45Z

We can wait for @aabadie to come back I guess. I don't mind.

PeterKietzmann · 2018-08-09T09:18:19Z

The PR is ACKed and only waits for testing. Unfortunately I don't have such a device available. @aabadie do you? Then, you can directly remove your change request :-). As I see it, the question about using a pointer to params seems to be answered.

tcschmidt · 2018-08-21T17:17:49Z

@aabadie ping

aabadie

Let's go with this one. ACK

Other drivers need to be adapted the same way.

aabadie assigned haukepetersen May 17, 2017

aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels May 17, 2017

aabadie added this to the Release 2017.07 milestone May 17, 2017

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 31, 2017

aabadie requested changes Jun 23, 2017

View reviewed changes

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

haukepetersen previously requested changes Aug 25, 2017

View reviewed changes

dylad force-pushed the adxl345_fix branch from a3c51f1 to 62b1d70 Compare August 28, 2017 19:20

dylad force-pushed the adxl345_fix branch from 62b1d70 to 66a9358 Compare November 10, 2017 21:05

aabadie reviewed Nov 10, 2017

View reviewed changes

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng modified the milestones: Release 2018.01, Release 2018.04 Jan 18, 2018

kaspar030 removed this from the Release 2018.04 milestone Apr 20, 2018

aabadie mentioned this pull request Apr 27, 2018

drivers/srf04: range finder sensor #9035

Merged

tcschmidt requested a review from jnohlgard June 26, 2018 21:01

jnohlgard approved these changes Jun 27, 2018

View reviewed changes

aabadie mentioned this pull request Jun 28, 2018

drivers/lis3dh: apply unified driver params scheme + cleanup #8697

Merged

dylad added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 5, 2018

dylad removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 3, 2018

drivers/adxl345: fixes & cleanup

d114e28

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

dylad force-pushed the adxl345_fix branch from 66a9358 to d114e28 Compare August 3, 2018 09:13

aabadie approved these changes Aug 22, 2018

View reviewed changes

aabadie merged commit 463b04a into RIOT-OS:master Aug 22, 2018

jia200x modified the milestone: Release 2018.10 Oct 4, 2018

dylad deleted the adxl345_fix branch May 8, 2019 18:53

Conversation

dylad commented May 16, 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!

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!

haukepetersen commented Aug 25, 2017

Uh oh!

dylad commented Aug 28, 2017

Uh oh!

dylad commented Oct 10, 2017

Uh oh!

dylad commented Nov 10, 2017

Uh oh!

aabadie Nov 10, 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!

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 Nov 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Nov 16, 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!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Apr 16, 2018

Uh oh!

dylad commented Apr 16, 2018

Uh oh!

kaspar030 commented Apr 20, 2018

Uh oh!

tcschmidt commented May 29, 2018

Uh oh!

dylad commented Jun 21, 2018

Uh oh!

tcschmidt commented Jun 21, 2018

Uh oh!

aabadie commented Jun 22, 2018

Uh oh!

tcschmidt commented Jun 26, 2018

Uh oh!

aabadie commented Jun 26, 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!

tcschmidt commented Jul 5, 2018

Uh oh!

dylad commented Jul 5, 2018

Uh oh!

aabadie Nov 10, 2017 •

edited

Loading

aabadie Nov 16, 2017 •

edited

Loading

aabadie Nov 16, 2017 •

edited

Loading