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

Fix for calculating CAN timing settings for STM32 #15317

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

chdelfs
Copy link
Contributor

@chdelfs chdelfs commented Aug 4, 2022

NominalPrescaler value needs to be as high as possible to ensure a good approximation of the target CAN speed.
Previous usage of macro IS_FDCAN_DATA_TSEG1 refers to (unsupported by Mbed ) FDCAN CAN controller settings and leads to too low prescaler values.
Usage Macro IS_FDCAN_NOMINAL_TSEG1 yields optimum results.
See also correct macro usage in line 158.

Summary of changes

Replaced macro IS_FDCAN_DATA_TSEG1 by IS_FDCAN_NOMINAL_TSEG1 to ensure that nominalPrescaler is calculated correctly.

Impact of changes

Method frequency() of CAN API can safely be used.
If no fix is applied, any call to this method will lead to improper CAN bus settings and CAN transmissions are not reliable or do not work at all.

Migration actions required

No

Documentation

None

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

The bug becomes visible as soon as the method frequency() of the CAN API is used, even if the default frequency of 100kHz is applied.
If no fix is applied, STM32G4 device will enter "bus off" state right after attempting to send CAN frames.
Reason is that the CAN timing settings will lead to a CAN bus frequency of the CAN Controller which deviates by 2% from the target frequency (for 100kHz target frequency). With a fix, this deviation is 0.2%.

I assume that so far CAN related tests used the default timing settings, but never checked the impact of using the method frequency().

Details are provided in https://forums.mbed.com/t/no-successful-can-bus-sending-with-nucleog474re/17449/12.

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

Reviewers


NominalPrescaler value needs to be as high as possible to ensure a good approximation of the target CAN speed.
Previous usage of macro IS_FDCAN_DATA_TSEG1 refers to (unsupported by Mbed ) FDCAN CAN controller settings and leads to too low prescaler values.
Usage Macro IS_FDCAN_NOMINAL_TSEG1 yields optimum results.
See also correct macro usage in line ARMmbed#158.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 4, 2022
@ciarmcom ciarmcom requested a review from a team August 4, 2022 17:30
@ciarmcom
Copy link
Member

ciarmcom commented Aug 4, 2022

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

@@ -335,7 +335,7 @@ int can_frequency(can_t *obj, int f)
// !When the sample point should be lower than 50%, this must be changed to
// !IS_FDCAN_DATA_TSEG2(ntq/nominalPrescaler), since
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this comment be updated ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this shall be now IS_FDCAN_NOMINAL_TSEG2 ?

0xc0170
0xc0170 previously approved these changes Aug 17, 2022
@chdelfs
Copy link
Contributor Author

chdelfs commented Sep 7, 2022

IMHO, the comment can or should be updated as well. See also the equivalent comment in function can_internal_init() which partially duplicates the code from can_frequency().

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 4, 2022

IMHO, the comment can or should be updated as well. See also the equivalent comment in function can_internal_init() which partially duplicates the code from can_frequency().

Will you update this pull request?

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 12, 2022

Shall this be updated so we can proceed with integration?

Modified comment as discussed.
@mergify mergify bot dismissed 0xc0170’s stale review October 12, 2022 15:17

Pull request has been modified.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 13, 2022

CI started

@mergify mergify bot added needs: CI and removed needs: review labels Oct 13, 2022
@mbed-ci
Copy link

mbed-ci commented Oct 13, 2022

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 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_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-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

@mbed-ci
Copy link

mbed-ci commented Oct 13, 2022

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 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_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-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 ✔️

@chdelfs
Copy link
Contributor Author

chdelfs commented Nov 1, 2022

Hi all,
maybe I am too impatient. But I am wondering when the fix is going to be merged. Could anybody please comment?
Many thanks!

@0xc0170 0xc0170 merged commit 102d2f8 into ARMmbed:master Nov 2, 2022
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 2, 2022

maybe I am too impatient. But I am wondering when the fix is going to be merged. Could anybody please comment?

You are not! Thanks for bringing this up. I merged it.

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.

7 participants