drivers/ads101x: update I2C API #9165

ZetaR60 · 2018-05-23T03:14:19Z

This is a WIP update to ads101x's I2C API usage. Currently untested due to lack of API update to atmega_common. See also: #6577 and #9162

ZetaR60 · 2018-05-23T03:20:12Z

I may need to rebase this on new_i2c_if

dylad · 2018-05-23T07:28:22Z

Hi @ZetaR60
Thanks for your PR. Unfortunately this PR cannot go to the master branch. It belongs to the new_i2c_if branch.
Once all the job will be done in new_i2c_if branch, the whole branch will be merge to master.
We're doing this way to prevent the CI from complaining.

ZetaR60 · 2018-05-23T07:43:18Z

@dylad I just had to rebase this on new_i2c_if, because switching it over brought a bunch of stuff from master with it. Should be good now.

dylad · 2018-05-23T13:23:45Z

@ZetaR60 After some offline discussions, we update the API a bit.
Now the API is merged so this should not move anymore. (see #9162)

ZetaR60 · 2018-05-23T21:37:30Z

Should be up to date again.

dylad

Changes look good but I'm not convinced by the NOSTOP flag. I don't think they are required.

dylad · 2018-05-24T07:29:11Z

drivers/ads101x/ads101x.c

-    if (i2c_init_master(i2c, I2C_SPEED) < 0) {
+
+    /* Register read test */
+    if (i2c_read_regs(i2c, addr, ADS101X_CONF_ADDR, &regs, 2, I2C_NOSTOP)


I don't see any reason here to have a NOSTOP flag. (same at other place in the code)
It's not forbidden to read some regs send a STOP and then send data into another register.

dylad · 2018-05-24T07:32:29Z

drivers/ads101x/ads101x.c

@@ -127,22 +134,21 @@ int ads101x_read_raw(const ads101x_t *dev, int16_t *raw)
    i2c_acquire(I2C);

    /* Read control register */
-    i2c_read_regs(I2C, ADDR, ADS101X_CONF_ADDR, &regs, 2);


NOSTOP required ?

why should it be required?

dylad · 2018-05-24T07:32:36Z

drivers/ads101x/ads101x.c

@@ -103,7 +110,7 @@ int ads101x_set_mux_gain(const ads101x_t *dev, uint8_t mux_gain)

    i2c_acquire(I2C);

-    i2c_read_regs(I2C, ADDR, ADS101X_CONF_ADDR, &regs, 2);


NOSTOP required ?

dylad · 2018-05-24T07:32:40Z

drivers/ads101x/ads101x.c

-    i2c_write_regs(i2c, addr, ADS101X_CONF_ADDR, &regs, 2);
-    i2c_read_regs(i2c, addr, ADS101X_CONF_ADDR, &regs, 2);
+    /* Register write test */
+    if (i2c_write_regs(i2c, addr, ADS101X_CONF_ADDR, &regs, 2, I2C_NOSTOP)


same here, is the NOSTOP really useful ?

ZetaR60 · 2018-05-24T20:56:03Z

NOSTOPs removed.

mali · 2018-05-25T14:27:25Z

@ZetaR60 , I'll push an atmega_common api update (WIP) soon. So you'll be able to test.

mali · 2018-05-29T19:25:51Z

@ZetaR60 you can try with this branch https://github.com/mali/RIOT/tree/atmega_new_i2c_if
It's based on the last API changes. It builds but last changes are untested as I'm waiting for an i2c sensor. Keep me informed !

ZetaR60 · 2018-05-30T03:54:38Z

@mali I am not seeing the changes to periph/i2c.c: https://github.com/mali/RIOT/blob/atmega_new_i2c_if/cpu/atmega_common/periph/i2c.c

mali · 2018-05-30T07:20:37Z

Oups .. sorry, it seems I've done something wrong :-/

mali · 2018-05-30T19:43:56Z

It's repaired ! see also #9252

ZetaR60 · 2018-05-31T03:14:18Z

I tried tests/driver_ads101x with #9252 and got this error (debug enabled for ads101x and gpio.c):

[liveuser@localhost-live driver_ads101x]$ make BOARD=mega-xplained term
/home/liveuser/Desktop/RIOT_ads101x_i2c_api_merge/dist/tools/pyterm/pyterm -p "/dev/ttyUSB0" -b "9600"
No handlers could be found for logger "root"
2018-05-30 23:06:06,059 - INFO # Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
2018-05-30 23:06:07,891 - INFO # 

2018-05-30 23:06:07,987 - INFO # main(): This is RIOT! (Version: 2018.07-devel-274-gf8bc-localhost-live-RIOT_ads101x_i2c_api)
2018-05-30 23:06:08,035 - INFO # ADS101X analog to digital driver test application
2018-05-30 23:06:08,035 - INFO # 
2018-05-30 23:06:08,131 - INFO # Initializing ADS101x analog to digital at I2C_DEV(0)... START condition transmitted
2018-05-30 23:06:08,147 - INFO # I2C Status is: START
2018-05-30 23:06:08,179 - INFO # ADDRESS and FLAG transmitted
2018-05-30 23:06:08,211 - INFO # NACK has been received for ADDRESS
2018-05-30 23:06:08,243 - INFO # STOP condition transmitted
2018-05-30 23:06:08,291 - INFO # [ads101x] init - error: unable to read reg 1
2018-05-30 23:06:08,291 - INFO # [Failed]
/exit
2018-05-30 23:06:11,546 - INFO # Exiting Pyterm
[liveuser@localhost-live driver_ads101x]$

I will continue investigation tomorrow.

ZetaR60 · 2018-06-02T20:06:08Z

The specific NACK received is 0x20. I have not been able to track down the problem yet. The odd thing is that trunk has almost the same execution path as the new code, but trunk works fine and the new code is failing. The main difference is moving i2c_init, but moving this back to ads101x.c does not fix the problem.

smlng · 2018-06-28T16:42:43Z

drivers/ads101x/ads101x.c

-    if (i2c_init_master(i2c, I2C_SPEED) < 0) {
+
+    /* Register read test */
+    if (i2c_read_regs(i2c, addr, ADS101X_CONF_ADDR, &regs, 2, 0x0)


I mostly saw a simple 0 for the flags in other places, IMHO we should stick to one style.

I use 0 for defaults that are numerical, 0x0 for bit fields, and NULL for pointers. This is consistent with my prior work. IMO asking for developers to conform to a community standard for such detailed stylistic choices is burdensome without significant benefit.

agreed, maybe the (most) correct way would be to use the I2C_FLAG_NONE to explicitly state that no flag should be set or used here. But we could leave that to a cleanup PR over all drivers and stuff later on.

no valid

smlng · 2018-07-04T08:52:37Z

drivers/ads101x/ads101x_saul.c

@@ -1,6 +1,6 @@
 /*
 * Copyright (C) 2017 OTA keys S.A.
- *               2018 Matthew Blue <[email protected]>
+ *               2018 Acutam Automation, LLC


unrelated, and unclear why? Here and other files

I have transferred my copyrights on RIOT code from myself (Matthew Blue) to my company (Acutam Automation, LLC). I figured it was a minor enough change that it did not require a separate PR, or detailed explanation.

ah okay, sorry was confused by GitHub username vs author name, never mind.

dylad · 2018-07-04T14:32:04Z

@ZetaR60 atmega I2C driver has been merge, do you think you can re-test this PR ?

ZetaR60 · 2018-07-04T16:42:13Z

Rebased to pick up ATmega changes.

ZetaR60 · 2018-07-04T17:10:18Z

Everything looks okay. Let me know when to squash.

user@user ~/Desktop/RIOT_ads101x_i2c_api/tests/driver_ads101x $ make BOARD=mega-xplained term
/home/user/Desktop/RIOT_ads101x_i2c_api/dist/tools/pyterm/pyterm -p "/dev/ttyUSB0" -b "9600"
2018-07-04 13:07:06,775 - INFO # Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
2018-07-04 13:07:08,696 - INFO #  

2018-07-04 13:07:08,776 - INFO # main(): This is RIOT! (Version: 2018.07-devel-409-gd2631-user-RIOT_ads101x_i2c_api)
2018-07-04 13:07:08,840 - INFO # ADS101X analog to digital driver test application
2018-07-04 13:07:08,840 - INFO # 
2018-07-04 13:07:08,904 - INFO # Initializing ADS101x analog to digital at I2C_DEV(0)... [OK]
2018-07-04 13:07:08,952 - INFO # Initializing ADS101x alert at I2C_DEV(0)... [OK]
2018-07-04 13:07:08,984 - INFO # Enabling alert interrupt: [OK]
2018-07-04 13:07:09,064 - INFO # Raw analog read. CH0: 5456 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,192 - INFO # Raw analog read. CH0: 5584 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,320 - INFO # Raw analog read. CH0: 5488 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,432 - INFO # Raw analog read. CH0: 5536 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,560 - INFO # Raw analog read. CH0: 5616 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,672 - INFO # Raw analog read. CH0: 5536 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,800 - INFO # Raw analog read. CH0: 5472 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:09,928 - INFO # Raw analog read. CH0: 5440 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,040 - INFO # Raw analog read. CH0: 5616 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,167 - INFO # Raw analog read. CH0: 5520 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,279 - INFO # Raw analog read. CH0: 5520 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,407 - INFO # Raw analog read. CH0: 5536 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,535 - INFO # Raw analog read. CH0: 5584 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,647 - INFO # Raw analog read. CH0: 5632 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,775 - INFO # Raw analog read. CH0: 5488 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:10,887 - INFO # Raw analog read. CH0: 5600 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,034 - INFO # Raw analog read. CH0: 5520 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,143 - INFO # Raw analog read. CH0: 5568 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,255 - INFO # Raw analog read. CH0: 5488 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,383 - INFO # Raw analog read. CH0: 5504 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,495 - INFO # Raw analog read. CH0: 5616 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,623 - INFO # Raw analog read. CH0: 5488 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,751 - INFO # Raw analog read. CH0: 5632 CH1: 18800 CH2: 32752 CH3: 32752
2018-07-04 13:07:11,863 - INFO # Raw analog read. CH0: 5472 CH1: 18800 CH2: 32752 CH3: 32752

dylad

ACK
Please squash

ZetaR60 · 2018-07-05T16:25:36Z

Squashed. Thanks for the help!

dylad · 2018-07-05T16:50:44Z

Here we go !

drivers/ads101x: update I2C API

ZetaR60 mentioned this pull request May 23, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

ZetaR60 changed the base branch from master to new_i2c_if May 23, 2018 03:18

ZetaR60 changed the base branch from new_i2c_if to master May 23, 2018 03:19

ZetaR60 mentioned this pull request May 23, 2018

ACU-AMNx board and MVP checklist #9143

Closed

24 tasks

ZetaR60 force-pushed the RIOT_ads101x_i2c_api branch from f2894ab to 27b43dd Compare May 23, 2018 07:40

ZetaR60 changed the base branch from master to new_i2c_if May 23, 2018 07:40

dylad added the TF: I2C Marks issues and PRs related to the work of the I²C rework task force label May 23, 2018

dylad reviewed May 24, 2018

View reviewed changes

ZetaR60 force-pushed the RIOT_ads101x_i2c_api branch from b8dc0a3 to c27f689 Compare May 31, 2018 02:39

aabadie added the Area: drivers Area: Device drivers label Jun 3, 2018

ZetaR60 mentioned this pull request Jun 6, 2018

cpu/atmega_common: adapt to new i2c api #9252

Merged

smlng previously requested changes Jun 28, 2018

View reviewed changes

smlng reviewed Jul 4, 2018

View reviewed changes

*/ads101x: copyright notice update

c6390c7

ZetaR60 force-pushed the RIOT_ads101x_i2c_api branch from 6a3dd3d to fa183d0 Compare July 4, 2018 16:41

dylad approved these changes Jul 5, 2018

View reviewed changes

drivers/ads101x: update I2C API

8ab0579

ZetaR60 force-pushed the RIOT_ads101x_i2c_api branch from d2631ed to 8ab0579 Compare July 5, 2018 16:21

dylad merged commit 43c4547 into RIOT-OS:new_i2c_if Jul 5, 2018

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9165 from ZetaR60/RIOT_ads101x_i2c_api

af73769

drivers/ads101x: update I2C API

dylad added a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9165 from ZetaR60/RIOT_ads101x_i2c_api

f1064e1

drivers/ads101x: update I2C API

dylad added a commit that referenced this pull request Jul 11, 2018

Merge pull request #9165 from ZetaR60/RIOT_ads101x_i2c_api

e6debb5

drivers/ads101x: update I2C API

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/ads101x: update I2C API #9165

drivers/ads101x: update I2C API #9165

ZetaR60 commented May 23, 2018

ZetaR60 commented May 23, 2018

dylad commented May 23, 2018

ZetaR60 commented May 23, 2018

dylad commented May 23, 2018

ZetaR60 commented May 23, 2018

dylad left a comment

dylad May 24, 2018

dylad May 24, 2018

smlng Jun 28, 2018

dylad May 24, 2018

dylad May 24, 2018

ZetaR60 commented May 24, 2018

mali commented May 25, 2018

mali commented May 29, 2018

ZetaR60 commented May 30, 2018

mali commented May 30, 2018

mali commented May 30, 2018

ZetaR60 commented May 31, 2018

ZetaR60 commented Jun 2, 2018

smlng Jun 28, 2018

ZetaR60 Jun 28, 2018

smlng Jun 28, 2018

smlng Jul 4, 2018

ZetaR60 Jul 4, 2018

smlng Jul 5, 2018

dylad commented Jul 4, 2018

ZetaR60 commented Jul 4, 2018

ZetaR60 commented Jul 4, 2018

dylad left a comment

ZetaR60 commented Jul 5, 2018

dylad commented Jul 5, 2018

		@@ -103,7 +110,7 @@ int ads101x_set_mux_gain(const ads101x_t *dev, uint8_t mux_gain)

		i2c_acquire(I2C);

		i2c_read_regs(I2C, ADDR, ADS101X_CONF_ADDR, &regs, 2);

drivers/ads101x: update I2C API #9165

drivers/ads101x: update I2C API #9165

Conversation

ZetaR60 commented May 23, 2018

ZetaR60 commented May 23, 2018

dylad commented May 23, 2018

ZetaR60 commented May 23, 2018

dylad commented May 23, 2018

ZetaR60 commented May 23, 2018

dylad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZetaR60 commented May 24, 2018

mali commented May 25, 2018

mali commented May 29, 2018

ZetaR60 commented May 30, 2018

mali commented May 30, 2018

mali commented May 30, 2018

ZetaR60 commented May 31, 2018

ZetaR60 commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylad commented Jul 4, 2018

ZetaR60 commented Jul 4, 2018

ZetaR60 commented Jul 4, 2018

dylad left a comment

Choose a reason for hiding this comment

ZetaR60 commented Jul 5, 2018

dylad commented Jul 5, 2018