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

Nuvoton: Fix NuMaker I2C timeout #13679

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Conversation

cyliangtw
Copy link
Contributor

Summary of changes

This PR is to fix I2C timeout measurement for NuMaker platforms instead of hardcode max timer count.

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


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 29, 2020
@ciarmcom ciarmcom requested a review from a team September 29, 2020 06:00
@ciarmcom
Copy link
Member

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

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Please add details to the commit message how is this fixing (was 0xffffff causing an issue on the bus , what was it?)

  some H/W timer count is 24 bits only, hardcode 0xffffffff causing
  wrong judgement of timeout as while H/W timer counting overflow.
@mergify mergify bot dismissed 0xc0170’s stale review September 29, 2020 13:07

Pull request has been modified.

@cyliangtw
Copy link
Contributor Author

Please add details to the commit message how is this fixing (was 0xffffff causing an issue on the bus , what was it?)

@0xc0170 , I added more details to this commit message. As while H/W timer counting overflow, it will encounter i2c fake time out and impact i2c application.

0xc0170
0xc0170 previously approved these changes Sep 29, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 29, 2020

Can you review gitattributetest, something isn ot correct. running git diff --exit-code failed.

@mergify mergify bot dismissed 0xc0170’s stale review September 30, 2020 02:28

Pull request has been modified.

@cyliangtw
Copy link
Contributor Author

@0xc0170 , Resolved gitattributetest issue by commit 3916026 and it already passed travis-ci/gitattributestest.

@mergify mergify bot added needs: CI and removed needs: work labels Sep 30, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2020

One thought: shouldn't we use us_timestamp_t ticker_read_us(const ticker_data_t *const ticker); instead of us ticker?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2020

CI started

@cyliangtw
Copy link
Contributor Author

One thought: shouldn't we use us_timestamp_t ticker_read_us(const ticker_data_t *const ticker); instead of us ticker?

@0xc0170 , ticker_read_us() is upper layer API and need critical section overhead. us_ticker_read() is low level API and could get more efficiency. Anyway, these 2 ways could achieve the same goal.

@mbed-ci
Copy link

mbed-ci commented Sep 30, 2020

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-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Oct 21, 2020
@cyliangtw cyliangtw deleted the nvt_i2c_timeout branch December 6, 2021 07:22
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.

5 participants