Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32: driver/Added I2C timing calculation algorithm #14557

Merged

Conversation

affrinpinhero-2356
Copy link
Contributor

@affrinpinhero-2356 affrinpinhero-2356 commented Apr 16, 2021

Summary of changes

  • Related issue: get_i2c_timing function uses wrong SysClock value #12907
  • Added i2c timing calculation algorithm for all supported families added.
    This patch improves I2C timing calculation by adding features of calculating timing value according to I2C input clock and I2C bus speed.
    Current modification helps user to change the system clock and I2C input clock.

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

@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested review from a team April 16, 2021 09:00
@0xc0170 0xc0170 changed the title driver/i2c: Added I2C timing calculation function. This commit added … driver/i2c: Added I2C timing calculation function Apr 22, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 22, 2021

@ARMmbed/team-st-mcd please review

targets/TARGET_STM/i2c_api.c Outdated Show resolved Hide resolved
@affrinpinhero-2356 affrinpinhero-2356 changed the title driver/i2c: Added I2C timing calculation function Driver: Added I2C timing calculation algorithm Apr 22, 2021
@mergify mergify bot dismissed 0xc0170’s stale review April 22, 2021 16:27

Pull request has been modified.

@affrinpinhero-2356
Copy link
Contributor Author

affrinpinhero-2356 commented Apr 23, 2021

@LMESTM @0xc0170 I have made requested changes.

@0xc0170 0xc0170 self-requested a review April 23, 2021 08:44
@affrinpinhero-2356 affrinpinhero-2356 changed the title Driver: Added I2C timing calculation algorithm STm32: driver/Added I2C timing calculation algorithm Apr 23, 2021
@@ -34,31 +42,14 @@ extern "C" {
#define I2CAPI_I2C3_CLKSRC RCC_I2C3CLKSOURCE_SYSCLK

/* Provide the suitable timing depending on requested frequency */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the comment still correct if the function was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 This can be removed. I have somehow missed.

uint32_t I2C_ComputeTiming(uint32_t clock_src_freq, uint32_t i2c_freq);
uint32_t get_i2c_timing(I2CName i2c, int hz);
void I2C_Compute_PRESC_SCLDEL_SDADEL(uint32_t clock_src_freq, uint32_t I2C_speed);
uint32_t I2C_Compute_SCLL_SCLH(uint32_t clock_src_freq, uint32_t I2C_speed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function declarations using camelcase or just as the rest of HAL only lower case, can we unify them?

I assume i2c_get_pclk() or if you want to keep this different from Mbed OS HAL, you can add a prefix for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 I think its better to change all to **camelCase** format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 This will be applicable for all files rite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 I have modified function name to below mentioned. Hope this will make it unique. Update me if this is okay.

uint32_t get_i2c_pclk(I2CName i2c);
uint32_t i2c_compute_timing(uint32_t clock_src_freq, uint32_t i2c_freq);
uint32_t get_i2c_timing(I2CName i2c, int hz);
void i2c_compute_Presc_Scldel_Sdadel(uint32_t clock_src_freq, uint32_t I2C_speed);
uint32_t i2c_compute_Scll_Sclh(uint32_t clock_src_freq, uint32_t I2C_speed);

Copy link
Contributor

@0xc0170 0xc0170 Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i2c_device.h belongs to HAL implementation? similar to i2c_api.c so it should be consistent with the naming. Mbed OS style guide specifies to use lower case with _ as a separator for function/methods names

If this is the case (this is HAL implementation), ti would be:

uint32_t i2c_compute_timing(uint32_t clock_src_freq, uint32_t i2c_freq);
uint32_t i2c_get_timing(I2CName i2c, int hz);
void i2c_compute_presc_scldel_sdadel(uint32_t clock_src_freq, uint32_t i2c_speed);
uint32_t i2c_compute_scll_sclh(uint32_t clock_src_freq, uint32_t i2c_speed);

@0xc0170 0xc0170 changed the title STm32: driver/Added I2C timing calculation algorithm STM32: driver/Added I2C timing calculation algorithm Apr 27, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 28, 2021

CI started

@mergify mergify bot added needs: work and removed needs: CI labels Apr 28, 2021
@mbed-ci
Copy link

mbed-ci commented Apr 28, 2021

Jenkins CI Test : ❌ FAILED

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-GCC_ARM
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM
jenkins-ci/mbed-os-ci_build-cloud-example-ARM
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM
jenkins-ci/mbed-os-ci_build-example-ARM
jenkins-ci/mbed-os-ci_build-greentea-ARM
jenkins-ci/mbed-os-ci_cmake-example-ARM
jenkins-ci/mbed-os-ci_build-example-GCC_ARM

@jeromecoutant
Copy link
Collaborator

Let's restart CI ?
Thx

@0xc0170
Copy link
Contributor

0xc0170 commented May 18, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented May 18, 2021

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

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

@jeromecoutant
Copy link
Collaborator

Jenkins CI Test : ❌ FAILED

jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ❌
jenkins-ci/mbed-os-ci_cmake-example-ARM ❌

Yes, new files have not been added in the CMakeLists.txt

@affrinpinhero-2356
Copy link
Contributor Author

Hi @jeromecoutant

I found there are n number of cmaks. And I was confused which one should I need to update.
If you can point out in which directory I need to update then it might be helpful.

@jeromecoutant
Copy link
Collaborator

For ex targets/TARGET_STM/TARGET_STM32F0/i2c_device.c has to be added in
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F0/CMakeLists.txt#L19

etc

@affrinpinhero-2356
Copy link
Contributor Author

Oh okay thanks. ☺

This commit adds I2C timing value automatic calculation algorithm
for all supported families added. This patch improves I2C timing calculation
according to I2C input clock and I2C bus speed.
This commit also allows user to change the system clock and I2C input clock.

Related issue: ARMmbed#12907

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

Signed-off-by: Affrin Pinhero <affrin.pinhero@hcl.com>
Copy link
Collaborator

@jeromecoutant jeromecoutant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's restart CI ?

@0xc0170
Copy link
Contributor

0xc0170 commented May 19, 2021

CI restarted

@mbed-ci
Copy link

mbed-ci commented May 19, 2021

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
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_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-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_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@adbridge adbridge merged commit 5ef56cc into ARMmbed:master May 24, 2021
@mergify mergify bot added the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label May 24, 2021
@mergify
Copy link

mergify bot commented May 24, 2021

This PR does not contain release version label after merging.

@0xc0170 0xc0170 added release-type: patch Indentifies a PR as containing just a patch Release-pending release-version: 6.12.0 and removed release version missing When PR does not contain release version, bot should label it and we fix it afterwards labels Jun 22, 2021
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants