cpu/lpc2387: implement periph/i2c #13037

benpicco · 2020-01-07T00:15:11Z

Contribution description

This adds a driver for the I2C master interface of the lpc23xx family of SoCs.

The driver operates in interrupt mode, for every change of the internal state machine an interrupt is generated. The data sheet lays out the required operations for each state change.

It doesn't seem to be possible to stop the state machine advancing without sending a stop condition, so I couldn't use the generic implementations of i2c_read_regs()/i2c_write_regs() but had to implement them in the driver using two transmit buffers. (One for the register address, one for the payload).

Testing procedure

Unfortunately msba2 doesn't have any i2c chips connected on board.
Initially I thought sht11 was i2c, but in fact it's own bit-banging protocol.

The only i2c capable pins that are available as headers are on JP3:

SDA: P0.10
SCL: P0.11

A basic test would be to add USEMODULE += i2c_scan to examples/default.

2020-01-07 00:52:54,989 #  i2c_scan 0
2020-01-07 00:52:54,990 # Scanning I2C device 0...
2020-01-07 00:52:54,992 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2020-01-07 00:52:54,993 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2020-01-07 00:52:54,994 # 0x00 R R R R R R R R R R R R R R - -
2020-01-07 00:52:54,995 # 0x10 - - - - - - - - - - - - - - - -
2020-01-07 00:52:54,996 # 0x20 - - - - - - - - - - - - - - - -
2020-01-07 00:52:54,997 # 0x30 - - - - - - - - - - - - - - - -
2020-01-07 00:52:54,998 # 0x40 - - - - - - - - - - - - - - - -
2020-01-07 00:52:54,999 # 0x50 - - - - - - - - - - - - - - - -
2020-01-07 00:52:55,000 # 0x60 - - - - - - - - - - - - - - - -
2020-01-07 00:52:55,001 # 0x70 - - - - - - - - R R R R R R R R

That's still boring, so let's hook something up to it.

I used the mcb2388 with a Tiny RTC module (DS1307, AT24C32 - #11929) as well as a GY-85 9-DOF (ADXL345, ITG3200 - #10092, HMC5883 - #10083)

The chips show up in i2c_scan, I've successfully ran

tests/driver_at24cxxx
tests/driver_adxl345
tests/driver_itg320x

Issues/PRs references

replacement for the driver removed in #6521
addresses #4693

maribu

See inline comments

boards/msba2/include/periph_conf.h

cpu/lpc2387/include/cpu.h

cpu/lpc2387/periph/i2c.c

maribu

All comments resolved. However, I guess the compiler will not be able to deduce that in _next_buffer() the value of the local variable uint8_t buf_cur is always 0. I think you can safe some ROM if you just set it to 0 rather then reading it from ctx[dev].buf_cur.

Maybe you should also call the function _second_buffer() or _switch_to_snd_buf(), so that it is clear that this function has the precondition that ctx[dev].buf_cur == 0.

cpu/lpc2387/periph/i2c.c

maribu · 2020-02-03T08:54:59Z

cpu/lpc2387/periph/i2c.c

+static struct i2c_ctx {
+    mutex_t lock;
+    mutex_t tx_done;
+    uint8_t *buf[TRX_BUFS_MAX];
+    uint8_t *buf_end[TRX_BUFS_MAX];
+    uint8_t *cur;
+    uint8_t *end;
+    int res;
+    uint8_t addr[TRX_BUFS_MAX];
+    uint8_t buf_num;
+    uint8_t buf_cur;
+} ctx[I2C_NUMOF];


How about

static struct i2c_ctx { /* ..., same as above */ } ctx[I2C_NUMOF] = { { .lock = MUTEX_INIT, .tx_done = MUTEX_INIT }, { .lock = MUTEX_INIT, .tx_done = MUTEX_INIT } };

That would get rid of the call to mutex_init(). If you take the suggestion, please squash that right in. Otherwise, just squash. I'll do one round of testing and then finally this should get in.

Sorry for the long stall. I have a lot to do recently...

This needed some additional #ifdefs that made it less pretty, but still saves about 200 bytes of ROM.

Sounds like a good trade off :-) Please squash!

Did you drop this by accident, or do you favor getting rid of the #ifdefs over the saved ROM size?

I think I simply forgot about it - added it now.
With USEMODULE=i2c_scan make -C examples/default the difference is only 48 bytes, but it's something :)

And it makes i2c_init() simpler, which should ease adaption to #13421

maribu · 2020-02-04T09:50:19Z

OK, I tested tests/driver_ina2xx with the MSB-A2 and nothing connected. The expected result is that initialization fails, however, the initialization blocks and never returns:

2020-02-04 10:49:08,340 # main(): This is RIOT! (Version: 2020.04-devel-244-g01a49-ben)
2020-02-04 10:49:08,342 # INA2XX sensor driver test application
2020-02-04 10:49:08,342 # 
2020-02-04 10:49:08,345 # Initializing INA2XX sensor at I2C_0, address 0x00000040

benpicco · 2020-02-06T23:03:17Z

Rebased to include #12898

The problem was that no interrupt is generated if nothing is connected to the bus.
I added a xtimer_mutex_lock_timeout() in b292c68 that solves the issue for me.

maribu · 2020-02-10T09:37:44Z

Can you drop the last fix? It turns out that this is not needed when the bug in the ina2xx driver is fixed. (*Embarrassed staring on the group*)

benpicco · 2020-02-10T23:17:34Z

Alright!
There is actually a case for the timeout: I2C0 pins are always open-drain and i2c_scan will get stuck there if nothing is connected. (no interrupt is generated)
But I'm not sure if the added overhead is worth this, it's not required for the other i2c ports and not if something is connected to the bus.

maribu · 2020-02-11T14:27:55Z

But I'm not sure if the added overhead is worth this, it's not required for the other i2c ports and not if something is connected to the bus.

Then I'd say no :-) Can you rebase and squash? I'll give it one last test to be sure.

benpicco · 2020-02-17T10:11:43Z

Rebase and squashed

benpicco · 2020-02-25T22:18:37Z

@fabian18 want to give this a try too? 😃

fabian18 · 2020-02-26T09:44:49Z

I tested tests/driver_sht3x with sht31 on msba2 and it works like a charm.

2020-02-26 10:37:33,640 # 
2020-02-26 10:37:33,642 # +--------Starting Measurements--------+
2020-02-26 10:37:33,655 # Temperature [°C]: 21.55
2020-02-26 10:37:33,658 # Relative Humidity [%]: 48.47
2020-02-26 10:37:33,660 # +-------------------------------------+
2020-02-26 10:37:34,663 # Temperature [°C]: 21.58
2020-02-26 10:37:34,664 # Relative Humidity [%]: 48.39
2020-02-26 10:37:34,666 # +-------------------------------------+
2020-02-26 10:37:35,654 # Temperature [°C]: 21.58
2020-02-26 10:37:35,669 # Relative Humidity [%]: 48.35
2020-02-26 10:37:35,670 # +-------------------------------------+
2020-02-26 10:37:36,681 # Temperature [°C]: 21.61
2020-02-26 10:37:36,682 # Relative Humidity [%]: 48.31
2020-02-26 10:37:36,684 # +-------------------------------------+

benpicco · 2020-02-26T20:07:35Z

rebased to fix missing header in at24mac.

cpu/lpc2387/periph/i2c.c

gschorcht · 2020-02-26T22:41:03Z

Please squash.

The lpc23xx MCU has up to three I2C interfaces. This adds a driver for it. The peripheral works in interrupt mode, each change of the state machine will generate an interrupt. The response to the states are laid out in the data sheet. This replaces the old driver that was removed in c560e28

I2C is not used by any chip on the board, but I2C2 is availiable on the JP3 connector (shared with UART2). - SDA: P0.10 - SCL: P0.11

The pins for I2C0 and I2C1 are available on the board, nothing is wired up to them.

We need that header file for the ARRAY_SIZE() macro.

benpicco · 2020-02-26T23:17:21Z

squashed.

gschorcht

ACK

benpicco · 2020-02-27T09:09:51Z

Thank you all for reviewing, testing and merging 😄

benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Jan 7, 2020

benpicco requested review from haukepetersen and maribu January 7, 2020 00:15

maribu reviewed Jan 13, 2020

View reviewed changes

benpicco force-pushed the lpc23xx_i2c branch from 7dbc7fd to 67f167a Compare January 14, 2020 22:23

maribu reviewed Jan 23, 2020

View reviewed changes

cpu/lpc2387/periph/i2c.c Outdated Show resolved Hide resolved

cpu/lpc2387/periph/i2c.c Show resolved Hide resolved

cpu/lpc2387/periph/i2c.c Show resolved Hide resolved

maribu reviewed Feb 3, 2020

View reviewed changes

benpicco force-pushed the lpc23xx_i2c branch from 86aae28 to 01a495d Compare February 3, 2020 15:57

benpicco force-pushed the lpc23xx_i2c branch from 01a495d to b292c68 Compare February 6, 2020 23:01

maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 10, 2020

maribu mentioned this pull request Feb 10, 2020

drivers/ina2xx: Fixed missing i2c_acquire/release #13324

Merged

benpicco force-pushed the lpc23xx_i2c branch from b292c68 to 8335474 Compare February 10, 2020 23:13

benpicco force-pushed the lpc23xx_i2c branch from 8335474 to 7e75689 Compare February 11, 2020 14:36

benpicco requested review from aabadie, fjmolinas and MrKevinWeiss as code owners February 26, 2020 19:13

benpicco force-pushed the lpc23xx_i2c branch from 8dbf959 to 44f18d0 Compare February 26, 2020 20:07

gschorcht requested changes Feb 26, 2020

View reviewed changes

cpu/lpc2387/periph/i2c.c Outdated Show resolved Hide resolved

benpicco added 4 commits February 26, 2020 23:41

boards/msba2: hook up i2c

4d5d3eb

I2C is not used by any chip on the board, but I2C2 is availiable on the JP3 connector (shared with UART2). - SDA: P0.10 - SCL: P0.11

boards/mcb2388: add I2C config

9d1a2b5

The pins for I2C0 and I2C1 are available on the board, nothing is wired up to them.

drivers/at24mac: include kernel_defines.h

00131bb

We need that header file for the ARRAY_SIZE() macro.

benpicco force-pushed the lpc23xx_i2c branch from 1287075 to 00131bb Compare February 26, 2020 22:42

gschorcht approved these changes Feb 27, 2020

View reviewed changes

benpicco merged commit 50a5845 into RIOT-OS:master Feb 27, 2020

benpicco deleted the lpc23xx_i2c branch February 27, 2020 08:58

leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/lpc2387: implement periph/i2c #13037

cpu/lpc2387: implement periph/i2c #13037

benpicco commented Jan 7, 2020 •

edited

Loading

maribu left a comment

maribu left a comment

maribu Feb 3, 2020

benpicco Feb 3, 2020 •

edited

Loading

maribu Feb 3, 2020

maribu Feb 26, 2020

benpicco Feb 26, 2020

maribu commented Feb 4, 2020

benpicco commented Feb 6, 2020

maribu commented Feb 10, 2020 •

edited

Loading

benpicco commented Feb 10, 2020

maribu commented Feb 11, 2020

benpicco commented Feb 17, 2020

benpicco commented Feb 25, 2020

fabian18 commented Feb 26, 2020

benpicco commented Feb 26, 2020

gschorcht commented Feb 26, 2020

benpicco commented Feb 26, 2020

gschorcht left a comment

benpicco commented Feb 27, 2020

cpu/lpc2387: implement periph/i2c #13037

cpu/lpc2387: implement periph/i2c #13037

Conversation

benpicco commented Jan 7, 2020 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

maribu left a comment

Choose a reason for hiding this comment

maribu left a comment

Choose a reason for hiding this comment

maribu Feb 3, 2020

Choose a reason for hiding this comment

benpicco Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

maribu Feb 3, 2020

Choose a reason for hiding this comment

maribu Feb 26, 2020

Choose a reason for hiding this comment

benpicco Feb 26, 2020

Choose a reason for hiding this comment

maribu commented Feb 4, 2020

benpicco commented Feb 6, 2020

maribu commented Feb 10, 2020 • edited Loading

benpicco commented Feb 10, 2020

maribu commented Feb 11, 2020

benpicco commented Feb 17, 2020

benpicco commented Feb 25, 2020

fabian18 commented Feb 26, 2020

benpicco commented Feb 26, 2020

gschorcht commented Feb 26, 2020

benpicco commented Feb 26, 2020

gschorcht left a comment

Choose a reason for hiding this comment

benpicco commented Feb 27, 2020

benpicco commented Jan 7, 2020 •

edited

Loading

benpicco Feb 3, 2020 •

edited

Loading

maribu commented Feb 10, 2020 •

edited

Loading