kinetis: Update i2c driver to new API by jnohlgard · Pull Request #9262 · RIOT-OS/RIOT

jnohlgard · 2018-06-01T05:22:50Z

Contribution description

Update the Kinetis I2C driver for the new API

Needs some more testing

Issues/PRs references

aabadie · 2018-06-01T07:14:25Z

cpu/kinetis/periph/i2c.c

+    1280, 1536, 1792, 2048, 2304, 2560, 3072, 3840, /* 0x38 ~ 0x3f */
 };

+static const i2c_conf_t i2c_config[I2C_NUMOF] = I2C_CONFIG;


Why not directly defining this array of structs in the board periph_conf (like stm32, sam0, etc) instead of using the I2C_CONFIG macro ?

I wanted to avoid unintentionally instancing this local array inside any other files, so I decided to put it as a define instead.

Ok but then it's not consistent with the other configuration arrays (UART, SPI, PWM, ADC, etc).

alright, will change this for now

jnohlgard · 2018-06-01T22:52:15Z

Cherry picked in #8933 #8814 #8930 to avoid rebase problems later

jnohlgard · 2018-06-04T11:30:43Z

Refactored to use IRQs when transferring the actual data bytes. This method lets the CPU perform other tasks while waiting for a slow I2C slave.

dylad

I have zero knowledge of this MCU but overall looks good. Just a few things could be change.
Were you able to test it with some devices ?

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

boards/teensy31/Makefile.features

-FEATURES_PROVIDED += periph_gpio
 FEATURES_PROVIDED += periph_pwm
+FEATURES_PROVIDED += periph_rtc
+FEATURES_PROVIDED += periph_rtt


Is this related ?

I had to pull in some upstream merged PRs to avoid problems rebasing later.

dylad · 2018-06-08T08:14:28Z

cpu/kinetis/periph/i2c.c

-        while (!(dev->S & I2C_S_IICIF_MASK));
-
-        dev->S = I2C_S_IICIF_MASK;
+    I2C_Type *i2c = i2c_config[dev].i2c;


Could you add a guard here please ?

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

Could you also return in case of unsupported features ?

len == 0 is not an error, it can be used to send a start condition or stop condition, depending on flags.
I don't think there are any unsupported flags here (other than REG16, which I think we can just ignore in the _bytes functions)

it can be used to send a start condition or stop condition, depending on flags

I never thought of it but is this really useful for the user ? I mean, we're suppose to read or write on the bus so why just sending a start out of the blue ?

The hih6130 is one weird device which uses empty writes to trigger measurements. The user sends a start condition for writing followed by a stop condition, so zero data bytes. Then when reading the device will return the most recent measurement.

The hih6130 is one weird device which uses empty writes to trigger measurements

Indeed, I'll look at this one. Is this even I2C/TWI compliant ?

Dunno, but it's a device that we have a driver for

jnohlgard · 2018-06-08T17:25:32Z

Were you able to test it with some devices ?

I have tested it with the fxos8700 driver with the updates in #9268

jnohlgard · 2018-06-08T17:27:09Z

It would be a good idea to test with the hih6130 as well because of the null write behaviour, but I don't remember where I put my breakout board for that sensor..

jnohlgard · 2018-06-08T17:27:47Z

Added some asserts for enabling interrupts and a check to ensure that data != NULL when len > 0

dylad · 2018-06-11T18:42:43Z

@gebart I tend to ACK this PR but I have no hardware to test it :(
Do you know if someone have the required hardware for testing ?

jnohlgard · 2018-06-11T18:53:58Z

I have Mulle and frdm boards, and I know that there are some kinetis boards among the FU Berlin people

dylad · 2018-06-11T19:36:50Z

I guess I'll resend an email on the devel mailing to require some help for testing

MichelRottleuthner · 2018-06-27T10:09:37Z

Just tested this with pba-d-01-kw2x and the already merged srf08 driver. Works fine

dylad

ACK.
Thanks for testing @MichelRottleuthner
Please squash so we can merge this one.

smlng

Tested with several sensor drivers on board pba-d-01-kw2x works like charm see #9455 and #9457. Please squash and then lets do this!

Uses timer freerunning counter mode (TFC) and updating an internal reference point instead of relying on RTT for reference time. The target accuracy has been greatly improved for all test cases in bench_periph_timer

jnohlgard · 2018-06-29T07:05:32Z

cpu/kinetis/periph/i2c.c

-#if I2C_3_EN
-    [I2C_3] = MUTEX_INIT
-#endif
+#define THREAD_FLAG_KINETIS_I2C (1u << 8)


Added this to avoid dependency on contested PR #9284. We can update the driver later if a consensus is reached regarding the reserved thread flags

jnohlgard

Changed the thread flag usage to avoid dependency on PR #9284. Immediately squashed after a search+replace of the thread flag constant in i2c.c.
Tested on frdm-kw41z with fxos8700 driver.

dylad · 2018-06-29T07:10:51Z

Just re-trigger Travis and we're good to go.

jnohlgard · 2018-06-29T07:14:07Z

cpu/kinetis/periph/i2c.c

+        ++dummy;
+        /* Wait until the ISR signals back */
+        TRACE("i2c: read C1=%02x S=%02x\n", (unsigned)i2c->C1, (unsigned)i2c->S);
+        thread_flags_t tflg = thread_flags_wait_any(THREAD_FLAG_KINETIS_I2C | THREAD_FLAG_TIMEOUT);


We are checking for the TIMEOUT thread flag, but there is not yet anything setting it in the driver. I left this thread flag check here to make it simple in the future to add a timeout, but I did not yet find a satisfying solution which does not add extra dependencies to the driver. This part of the code can still be used if a user wants to eliminate the possibility of a bus hang blocking the application, they can use xtimer to provide a timer which sets the timeout flag.

I'm fine with this, switching thread while waiting here is an excellent idea and IMHO we should do it for every MCU/drivers :)

dylad · 2018-06-29T07:43:29Z

Here we go ! thanks @gebart for your work !

kinetis: Update i2c driver to new API

aabadie reviewed Jun 1, 2018

View reviewed changes

haukepetersen mentioned this pull request Jun 1, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

jnohlgard force-pushed the pr/kinetis-i2c branch 4 times, most recently from 241b1ed to d244646 Compare June 1, 2018 22:48

jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 1, 2018

jnohlgard force-pushed the pr/kinetis-i2c branch from 9f34e8b to fd386c4 Compare June 4, 2018 10:57

jnohlgard mentioned this pull request Jun 4, 2018

core: Add THREAD_FLAG_IRQ_SERVICED #9284

Closed

dylad reviewed Jun 8, 2018

View reviewed changes

dylad approved these changes Jun 27, 2018

View reviewed changes

smlng approved these changes Jun 29, 2018

View reviewed changes

Joakim Nohlgård added 5 commits June 29, 2018 09:00

cpu/kinetis: Refactor LPTMR driver implementation

866576e

Uses timer freerunning counter mode (TFC) and updating an internal reference point instead of relying on RTT for reference time. The target accuracy has been greatly improved for all test cases in bench_periph_timer

mulle: Update LPTMR config

90af645

frdm-kw41z: Update LPTMR config

52072be

frdm-k22f: Update LPTMR config

27605dc

kinetis: Clean up PIT timer implementation

56ee627

Joakim Nohlgård added 10 commits June 29, 2018 09:00

kinetis: Move RTT config to periph_cpu.h

549101f

teensy31: Clean up Makefile.features

363056b

core: uncrustify thread_flags.h

c430083

kinetis: Reflow text in doc.txt

e280759

kinetis: Refactor I2C driver

7f6c6c3

frdm-k22f: Update I2C configuration

f346b04

frdm-k64f: Update I2C configuration

abb44dd

frdm-kw41z: Update I2C configuration

db2d443

mulle: Update I2C configuration

283f42b

pba-d-01-kw2x: Update I2C configuration

00703d8

jnohlgard force-pushed the pr/kinetis-i2c branch from 5b99c11 to 8be0cd4 Compare June 29, 2018 07:02

Joakim Nohlgård added 6 commits June 29, 2018 09:03

kinetis: Interrupt based I2C transfers

78e91a6

frdm-k22f: Update I2C ISR config

b05c86c

frdm-k64f: Update I2C ISR config

71a12fc

frdm-kw41z: Update I2C ISR config

95329df

mulle: Update I2C ISR config

1c147cb

pba-d-01-kw2x: Update I2C ISR config

df199b5

jnohlgard force-pushed the pr/kinetis-i2c branch from 8be0cd4 to df199b5 Compare June 29, 2018 07:03

jnohlgard commented Jun 29, 2018

View reviewed changes

dylad removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jun 29, 2018

jnohlgard commented Jun 29, 2018

View reviewed changes

dylad merged commit 2e06e99 into RIOT-OS:new_i2c_if Jun 29, 2018

jnohlgard deleted the pr/kinetis-i2c branch June 30, 2018 12:06

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

Merge pull request RIOT-OS#9262 from gebart/pr/kinetis-i2c

c1fa90c

kinetis: Update i2c driver to new API

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

Merge pull request RIOT-OS#9262 from gebart/pr/kinetis-i2c

5dde1b5

kinetis: Update i2c driver to new API

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

Merge pull request #9262 from gebart/pr/kinetis-i2c

001abaa

kinetis: Update i2c driver to new API

Conversation

jnohlgard commented Jun 1, 2018

Contribution description

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

jnohlgard commented Jun 1, 2018

Uh oh!

jnohlgard commented Jun 4, 2018

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!

dylad Jun 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jun 8, 2018

Uh oh!

jnohlgard commented Jun 8, 2018

Uh oh!

jnohlgard commented Jun 8, 2018

Uh oh!

dylad commented Jun 11, 2018

Uh oh!

jnohlgard commented Jun 11, 2018

Uh oh!

dylad commented Jun 11, 2018

Uh oh!

MichelRottleuthner commented Jun 27, 2018

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

dylad commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

aabadie Jun 1, 2018 •

edited

Loading

dylad Jun 8, 2018 •

edited

Loading