RFC HAL I2C: Reference implementations for K64F and STM32F0 #8682

ghost · 2018-11-08T17:01:47Z

Description

This PR contains reference implementations of the I2C HAL RFC changes for the FRDM-K64F and NUCLEO_F070RB boards.

Tasks

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[X] Breaking change

cmonr · 2018-11-08T22:45:32Z

@scartmell-arm Something about this PR's comit history and files changed does not look right. Could you double check?

cmonr · 2018-11-15T18:06:24Z

@mbed-os-maintainers @ARMmbed/mbed-os-hal Readt for review when able.

maciejbocianski

I have additional question to all HAL and DRIVERS API functions.
Whenever we passes device address it should be shifted left by 1 bit and filled by 1 or 0 (or LSB bit is ignored) depending of situation, it's valid for both 7bit and 10bit addressing?

targets/TARGET_STM/i2c_api.c

maciejbocianski · 2018-11-21T14:00:31Z

drivers/I2CSlave.cpp

 {
    int addr = (address & 0xFF) | 1;
-    i2c_slave_address(&_i2c, 0, addr, 0);
+


The line:
int addr = (address & 0xFF) | 1;
breaks 10bit addressing

cmonr · 2018-12-14T01:02:48Z

@scartmell-arm You have questions/comments waiting for your attention.

targets/TARGET_STM/i2c_api.c

LMESTM

Few comments or questions ...

targets/TARGET_STM/i2c_api.c

0xc0170 · 2019-01-16T12:29:27Z

@scartmell-arm Is it time to update this one and make sure reviewers find time to review?

ghost · 2019-01-16T12:46:11Z

@scartmell-arm Is it time to update this one and make sure reviewers find time to review?

There were a few outstanding concerns I was working through, I'll push a resolution to all of them soon.

hal/i2c_api.h

0xc0170 · 2019-02-13T13:42:17Z

@scartmell-arm Who should review , anyone from @ARMmbed/mbed-os-hal or @maciejbocianski is sufficient (still some comments not addressed).

How can we progress i2c feature branch?

cmonr · 2019-02-20T19:34:52Z

@scartmell-arm @ARMmbed/mbed-os-hal How can we progress this PR?

cmonr · 2019-02-26T05:42:21Z

@ARMmbed/mbed-os-hal What do we need to move this forward? Or is this still being worked on?
This will soon be closed due to lack of communication.

@bulislaw @donatieng

donatieng · 2019-03-04T14:02:34Z

@maciejbocianski could you have another look at this please?

donatieng

Missing implementation of async in the driver - @scartmell-arm @maciejbocianski would you be able to pick this up so that we get this merged soon?

donatieng · 2019-03-04T14:05:36Z

drivers/I2C.cpp

 #if DEVICE_I2C_ASYNCH

 int I2C::transfer(int address, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length, const event_callback_t &callback, int event, bool repeated)
 {


I think that shouldn't be empty!

maciejbocianski · 2019-03-07T09:14:33Z

targets/TARGET_STM/i2c_api.c

+    }
 }

 int i2c_byte_read(i2c_t *obj, int last)


@scartmell-arm
i2c_byte_read and i2c_byte_write should be removed since it was removed from API

maciejbocianski · 2019-03-27T13:50:37Z

After last fixes tests were successful for both reference implementations K64F and NUCLEO_F070RB

Following limitations were made:

10bit addressing was disabled for both targets
slave mode was disabled due to some implementation problems for k64f

CI i2c test (#9564)

| target                | platform_name | test suite         | test case                                  | passed | failed | result | elapsed_time (sec) |
|-----------------------|---------------|--------------------|--------------------------------------------|--------|--------|--------|--------------------|
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - enable/disable clock stretching test | 1      | 0      | OK     | 0.08               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - get capabilities test                | 1      | 0      | OK     | 0.06               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - init/free test                       | 1      | 0      | OK     | 0.04               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - master read/write test               | 1      | 0      | OK     | 0.06               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - set frequency test                   | 1      | 0      | OK     | 0.06               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - setting slave address test           | 1      | 0      | OK     | 0.06               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - slave state test                     | 1      | 0      | OK     | 0.06               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - start signal test                    | 1      | 0      | OK     | 0.04               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - stop signal test                     | 1      | 0      | OK     | 0.04               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbed_hal-i2c | i2c - transfer async test                  | 1      | 0      | OK     | 3.05               |


| target       | platform_name | test suite         | test case                    | passed | failed | result | elapsed_time (sec) |
|--------------|---------------|--------------------|------------------------------|--------|--------|--------|--------------------|
| K64F-GCC_ARM | K64F          | tests-mbed_hal-i2c | i2c - get capabilities test  | 1      | 0      | OK     | 0.06               |
| K64F-GCC_ARM | K64F          | tests-mbed_hal-i2c | i2c - init/free test         | 1      | 0      | OK     | 0.06               |
| K64F-GCC_ARM | K64F          | tests-mbed_hal-i2c | i2c - master read/write test | 1      | 0      | OK     | 0.06               |
| K64F-GCC_ARM | K64F          | tests-mbed_hal-i2c | i2c - set frequency test     | 1      | 0      | OK     | 0.04               |
| K64F-GCC_ARM | K64F          | tests-mbed_hal-i2c | i2c - start signal test      | 1      | 0      | OK     | 0.06               |
| K64F-GCC_ARM | K64F          | tests-mbed_hal-i2c | i2c - stop signal test       | 1      | 0      | OK     | 0.06               |

I2C communication test (#9573)

master(NUCLEO_F070RB) - slave(NUCLEO_F070RB)
+--------------------------------------------------------------+---------+-------------+-------------+---------------+----------+
| Testcase                                                     | Verdict | Fail Reason | Skip Reason |   platforms   | duration |
+--------------------------------------------------------------+---------+-------------+-------------+---------------+----------+
| I2C_COM_MASTER-SLAVE_ASYNC_TRANSFER_1000B_MAX_FREQUENCY      |   pass  |             |             | NUCLEO_F070RB |  15.294  |
| I2C_COM_MASTER-SLAVE_ASYNC_TRANSFER_1025B_MIN_FREQUENCY      |   pass  |             |             | NUCLEO_F070RB |  3.516   |
| I2C_COM_MASTER-SLAVE_ASYNC_TRANSFER_10B_MAX_FREQUENCY        |   pass  |             |             | NUCLEO_F070RB |  2.009   |
| I2C_COM_MASTER-SLAVE_ASYNC_TRANSFER_10B_MIN_FREQUENCY        |   pass  |             |             | NUCLEO_F070RB |  2.136   |
| I2C_COM_MASTER-SLAVE_ASYNC_TRANSFER_1B_MAX_FREQUENCY         |   pass  |             |             | NUCLEO_F070RB |  1.959   |
| I2C_COM_MASTER-SLAVE_ASYNC_TRANSFER_1B_MIN_FREQUENCY         |   pass  |             |             | NUCLEO_F070RB |  2.015   |
| I2C_COM_MASTER-SLAVE_ASYNC_WRITE-READ_1025B_MAX_FREQUENCY    |   pass  |             |             | NUCLEO_F070RB |  2.151   |
| I2C_COM_MASTER-SLAVE_ASYNC_WRITE-READ_1025B_MIN_FREQUENCY    |   pass  |             |             | NUCLEO_F070RB |  3.485   |
| I2C_COM_MASTER-SLAVE_ASYNC_WRITE-READ_10B_MAX_FREQUENCY      |   pass  |             |             | NUCLEO_F070RB |  1.961   |
| I2C_COM_MASTER-SLAVE_ASYNC_WRITE-READ_10B_MIN_FREQUENCY      |   pass  |             |             | NUCLEO_F070RB |   1.96   |
| I2C_COM_MASTER-SLAVE_ASYNC_WRITE-READ_1B_MAX_FREQUENCY       |   pass  |             |             | NUCLEO_F070RB |   2.02   |
| I2C_COM_MASTER-SLAVE_ASYNC_WRITE-READ_1B_MIN_FREQUENCY       |   pass  |             |             | NUCLEO_F070RB |   1.97   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1025B_MAX_FREQUENCY |   pass  |             |             | NUCLEO_F070RB |  2.271   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1025B_MIN_FREQUENCY |   pass  |             |             | NUCLEO_F070RB |  3.491   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_10B_MAX_FREQUENCY   |   pass  |             |             | NUCLEO_F070RB |  1.961   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_10B_MIN_FREQUENCY   |   pass  |             |             | NUCLEO_F070RB |  2.036   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1B_MAX_FREQUENCY    |   pass  |             |             | NUCLEO_F070RB |   1.95   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1B_MIN_FREQUENCY    |   pass  |             |             | NUCLEO_F070RB |  1.912   |
+--------------------------------------------------------------+---------+-------------+-------------+---------------+----------+

master(K64F) - slave(NUCLEO_F070RB)
+--------------------------------------------------------------+---------+-------------+-------------+--------------------+----------+
| Testcase                                                     | Verdict | Fail Reason | Skip Reason |     platforms      | duration |
+--------------------------------------------------------------+---------+-------------+-------------+--------------------+----------+
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1B_MAX_FREQUENCY    |   pass  |             |             | K64F,NUCLEO_F070RB |  4.535   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1025B_MIN_FREQUENCY |   pass  |             |             | K64F,NUCLEO_F070RB |  5.889   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1025B_MAX_FREQUENCY |   pass  |             |             | K64F,NUCLEO_F070RB |  4.649   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_1B_MIN_FREQUENCY    |   pass  |             |             | K64F,NUCLEO_F070RB |   4.53   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_10B_MAX_FREQUENCY   |   pass  |             |             | K64F,NUCLEO_F070RB |  4.515   |
| I2C_COM_MASTER-SLAVE_BLOCKING_WRITE-READ_10B_MIN_FREQUENCY   |   pass  |             |             | K64F,NUCLEO_F070RB |  4.539   |
+--------------------------------------------------------------+---------+-------------+-------------+--------------------+----------+

maciejbocianski · 2019-03-27T13:58:02Z

As most of issues was fixed can we move forward with this PR?
This new I2C API is essential for FPGA shield tests development thus it would be good to have it merged ASAP

jamesbeyond · 2019-03-27T14:09:44Z

seems something wrong with Travis? @0xc0170

cmonr · 2019-03-27T18:47:12Z

Travis should be good now.

cmonr · 2019-03-27T20:09:14Z

@maciejbocianski @LMESTM @donatieng Any more comments for this PR?

cmonr · 2019-03-27T20:09:38Z

@scartmell-arm I take it this won't be making it in?

Driver API Async

cmonr · 2019-04-09T20:02:07Z

@donatieng @ARMmbed/mbed-os-hal Is this good to go?

0xc0170 · 2019-04-10T09:34:28Z

cc @ithinuel

maciejbocianski · 2019-04-10T09:45:28Z

@cmonr @0xc0170 @ithinuel
I'm working on missing implementations of slave/async modes.

cmonr · 2019-04-12T00:09:33Z

It contains all changes from #8682 plus:

Closing in favor of #10374

cmonr added needs: review release-version: feature branch labels Nov 8, 2018

cmonr requested a review from a team November 8, 2018 22:44

cmonr added needs: work and removed needs: review labels Nov 8, 2018

ghost mentioned this pull request Nov 13, 2018

Add porting guide for HAL I2C API ARMmbed/mbed-os-5-docs#807

Closed

cmonr requested a review from a team November 14, 2018 16:05

cmonr added needs: CI needs: review and removed needs: work needs: CI labels Nov 15, 2018

maciejbocianski reviewed Nov 21, 2018

View reviewed changes

jamesbeyond mentioned this pull request Dec 7, 2018

HAL I2C: new I2C implementation for Fast Model targets #9013

Closed

maciejbocianski reviewed Dec 18, 2018

View reviewed changes

targets/TARGET_STM/i2c_api.c Outdated Show resolved Hide resolved

LMESTM reviewed Dec 18, 2018

View reviewed changes

maciejbocianski reviewed Dec 19, 2018

View reviewed changes

targets/TARGET_STM/i2c_api.c Outdated Show resolved Hide resolved

maciejbocianski mentioned this pull request Jan 31, 2019

Add CI i2c hal test #9564

Closed

maciejbocianski reviewed Jan 31, 2019

View reviewed changes

hal/i2c_api.h Show resolved Hide resolved

maciejbocianski mentioned this pull request Jan 31, 2019

Add Ice Tea I2C communication test #9573

Closed

donatieng suggested changes Mar 4, 2019

View reviewed changes

maciejbocianski suggested changes Mar 7, 2019

View reviewed changes

Steve Cartmell and others added 10 commits March 26, 2019 16:11

refactor(hal-i2c): Remove i2c_timeout function from the API

216b1f3

style(hal-i2c): Fix astyle code formatting failures

77e0cba

fix(hal-i2c): K64F: Ignore calls to set slave address in master mode

4bda684

fix(hal-i2c): Disable I2CSLAVE feature flag on K64F devices

c2390bb

fix(hal-i2c): Return the num bytes transferred in STM slave read/write

19ae834

fix(hal-i2c): Disable slave mode and 10-bit from I2C capabilities on K64

51fc969

fix(hal-i2c): Disable 10-bit addressing I2C capabilities on STM32

c57fb11

fix(hal-i2c): Fix compiler warning from unused variable on STM32

d150905

fix(hal-i2c): Fix return value for transfers that timeout in STM

c1ba83f

rebase fixes

7d9de66

cmonr requested review from LMESTM, donatieng and maciejbocianski and removed request for LMESTM and donatieng March 27, 2019 20:08

maciejbocianski mentioned this pull request Apr 11, 2019

I2c ref impl #10374

Merged

5 tasks

cmonr closed this Apr 12, 2019

cmonr removed the needs: review label Apr 12, 2019

RFC HAL I2C: Reference implementations for K64F and STM32F0 #8682

RFC HAL I2C: Reference implementations for K64F and STM32F0 #8682

Uh oh!

Conversation

ghost commented Nov 8, 2018

Description

Tasks

Pull request type

Uh oh!

cmonr commented Nov 8, 2018

Uh oh!

cmonr commented Nov 15, 2018

Uh oh!

maciejbocianski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maciejbocianski Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

cmonr commented Dec 14, 2018

Uh oh!

Uh oh!

LMESTM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xc0170 commented Jan 16, 2019

Uh oh!

ghost commented Jan 16, 2019

Uh oh!

Uh oh!

0xc0170 commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Feb 20, 2019

Uh oh!

cmonr commented Feb 26, 2019

Uh oh!

donatieng commented Mar 4, 2019

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

donatieng Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

maciejbocianski Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

maciejbocianski commented Mar 27, 2019

Uh oh!

maciejbocianski commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesbeyond commented Mar 27, 2019

Uh oh!

cmonr commented Mar 27, 2019

Uh oh!

cmonr commented Mar 27, 2019

Uh oh!

cmonr commented Mar 27, 2019

Uh oh!

cmonr commented Apr 9, 2019

Uh oh!

0xc0170 commented Apr 10, 2019

Uh oh!

maciejbocianski commented Apr 10, 2019

Uh oh!

cmonr commented Apr 12, 2019

0xc0170 commented Feb 13, 2019 •

edited

Loading

maciejbocianski commented Mar 27, 2019 •

edited

Loading