Skip to content

net: use bps units for bitrate in SocketCAN interfaces. #16225

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

Merged
merged 1 commit into from
Apr 16, 2025

Conversation

csanchezdll
Copy link
Contributor

@csanchezdll csanchezdll commented Apr 16, 2025

Summary

This makes SocketCAN bitrate units compatible with what Linux uses (bps instead of previous kbps) and allows representing usual Single Wire Can (SAE J2411) usual bitrates of 33333 and 83333 bps.

Impact

All calls to bitrate-related ioctl need to be updated. I have updated all affected drivers in this PR, and made a separate PR to update tools (apache/nuttx-apps#3061)

  • Is new feature added? NO Is existing feature changed? YES
  • Impact on build: NO
  • Impact on hardware: NO. The setting programmed on the HW is unaffected, only how this is represented on user space does.
  • Impact on documentation: YES, but minimal. The affected IOCTLs are Nuttx-specific, and the data stuctures providing the info (and thus determining the units) were only described on header files, which the patch updates (including such comments)
  • Impact on security: NO
  • Impact on compatibility: YES, this makes the units used for bitrate in SocketCAN bps, which are the same units used by Linux to perform the same operation using netlink.

Testing

Tested using slcan to check bitrate is properly set (CAN message still arrive properly) after this change.

This makes units compatible with what Linux uses for SocketCAN bitrate
and allows representing usual Single Wire Can (SAE J2411) usual bitrates
of 33333 and 83333 bps.

Signed-off-by: Carlos Sanchez <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Apr 16, 2025
csanchezdll added a commit to csanchezdll/nuttx-apps that referenced this pull request Apr 16, 2025
This matches PR apache/nuttx#16225 in Nuttx where
units for SocketCAN bitrate ioctls were changed.
@nuttxpr
Copy link

nuttxpr commented Apr 16, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown of what's missing:

  • Summary: Lacks details on what functional part of the code is changed and how the change works. Saying "makes units compatible" is vague. Which units? How are they made compatible? The reference to the tools PR (XXX) needs a proper link.

  • Impact: The impact section is severely incomplete. It only addresses user impact. It needs to address all other impact points:

    • Is new feature added? Is existing feature changed? (Yes, existing feature changed)
    • Impact on build: NO/YES and explanation.
    • Impact on hardware: NO/YES and explanation.
    • Impact on documentation: NO/YES and explanation (critical since the user impact mentions changes are needed).
    • Impact on security: NO/YES and explanation.
    • Impact on compatibility: NO/YES and explanation.
    • Anything else to consider?
  • Testing: While testing logs are included placeholders, the build host and target details are missing. Need to specify the OS, CPU, compiler for the host, and the architecture and board configuration for the target. Also, simply stating "CAN message still arrive properly" is insufficient. More detailed logs demonstrating the bitrate change are necessary. For instance, logs from slcan showing the configured bitrate would be beneficial.

In short, the PR needs significant expansion to meet the NuttX requirements. It needs to be more specific and comprehensive in all sections.

@csanchezdll csanchezdll marked this pull request as ready for review April 16, 2025 11:07
@acassis acassis added the breaking change This change requires a mitigation entry in the release notes. label Apr 16, 2025
@acassis
Copy link
Contributor

acassis commented Apr 16, 2025

Added "breaking change" for compatibility

@acassis acassis merged commit d976c66 into apache:master Apr 16, 2025
39 checks passed
xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request Apr 17, 2025
This matches PR apache/nuttx#16225 in Nuttx where
units for SocketCAN bitrate ioctls were changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture breaking change This change requires a mitigation entry in the release notes. 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.

4 participants