cpu/atmega_common: adapt to new i2c api by mali · Pull Request #9252 · RIOT-OS/RIOT

mali · 2018-05-30T19:40:30Z

Contribution description

Rework atmega i2c to match new i2c interface.
Only i2c_read_bytes and i2c_write_bytes are implemented.

WIP : Comments welcome !

Issues/PRs references

ZetaR60 · 2018-06-06T23:44:19Z

@mali I have gone over this PR and #9165 several times, but I have been unable to track down the problem I have been having in #9165. The slave does not ack on the bus with the new code, but I have confirmed that the old code works fine. The two code sets don't have a lot of differences in behavior prior to the failure, so it is probably just something I have overlooked. Could you take a look and see if you spot anything that could be causing it?

If anyone has any other I2C devices that have been ported and can be tested with this PR it would be much appreciated!

aabadie · 2018-06-07T06:49:29Z

cpu/atmega_common/periph/i2c.c

        /* receive data byte */
-        my_data[i] = TWDR;
-        DEBUG("Byte %i received\n", i+1);
+        my_data[0] = TWDR;


This looks like an introduced bug.

aabadie · 2018-06-07T06:50:18Z

cpu/atmega_common/periph/i2c.c

        /* Load DATA into TWDR Register.
         * Clear TWINT bit in TWCR to start transmission of data */
-        TWDR = data[i];
+        TWDR = data[0];


dylad

The I2C API has evolved a bit lately. We decide to use errno return code so some basic changes are needed.

dylad · 2018-06-08T08:01:50Z

cpu/atmega_common/periph/i2c.c

@@ -57,18 +64,24 @@ int i2c_init_master(i2c_t dev, i2c_speed_t speed)

    /* check if the line is valid */
    if (dev >= I2C_NUMOF) {


prefer the use of assert(dev < I2C_NUMOF) instead of if

dylad · 2018-06-08T08:03:08Z

cpu/atmega_common/periph/i2c.c

-int i2c_write_byte(i2c_t dev, uint8_t address, uint8_t data)
-{
-    return i2c_write_bytes(dev, address, &data, 1);
+    return I2C_ACK;


I2C_ACK doesn't exist anymore as we switch to errno return code for the whole API. You can replace I2C_ACK by 0

dylad · 2018-06-08T08:04:19Z

cpu/atmega_common/periph/i2c.c

-
-    assert(length > 0);
-
+    assert (len > 0);


must be switch to :

if(data == NULL || len == 0) { return -EINVAL; }

dylad · 2018-06-08T08:06:13Z

cpu/atmega_common/periph/i2c.c

    /* send out data bytes */
-    bytes = _write(data, length);
+    if(!_write(data, len))
+        return I2C_DATA_NACK;


must be switch to return -EIO;

dylad · 2018-06-08T08:06:36Z

cpu/atmega_common/periph/i2c.c

+        _stop();

-    return bytes;
+    return I2C_ACK;


must be switch to return 0;

dylad · 2018-06-08T08:07:17Z

cpu/atmega_common/periph/i2c.c

-void i2c_poweron(i2c_t dev)
+static void i2c_poweron(i2c_t dev)
 {
    assert(dev < I2C_NUMOF);


please unify the use of assert for each dev test against I2C_NUMOF

dylad · 2018-06-08T08:08:19Z

cpu/atmega_common/periph/i2c.c

+        } 
    }
-
    return length;


I2C API functions does not return the length anymore. This should be replaced by 0

dylad · 2018-06-13T08:22:06Z

@mali Any progress on this ?

mali · 2018-06-15T11:50:17Z

@dylad I'm wating for an i2c shield I've ordered. (the same that aabadie had lent me during the Hackaton).
It should arrive soon, i hope, so i'll be able to test for real.

MrKevinWeiss · 2018-06-28T13:55:57Z

@mali Any update? I can test here.

mali · 2018-07-02T19:20:07Z

@MrKevinWeiss , please do !

aabadie · 2018-07-03T09:08:31Z

@mali, I allowed myself to update this PR with a few things:

use assert as requested by @dylad
fix a few issues related to coding conventions
improve debug messages
adapt I2C bus speed switch in device initialization
adapt atmega based board configuration

mali · 2018-07-03T09:37:06Z

@aabadie , no problem, you're welcome !
I know time is ticking and I do not have as much as I would have liked

Aang-3 · 2018-07-03T11:07:17Z

We have just tested it on an Arduino Mega 2560 board using the driver_hts221' test. It seems to work fine.

aabadie · 2018-07-03T11:11:45Z

Thanks for testing @Aang-3. Just in case, I squashed the branch. @dylad, are you happy with the changes ?

dylad

@aabadie I'm fime with the current state, I have some idea in the future but this is out-of-scope.
ACK
Just wait a bit so others can test if they want just to be sure.

aabadie · 2018-07-03T11:41:47Z

Please wait before merging. With @Aang-3, we realized that this PR is broken in the current state.

aabadie · 2018-07-03T11:52:54Z

@dylad, I fixed the driver (plus a couple of other things), so it can be merged now.

MrKevinWeiss · 2018-07-03T12:59:15Z

I am having some issues testing... I need some time to figure it out.

MrKevinWeiss

It seems like the flags are not functioning, this made write reg fail the test. I believe if you have ~(0x00 & 0x04) == (0xFF) == true but if you have ~(0x04 & 0x04) == 0xFB == true. You want !(0x00 & 0x04) == true and !(0x04 & 0x04) == false

MrKevinWeiss · 2018-07-04T07:10:24Z

cpu/atmega_common/periph/i2c.c

    /* send start condition and slave address */
-    if (_start(address, I2C_FLAG_READ) != 0) {
-        return 0;
+    if (~(flags & I2C_NOSTART)) {


I think you mean ! not ~

MrKevinWeiss · 2018-07-04T07:10:58Z

cpu/atmega_common/periph/i2c.c

-
-    /* send register address and wait for complete transfer to be finished*/
-    if (_write(&reg, 1) != 1) {
+    if (~(flags & I2C_NOSTOP)) {


Same here, ! not ~

MrKevinWeiss · 2018-07-04T07:11:25Z

cpu/atmega_common/periph/i2c.c

    /* start transmission and send slave address */
-    if (_start(address, I2C_FLAG_WRITE) != 0) {
-        return 0;
+    if (~(flags & I2C_NOSTART)) {


Almost there, ! not ~

MrKevinWeiss · 2018-07-04T07:11:54Z

cpu/atmega_common/periph/i2c.c

-
-    return bytes;
-}
+    if (~(flags & I2C_NOSTOP)) {


finally, ! not ~

aabadie · 2018-07-04T08:10:45Z

It seems like the flags are not functioning, this made write reg fail the test

Good catch! And to move fast here, I pushed a fixup commit that addresses your comments. Please test again @MrKevinWeiss !

MrKevinWeiss · 2018-07-04T08:28:52Z

ACK, passing all non-critical tests!

Rework atmega i2c to match new i2c interface. Only i2c_read_bytes and i2c_write_bytes are implemented.

aabadie · 2018-07-04T08:40:20Z

Squashed. We can merge if Travis doesn't report know issues.

mali · 2018-07-04T08:45:12Z

👍

cpu/atmega_common: adapt to new i2c api

mali mentioned this pull request May 30, 2018

drivers/ads101x: update I2C API #9165

Merged

dylad added Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels May 30, 2018

haukepetersen mentioned this pull request Jun 1, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

aabadie reviewed Jun 7, 2018

View reviewed changes

dylad requested changes Jun 8, 2018

View reviewed changes

aabadie force-pushed the atmega_new_i2c_if branch from 5543aaf to fe84e0e Compare July 3, 2018 09:05

aabadie force-pushed the atmega_new_i2c_if branch from fe84e0e to 09da63d Compare July 3, 2018 09:18

aabadie force-pushed the atmega_new_i2c_if branch from 09da63d to 07e545b Compare July 3, 2018 11:04

aabadie force-pushed the atmega_new_i2c_if branch from 07e545b to 7ec8623 Compare July 3, 2018 11:18

dylad approved these changes Jul 3, 2018

View reviewed changes

aabadie force-pushed the atmega_new_i2c_if branch from 7ec8623 to ca0950f Compare July 3, 2018 11:51

aabadie force-pushed the atmega_new_i2c_if branch from ca0950f to d797048 Compare July 3, 2018 11:58

MrKevinWeiss requested changes Jul 4, 2018

View reviewed changes

MrKevinWeiss approved these changes Jul 4, 2018

View reviewed changes

mali and others added 2 commits July 4, 2018 10:38

cpu/atmega_common: adapt to new i2c api

e7f691c

Rework atmega i2c to match new i2c interface. Only i2c_read_bytes and i2c_write_bytes are implemented.

boards: adapt atmega boards with new i2c API

bc83ad1

aabadie force-pushed the atmega_new_i2c_if branch from 3f96610 to bc83ad1 Compare July 4, 2018 08:39

aabadie merged commit c4bf16e into RIOT-OS:new_i2c_if Jul 4, 2018

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

Merge pull request RIOT-OS#9252 from mali/atmega_new_i2c_if

f6ac37a

cpu/atmega_common: adapt to new i2c api

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

Merge pull request RIOT-OS#9252 from mali/atmega_new_i2c_if

94eb386

cpu/atmega_common: adapt to new i2c api

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

Merge pull request #9252 from mali/atmega_new_i2c_if

c79c516

cpu/atmega_common: adapt to new i2c api

		@@ -57,18 +64,24 @@ int i2c_init_master(i2c_t dev, i2c_speed_t speed)

		/* check if the line is valid */
		if (dev >= I2C_NUMOF) {

Conversation

mali commented May 30, 2018 • edited by aabadie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

ZetaR60 commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

dylad commented Jun 13, 2018

Uh oh!

mali commented Jun 15, 2018

Uh oh!

MrKevinWeiss commented Jun 28, 2018

Uh oh!

mali commented Jul 2, 2018

Uh oh!

aabadie commented Jul 3, 2018

Uh oh!

mali commented Jul 3, 2018

Uh oh!

Aang-3 commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Jul 3, 2018

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jul 3, 2018

Uh oh!

aabadie commented Jul 3, 2018

Uh oh!

MrKevinWeiss commented Jul 3, 2018

Uh oh!

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

aabadie commented Jul 4, 2018

Uh oh!

MrKevinWeiss commented Jul 4, 2018

Uh oh!

aabadie commented Jul 4, 2018

Uh oh!

mali commented Jul 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mali commented May 30, 2018 •

edited by aabadie

Loading

Aang-3 commented Jul 3, 2018 •

edited

Loading