driver/i2c: STM32: I2C memory usage and time delay in read-write solved. #14776

affrinpinhero-2356 · 2021-06-15T09:51:08Z

Summary of changes

This PR solves I2C Read Write Take long time and excess usage of RAM. Modified
i2c_device drivers to help user to enable disable I2C timing algorithm. Algorithm Disable
and enable can done using target.json.

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
x[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

Modified i2c_device.h file. This will solve I2C read write long time issue. Updated default I2C peripheral clock value to 160MHz. fix: ARMmbed#14732 Signed-off-by: Affrin Pinhero <[email protected]>

ciarmcom · 2021-06-15T10:00:17Z

@affrinpinhero-2356, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

jeromecoutant

Does this fix #14732 ?

@pilotak

jeromecoutant · 2021-06-15T12:18:31Z

targets/targets.json

+            },
+            "i2c_timing_value_algo": {
+                "help": "If value was set I2C timing algorithm is enabled. Enabling may leads to performance issue. Keeping this false and changing system clock will cause compilation error)",
+                "value": true


Why this value is true ?

No missed after testing. changed to false

jeromecoutant

I2C test is crashing with F072 Nucleo board :-(

affrinpinhero-2356 · 2021-06-15T13:04:12Z

I2C test is crashing with F072 Nucleo board :-(

okay. Let me run once again. Is there any specific error code that to check?

jeromecoutant · 2021-06-15T13:22:35Z

I2C test is crashing with F072 Nucleo board :-(

I checked the issue:

i2c_get_timing is called quite at the beginning of i2c_frequency
at this time, I2C clock source is not set, so value is 0 which is HSI which is not I2C_PCLK_DEF ==> crash
then __HAL_RCC_I2C1_CONFIG(I2CAPI_I2C1_CLKSRC);
all next i2c_get_timing call is OK

affrinpinhero-2356 · 2021-06-15T13:29:25Z

I2C test is crashing with F072 Nucleo board :-(

I checked the issue:

i2c_get_timing is called quite at the beginning of i2c_frequency

at this time, I2C clock source is not set, so value is 0 which is HSI which is not I2C_PCLK_DEF ==> crash

then __HAL_RCC_I2C1_CONFIG(I2CAPI_I2C1_CLKSRC);

all next i2c_get_timing call is OK
That is strange. If that was the case we cant initialize the value in JSON as false.

affrinpinhero-2356 · 2021-06-15T15:37:09Z

targets/TARGET_STM/i2c_api.c

+    /*  Only predefined timing for below frequencies are supported */
+    MBED_ASSERT((hz == 100000) || (hz == 400000) || (hz == 1000000));
+    /* Calculates I2C timing value with respect to I2C input clock and I2C bus frequency */
+    handle->Init.Timing = i2c_get_timing(obj_s->i2c, hz);


I hope this will solve.

You forgot #ifdef I2C_IP_VERSION_V2

jeromecoutant · 2021-06-15T16:14:23Z

targets/TARGET_STM/i2c_api.c

+    /*  Only predefined timing for below frequencies are supported */
+    MBED_ASSERT((hz == 100000) || (hz == 400000) || (hz == 1000000));
+    /* Calculates I2C timing value with respect to I2C input clock and I2C bus frequency */
+    handle->Init.Timing = i2c_get_timing(obj_s->i2c, hz);


You forgot #ifdef I2C_IP_VERSION_V2

chrJost · 2021-06-16T07:31:12Z

targets/TARGET_STM/TARGET_STM32WL/i2c_device.c

-                break;
+            true  break;


This seems to be an error, the "true" here should not be there, I guess

pilotak · 2021-06-16T11:27:54Z

Maybe i do something wrong when I compile it for NUCLEO_F091RC i get following error from debug console. I don't actually run it on nucleo but custom board in which case internal clock is used. I also did a test for custom board with external crystal setting - the result is the same

In mbed_config.h there is correct value set

#define MBED_CONF_TARGET_I2C_TIMING_VALUE_ALGO                                0

from debug

++ MbedOS Error Info ++
Error Status: 0x80FF0144 Code: 324 Module: 255
Error Message: Assertion failed: MBED_CONF_TARGET_I2C_TIMING_VALUE_ALGO
Location: 0x800281D
File: .\mbed-os\targets\TARGET_STM\TARGET_STM32F0\i2c_device.c+84
Error Value: 0x0
Current Thread: main Id: 0x20000AA4 Entry: 0x80019D9 StackSize: 0x1000 StackMem: 0x20000CE0 SP: 0x20001B9C
Next:
main  State: 0xhX Entry: 0x00000002 Stack Size: 0x080019D9 Mem: 0x00001000 SP: 0x20000CE0
Ready:
rtx_idle  State: 0xhX Entry: 0x00000001 Stack Size: 0x08001AD5 Mem: 0x00000200 SP: 0x20000838
Wait:
rtx_timer  State: 0xhX Entry: 0x00000083 Stack Size: 0x08001829 Mem: 0x00000300 SP: 0x20000538
Delay:
For more info, visit: https://mbed.com/s/error?error=0x80FF0144&tgt=NUCLEO_F091RC
-- MbedOS Error Info --

jeromecoutant · 2021-06-16T11:30:06Z

@pilotak did you fetch before:

affrinpinhero-2356 force-pushed the affrinpinhero-2356:i2c_longTime_Mem_Solve branch from 2c464c8 to 1c10ea4 20 hours ago

pilotak · 2021-06-16T11:34:53Z

@jeromecoutant i'm at a06d6a6

affrinpinhero-2356 · 2021-06-16T12:51:15Z

@pilotak are you using default clock configuration or custom?

pilotak · 2021-06-16T13:00:01Z

@affrinpinhero-2356 default, as a said I compiled the code for NUCLEO_F091RC and ran on custom board

affrinpinhero-2356 · 2021-06-16T18:02:54Z

@pilotak You need to change the value of I2C_TIMING_VALUE_ALGO to true I target.json file. If your clock configuration is not default then this error will get triggered.

pilotak · 2021-06-17T06:48:00Z

@affrinpinhero-2356 could you please clarify what do you mean by default configuration? - does it mean that it will only work on nucleo with external clock and not with external crystal / internal clock?

I didn't adjust anything so i'm using default setting USE_PLL_HSE_EXTC|USE_PLL_HSI (NUCLEO_F091RC derived clock from MCU_STM32F0) in which my case EXTC in not provided - that basically means that it's running on internal lock (HSI).

0xc0170 · 2021-06-18T10:21:28Z

targets/TARGET_STM/TARGET_STM32F0/i2c_device.c

+        /* If MBED_CONF_TARGET_I2C_TIMING_VALUE_ALGO assert is triggered. 
+        User needs to enable I2C_TIMING_VALUE_ALGO in target.json for specific target. 
+        Enabling this may impact performance*/
+        MBED_ASSERT(MBED_CONF_TARGET_I2C_TIMING_VALUE_ALGO);


Shouldn't this be an error rather and print a message how to fix it?

Mentioned as a comment what to do assert occurred. So I think this will be enough.

0xc0170 · 2021-06-18T10:22:43Z

targets/targets.json

+            },
+            "i2c_timing_value_algo": {
+                "help": "If value was set I2C timing algorithm is enabled. Enabling may leads to performance issue. Keeping this false and changing system clock will cause compilation error)",
+                "value": false


If this is false (default value), and I change system clock, why do I get compilation error?

In this scenario, there will no error while compiling. But you may get an ASSERT while running.

@affrinpinhero-2356 please update help description accordingly

modified help description.

jeromecoutant · 2021-06-22T07:25:52Z

Hi

Maybe @pilotak issue is related to I2C_2 ?
It seems that I2C_2 is not expected in STM32F0...
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F0/i2c_device.c#L46

jeromecoutant · 2021-06-22T07:35:02Z

@facchinm @pennam
I am wondering if PORTENTA should enable "i2c_timing_value_algo" new config ?

pennam · 2021-06-22T07:46:13Z

@facchinm @pennam
I am wondering if PORTENTA should enable "i2c_timing_value_algo" new config ?

@jeromecoutant thanks for keep us updated! i've just had a quick look at this pr, but yes, by default the "i2c_timing_value_algo" should be enable for PORTENTA.

jeromecoutant

Needs update for @pilotak and @pennam

pilotak · 2021-06-22T08:04:26Z

@jeromecoutant i'm using PA_9 and PA_10 which is I2C_1

I2C_2 is not expected? really? i'm planning to use it as well so should not happend doesn't seems to be right, F091 has two regular buses

mbed-os/targets/TARGET_STM/TARGET_STM32F0/i2c_device.c

Line 45 in cecc47b

// should not happend

pilotak · 2021-06-22T08:07:51Z

Error when using I2C_2 at pins PB_13 and PB_14 (@F091RC)

++ MbedOS Error Info ++
Error Status: 0x80FF0100 Code: 256 Module: 255
Error Message: Fatal Run-time error
Location: 0x80052AD
Error Value: 0x0
Current Thread: main Id: 0x20000AA4 Entry: 0x80019D9 StackSize: 0x1000 StackMem: 0x20000CE0 SP: 0x20001B9C
Next:
main  State: 0xhX Entry: 0x00000002 Stack Size: 0x080019D9 Mem: 0x00001000 SP: 0x20000CE0
Ready:
rtx_idle  State: 0xhX Entry: 0x00000001 Stack Size: 0x08001AD5 Mem: 0x00000200 SP: 0x20000838
Wait:
rtx_timer  State: 0xhX Entry: 0x00000083 Stack Size: 0x08001829 Mem: 0x00000300 SP: 0x20000538
Delay:
For more info, visit: https://mbed.com/s/error?error=0x80FF0100&tgt=NUCLEO_F091RC
-- MbedOS Error Info --
I2C: unknown instance

pilotak · 2021-06-23T08:44:12Z

@affrinpinhero-2356 with 2fae588 it's working as expected.

ciarmcom · 2021-06-28T07:50:49Z

This pull request has automatically been marked as stale because it has had no recent activity. @jeromecoutant, @0xc0170, please complete review of the changes to move the PR forward. Thank you for your contributions.

jeromecoutant · 2021-06-28T09:14:28Z

targets/targets.json

+            },
+            "i2c_timing_value_algo": {
+                "help": "If value was set to true I2C timing algorithm is enabled. Enabling may leads to performance issue. Keeping this false and changing system clock will trigger assert.)",
+                "value": true


I don't think this is working ?
As PORTENTA_H7 is inheriting from MCU_STM32H7, "i2c_timing_value_algo" config already exist,
so I think you need to overwrite the value to true in the "overrides" part below ?

mbed test -m PORTENTA_H7_M7 -t ARM [ERROR] Parameter name 'i2c_timing_value_algo' defined in both 'target:PORTENTA_H7' and 'target:MCU_STM32H7'

Moved to overrides

@@ -3178,10 +3178,6 @@ "usb_speed": { "help": "USE_USB_OTG_FS or USE_USB_OTG_HS or USE_USB_HS_IN_FS", "value": "USE_USB_OTG_HS" - }, - "i2c_timing_value_algo": { - "help": "If value was set to true I2C timing algorithm is enabled. Enabling may leads to performance issue. Keeping this false and changing system clock will trigger assert.)", - "value": true } }, "macros_add": [ @@ -3202,7 +3198,8 @@ "clock_source": "USE_PLL_HSE_EXTC", "lse_available": 1, "lpticker_delay_ticks": 0, - "network-default-interface-type": "ETHERNET" + "network-default-interface-type": "ETHERNET", + "i2c_timing_value_algo": true }, "device_name": "STM32H747XIHx" },

This commit solves excess usage of RAM. User can now enable/disable I2C timing algorithm. Disabling of I2C timing algorithm would reduce RAM usage. Signed-off-by: Affrin Pinhero <[email protected]>

0xc0170 · 2021-06-29T07:26:18Z

CI started

mbed-ci · 2021-06-29T08:23:31Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

driver/i2c: STM32: I2C Read Write Take long time

ef5ac45

Modified i2c_device.h file. This will solve I2C read write long time issue. Updated default I2C peripheral clock value to 160MHz. fix: ARMmbed#14732 Signed-off-by: Affrin Pinhero <[email protected]>

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jun 15, 2021

ciarmcom requested a review from a team June 15, 2021 10:00

ciarmcom added needs: review devices: st labels Jun 15, 2021

jeromecoutant reviewed Jun 15, 2021

View reviewed changes

affrinpinhero-2356 force-pushed the i2c_longTime_Mem_Solve branch from 1dd44d2 to 2c464c8 Compare June 15, 2021 12:33

jeromecoutant suggested changes Jun 15, 2021

View reviewed changes

affrinpinhero-2356 force-pushed the i2c_longTime_Mem_Solve branch from 2c464c8 to 1c10ea4 Compare June 15, 2021 15:35

affrinpinhero-2356 commented Jun 15, 2021

View reviewed changes

jeromecoutant suggested changes Jun 15, 2021

View reviewed changes

affrinpinhero-2356 force-pushed the i2c_longTime_Mem_Solve branch from 1c10ea4 to ee3d0bd Compare June 15, 2021 16:22

chrJost reviewed Jun 16, 2021

View reviewed changes

affrinpinhero-2356 force-pushed the i2c_longTime_Mem_Solve branch from ee3d0bd to a06d6a6 Compare June 16, 2021 08:18

affrinpinhero-2356 requested review from jeromecoutant and removed request for a team June 16, 2021 08:21

jeromecoutant approved these changes Jun 16, 2021

View reviewed changes

jeromecoutant mentioned this pull request Jun 16, 2021

STM32: I2C write takes a long time #14732

Closed

0xc0170 reviewed Jun 18, 2021

View reviewed changes

jeromecoutant suggested changes Jun 22, 2021

View reviewed changes

affrinpinhero-2356 force-pushed the i2c_longTime_Mem_Solve branch from 51ae319 to 2fae588 Compare June 23, 2021 08:30

affrinpinhero-2356 requested review from jeromecoutant and 0xc0170 June 25, 2021 14:06

ciarmcom added the stale Stale Pull Request label Jun 28, 2021

jeromecoutant approved these changes Jun 28, 2021

View reviewed changes

ciarmcom removed the stale Stale Pull Request label Jun 28, 2021

driver/i2c: STM32: Solves excess memory usage issue.

8f24f09

This commit solves excess usage of RAM. User can now enable/disable I2C timing algorithm. Disabling of I2C timing algorithm would reduce RAM usage. Signed-off-by: Affrin Pinhero <[email protected]>

affrinpinhero-2356 force-pushed the i2c_longTime_Mem_Solve branch from 2fae588 to 8f24f09 Compare June 28, 2021 12:44

jeromecoutant approved these changes Jun 28, 2021

View reviewed changes

0xc0170 approved these changes Jun 28, 2021

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jun 28, 2021

mergify bot added ready for merge and removed needs: CI labels Jun 29, 2021

0xc0170 merged commit 8188f5f into ARMmbed:master Jun 29, 2021

mergify bot removed the ready for merge label Jun 29, 2021

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021

driver/i2c: STM32: I2C memory usage and time delay in read-write solved. #14776

driver/i2c: STM32: I2C memory usage and time delay in read-write solved. #14776

Uh oh!

Conversation

affrinpinhero-2356 commented Jun 15, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Jun 15, 2021

Uh oh!

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

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

affrinpinhero-2356 commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromecoutant commented Jun 15, 2021

Uh oh!

affrinpinhero-2356 commented Jun 15, 2021

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!

pilotak commented Jun 16, 2021

Uh oh!

jeromecoutant commented Jun 16, 2021

Uh oh!

pilotak commented Jun 16, 2021

Uh oh!

affrinpinhero-2356 commented Jun 16, 2021

Uh oh!

pilotak commented Jun 16, 2021

Uh oh!

affrinpinhero-2356 commented Jun 16, 2021

Uh oh!

pilotak commented Jun 17, 2021

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!

jeromecoutant commented Jun 22, 2021

Uh oh!

jeromecoutant commented Jun 22, 2021

Uh oh!

pennam commented Jun 22, 2021

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

pilotak commented Jun 22, 2021

affrinpinhero-2356 commented Jun 15, 2021 •

edited

Loading

jeromecoutant Jun 28, 2021 •

edited

Loading