Dev stm i2c v2 unitary functions #3488

LMESTM · 2016-12-21T09:25:00Z

Description

This PR shall close my recent series of rework (and fixes) to get STM32 I2C into a single file.
This one covers the unitary start / stop / byte read and write functions for STM32 for F0, F3, F3, L0 and L4 families.

Status

Status READY

Test report:

Related PRs

To have ARM ci test shield I2C tests working ok as well on those targets, an update of the I2Ceeprom driver is also needed on top of this PR. The proposed patch has been submitted here:
https://developer.mbed.org/users/LMESTM/code/I2CEeprom/

With the I2Ceeprom driver update and this PR, the I2C ci test shield is tested ok:

+-------------------+---------------+---------------+------------------------------------+--------+--------+--------+--------------------+
| target | platform_name | test suite | test case | passed | failed | result | elapsed_time (sec) |
+-------------------+---------------+---------------+------------------------------------+--------+--------+--------+--------------------+
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-api-i2c | I2C - EEProm WR 10 Bytes | 1 | 0 | OK | 0.06 |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-api-i2c | I2C - EEProm WR 100 Bytes | 1 | 0 | OK | 0.08 |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-api-i2c | I2C - EEProm WR 2 Bytes | 1 | 0 | OK | 0.06 |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-api-i2c | I2C - EEProm WR Single Byte | 1 | 0 | OK | 0.02 |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-api-i2c | I2C - Instantiation of I2C Object | 1 | 0 | OK | 0.03 |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-api-i2c | I2C - LM75B Temperature Read | 1 | 0 | OK | 0.07 |
+-------------------+---------------+---------------+------------------------------------+--------+--------+--------+--------------------+

STM32 supported targets have 2 possible versions of I2C. This patch makes the start / stop / read and write byte work ok for IP V2. This was not working before and does not seem to be widely used.

This avoids conflicts with slave on ci-test-shield

depending on timing and HW, there might be some delay before the master request gets notified, so better loop in while than a single call to slave.receive()

it's master not slave that shall send STOP at the end of I2C transfer

bridadan

Thanks for all of your work on this! I've found a few lines that need some clarification, could you please take a look?

bridadan · 2016-12-27T16:42:37Z

targets/TARGET_STM/i2c_api.c

@@ -514,7 +525,7 @@ int i2c_byte_read(i2c_t *obj, int last) {
    timeout = FLAG_TIMEOUT;
    while (__HAL_I2C_GET_FLAG(handle, I2C_FLAG_RXNE) == RESET) {
        if ((timeout--) == 0) {
-            return -1;
+            return 2;


Can you clarify the reason for this change? Seems like this should return an error instead of what could be considered valid data?

that's how this is defined in the MBED HAL API of i2c_byte_read
https://github.com/ARMmbed/mbed-os/blob/master/hal/i2c_api.h#L144

@return 0 if NAK was received, 1 if ACK was received, 2 for timeout.

@LMESTM the line you have highlighted for i2c_byte_write, not i2c_byte_read.

@bridadan sorry ! I missed the point ... will post an update

bridadan · 2016-12-27T16:44:29Z

targets/TARGET_STM/i2c_api.c

+    if ((tmpreg & I2C_CR2_RELOAD) != 0) {
+        while (!__HAL_I2C_GET_FLAG(handle, I2C_FLAG_TCR)) {
+            if ((timeout--) == 0) {
+                printf("timeout in byte_read\r\n");


Looks like this printf was left in by mistake? This should probably just return an error code (or maybe print a debug message if debug messages are enabled? Haven't looked at our debug prints in a while though...)

right - will correct this !

bridadan · 2016-12-27T16:45:43Z

targets/TARGET_STM/i2c_api.c

        if ((timeout--) == 0) {
-            return -1;
+            return 2;


Similar comment to what I made above. If this is a timeout, we probably shouldn't return anything non-negative, as that could be perceived as actual data, correct?

same as above

bridadan · 2016-12-27T16:46:01Z

targets/TARGET_STM/i2c_api.c

+        if ((tmpreg & I2C_CR2_RELOAD) != 0) {
+            while (!__HAL_I2C_GET_FLAG(handle, I2C_FLAG_TCR)) {
+                if ((timeout--) == 0) {
+                    printf("timeout in byte_write\r\n");


Found another printf!

will correct - thx for review

0xc0170 · 2016-12-30T11:39:12Z

@LMESTM bump

btw, where is version v2 macro defined?

LMESTM · 2017-01-03T09:09:42Z

@0xc0170 : the version V1 or V2 is defined in i2c_device.h of each family.
@bridadan : I will remove printf and update

Few debug lines were to be removed / updated. Move the printf to DEBUG_PRINTF and return the error when needed.

LMESTM · 2017-01-03T09:20:56Z

@0xc0170 @bridadan update done

0xc0170 · 2017-01-03T14:53:58Z

@bridadan please review again

bridadan

Thanks for removing the printfs! I still have concerns over the return value for i2c_byte_read. The return values are different than that of i2c_byte_write, but this PR makes them the same.

To make clear that an error is being reported, we shall report -1, 2 being the timeout error for i2c_byte_write only.

0xc0170 · 2017-01-03T18:06:27Z

@LMESTM I noticed i2c_byte_read does not define properly return types. Please can you file an issue?

bridadan

Great work!

bridadan · 2017-01-03T20:03:31Z

/morph test

mbed-bot · 2017-01-03T22:29:26Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1349

All builds and test passed!

bridadan · 2017-01-03T22:51:49Z

@VeliMattiLahtela Can you comment on the Oulu CI failure please?

LMESTM · 2017-01-04T07:58:52Z

@0xc0170 Issue #3522 submitted
@bridadan I don't know which CI failure you're pointing to, but I have pushed a Pull request for the I2C eeprom driver used for CI testing and haven't recevied an answer so far; https://developer.mbed.org/users/rhourahane/code/I2CEeprom/pull-request/1

bridadan · 2017-01-04T16:49:50Z

@LMESTM The CI error has since been resolved, so I think this ready to come in @0xc0170!

LMESTM · 2017-01-04T16:52:51Z

@bridadan sounds good - thx !

Ports for Upcoming Targets Fixes and Changes 3488: Dev stm i2c v2 unitary functions ARMmbed/mbed-os#3488 3492: Fix #3463 CAN read() return value ARMmbed/mbed-os#3492 3503: [LPC15xx] Ensure that PWM=1 is resolved correctly ARMmbed/mbed-os#3503 3504: [LPC15xx] CAN implementation improvements ARMmbed/mbed-os#3504 3539: NUCLEO_F412ZG - Add support of TRNG peripheral ARMmbed/mbed-os#3539 3540: STM: SPI: Initialize Rx in spi_master_write ARMmbed/mbed-os#3540 3438: K64F: Add support for SERIAL ASYNCH API ARMmbed/mbed-os#3438 3519: MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set ARMmbed/mbed-os#3519 3544: STM32L4 deepsleep improvement ARMmbed/mbed-os#3544 3546: NUCLEO-F412ZG - Add CAN peripheral ARMmbed/mbed-os#3546 3551: Fix I2C driver for RZ/A1H ARMmbed/mbed-os#3551 3558: K64F UART Asynch API: Fix synchronization issue ARMmbed/mbed-os#3558 3563: LPC4088 - Fix vector checksum ARMmbed/mbed-os#3563 3567: Dev stm32 F0 v1.7.0 ARMmbed/mbed-os#3567 3577: Fixes linking errors when building with debug profile ARMmbed/mbed-os#3577

LMESTM added 5 commits December 21, 2016 09:27

STM32: I2C unitary functions for IP V2

20c9af8

STM32 supported targets have 2 possible versions of I2C. This patch makes the start / stop / read and write byte work ok for IP V2. This was not working before and does not seem to be widely used.

I2C test: change slave address

7066734

This avoids conflicts with slave on ci-test-shield

I2C test: let slave time to get notified

d67b431

depending on timing and HW, there might be some delay before the master request gets notified, so better loop in while than a single call to slave.receive()

I2C test: master should send stop

bcf82b0

it's master not slave that shall send STOP at the end of I2C transfer

I2C test: add L073RZ

517d0d9

0xc0170 added the needs: review label Dec 21, 2016

bridadan suggested changes Dec 27, 2016

View reviewed changes

bridadan added needs: work and removed needs: review labels Dec 27, 2016

STM32: I2C: remove debug code

455c2ec

Few debug lines were to be removed / updated. Move the printf to DEBUG_PRINTF and return the error when needed.

0xc0170 added needs: review and removed needs: work labels Jan 3, 2017

0xc0170 approved these changes Jan 3, 2017

View reviewed changes

bridadan suggested changes Jan 3, 2017

View reviewed changes

STM32: I2C: i2c_byte_read return value in case of error

4297e3f

To make clear that an error is being reported, we shall report -1, 2 being the timeout error for i2c_byte_write only.

bridadan approved these changes Jan 3, 2017

View reviewed changes

bridadan added needs: CI and removed needs: review labels Jan 3, 2017

bridadan added ready for merge and removed needs: CI labels Jan 4, 2017

sg- merged commit ddcd3ad into ARMmbed:master Jan 9, 2017

sg- added release-version: 5.3.3 and removed ready for merge labels Jan 9, 2017

LMESTM deleted the dev_stm_i2c_v2_unitary_functions branch February 27, 2017 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev stm i2c v2 unitary functions #3488

Dev stm i2c v2 unitary functions #3488

LMESTM commented Dec 21, 2016

bridadan left a comment

bridadan Dec 27, 2016

LMESTM Jan 3, 2017

bridadan Jan 3, 2017

LMESTM Jan 3, 2017

bridadan Dec 27, 2016

LMESTM Jan 3, 2017

bridadan Dec 27, 2016

LMESTM Jan 3, 2017

bridadan Dec 27, 2016

LMESTM Jan 3, 2017

0xc0170 commented Dec 30, 2016

LMESTM commented Jan 3, 2017

LMESTM commented Jan 3, 2017

0xc0170 commented Jan 3, 2017

bridadan left a comment

0xc0170 commented Jan 3, 2017

bridadan left a comment

bridadan commented Jan 3, 2017

mbed-bot commented Jan 3, 2017

bridadan commented Jan 3, 2017

LMESTM commented Jan 4, 2017

bridadan commented Jan 4, 2017

LMESTM commented Jan 4, 2017

Dev stm i2c v2 unitary functions #3488

Dev stm i2c v2 unitary functions #3488

Conversation

LMESTM commented Dec 21, 2016

Description

Status

Related PRs

bridadan 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

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

0xc0170 commented Dec 30, 2016

LMESTM commented Jan 3, 2017

LMESTM commented Jan 3, 2017

0xc0170 commented Jan 3, 2017

bridadan left a comment

Choose a reason for hiding this comment

0xc0170 commented Jan 3, 2017

bridadan left a comment

Choose a reason for hiding this comment

bridadan commented Jan 3, 2017

mbed-bot commented Jan 3, 2017

Output

bridadan commented Jan 3, 2017

LMESTM commented Jan 4, 2017

bridadan commented Jan 4, 2017

LMESTM commented Jan 4, 2017