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

boards/stm32: fix CAN bit timings for boards #15833

Merged
merged 6 commits into from
Feb 14, 2025
Merged

Conversation

raiden00pl
Copy link
Member

Summary

  • boards/nucleo-f722ze: fix CAN bit timings
  • boards/nucleo-g431rb: fix CAN bit timings and unify bitrate
  • boards/nucleo-f302r8: fix CAN bit timings
  • boards/nucleo-f446re: fix CAN bit timings
  • boards/b-g431b-esc1: fix CAN bit timings
  • boards/nucleo-f303re: fix CAN bit timings

Impact

all updated CAN configurations are now compatible with each other

Testing

different board combinations in the CAN network.

here example with b-g431rb-esc [1] nucleo-g431rb [2] and nucleo-f302r8:

[1]
nsh> cansend can0 123#DEADBEEF

[2]
nsh> candump can0
  can0  123   [4]  DE AD BE EF
  
[3]
nsh> candump can0
  can0  123   [4]  DE AD BE EF

boards/nucleo-f722ze: fix CAN bit timings, calculated with http://www.bittiming.can-wiki.info/

Signed-off-by: raiden00pl <[email protected]>
boards/nucleo-g431rb: fix CAN bit timings, calculated with https://phryniszak.github.io/stm32g-fdcan/

also update bit rate to 250000 so it's the same as for other stm32 boards

Signed-off-by: raiden00pl <[email protected]>
boards/nucleo-f302r8: fix CAN bit timings, calculated with http://www.bittiming.can-wiki.info/

Signed-off-by: raiden00pl <[email protected]>
boards/nucleo-f446re: fix CAN bit timings, calculated with http://www.bittiming.can-wiki.info/

Signed-off-by: raiden00pl <[email protected]>
boards/b-g431b-esc1: fix CAN bit timings, calculated with https://phryniszak.github.io/stm32g-fdcan/

Signed-off-by: raiden00pl <[email protected]>
boards/nucleo-f303re: fix CAN bit timings, calculated with http://www.bittiming.can-wiki.info/

Signed-off-by: raiden00pl <[email protected]>
@raiden00pl raiden00pl changed the title Stm32 can1 boards/stm32: fix CAN bit timings for boards Feb 13, 2025
@github-actions github-actions bot added Board: arm Size: S The size of the change in this PR is small labels Feb 13, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 13, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be more thorough.

Strengths:

  • Clear Summary of Changes: The functional area (CAN bit timings) and affected boards are clearly listed. The "why" (fix) is implied, but could be stated explicitly (e.g., "fix incorrect CAN bit timings to enable interoperability").
  • Impact Description: The impact on the user (interoperability) is stated concisely.
  • Testing Evidence: The testing demonstrates interoperability between several boards, which directly addresses the stated fix.

Weaknesses:

  • Missing Detail in Summary: How the change works is missing. A brief explanation of the nature of the bit timing corrections would be helpful. Mentioning related issues would also strengthen the context.
  • Incomplete Impact Assessment: The impact sections for build, hardware, documentation, security, and compatibility are all implied as "NO" but not explicitly stated. While likely true, it's better to explicitly confirm these for completeness.
  • Limited Testing Detail: While the test output demonstrates basic functionality, it lacks detail. Specify the OS, CPU, compiler used on the build host. For the targets, list the full board:config names. Include more details about the "different board combinations" tested. More comprehensive testing would include various message lengths and frequencies. "Before" logs are missing, although they might not be relevant if communication was previously impossible.

Recommendation:

While the core information is present, expanding on the weaknesses identified above would significantly improve the PR's clarity and ensure it fully meets the NuttX requirements.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thanks @raiden00pl :-) Also thank you for providing runtime verification logs :-)

@jerpelea jerpelea merged commit 107b375 into apache:master Feb 14, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants