Skip to content

Conversation

@csanchezdll
Copy link
Contributor

Summary

A recent change (apache/nuttx#16199) has made the bitrate setting no longer bring the interface up. Moreover, it is now no longer possible to change bitrate of a CAN interface if it is up. Therefore, slcan needs to bring the interface down to change it. Fortunately, it already had commands to open and close the interface which map nicely to this.

Impact

slcan would be unable to change the interface bitrate after the aforementioned (already merged) Nuttx patch, without this change.

Testing

Tested using a second serial line (/dev/gsmtty2) and CAN interface (named can_6_14) on a custom board. The CAN bus is attacked using an adapter talking socketcand protocol:

< hi >< can_6_14 B 1000000 -1 -1 -1 -1 -1 -1 -1 >
< ok >< open can_6_14 >
< ok >< send 123 0 >

Before this patch (and with Nuttx updated so the patch there is applied) changing bitrate no longer brings the interface up, so no frames can be seen. After this patch, I can explicitly bring the interface up with the "O" command and see the traffic:

nsh> slcan can_6_14 /dev/gsmtty2
[01/01/70 00:00:28.620152] Starting slcan on NuttX
[01/01/70 00:00:28.620274] slcanBus
[01/01/70 00:00:28.620427] CAN socket open
[01/01/70 00:00:28.620518] Serial interface open /dev/gsmtty2
[01/01/70 00:00:34.440030] set speed 1000000
[01/01/70 00:00:53.280061] Open interface
[01/01/70 00:01:03.243936] R1, Id:0x123

I also checked bringing the interface down with "C" stops receiving frames.

I had to make another small fix to slcan.c (in a separate commit also in this patchset) because it originally only supported CAN interface names of up to 4 characters.

@csanchezdll csanchezdll marked this pull request as ready for review April 15, 2025 17:19
@nuttxpr
Copy link

nuttxpr commented Apr 15, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be more explicit.

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change, and links it to a prior NuttX PR.
  • Impact on User: Clearly explains the user impact (inability to change bitrate without the fix).
  • Testing: Provides a reasonable test setup and logs demonstrating the issue before and after the fix. The additional fix for interface names is also mentioned.

Areas for Improvement:

  • Summary: While the related NuttX PR is linked, a related issue (if one exists) should also be linked. If not, state that there isn't a related issue.
  • Impact: While the user impact is clear, other impact sections are marked "NO" without explanation. Even if there's no impact, explicitly stating "NO - No change to the build process" is better for clarity. Be very explicit.
  • Impact - Compatibility: The summary mentions a backward compatibility issue with a prior NuttX change. This should be explicitly called out in the "Impact on compatibility" section. Is this change backward compatible with previous versions of slcan?
  • Testing: While the test setup is described, the host environment isn't fully specified (OS, compiler, etc.). While can_6_14 is mentioned as a CAN interface on a custom board, more information on the board and the target architecture would be helpful. For instance: "Target: ARM, custom board (STM32F4xx based) with a CAN controller". Also specify the NuttX version being tested against.

Example of Improved Impact Section:

## Impact

* Is new feature added? NO
* Is existing feature changed? YES, the bitrate setting logic is modified to accommodate a recent change in the NuttX CAN driver.
* Impact on user (will user need to adapt to change)? NO.  Users who were previously using the bitrate setting feature of slcan will find it working correctly again.
* Impact on build (will build process change)? NO - No changes to the build process.
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO - No changes to hardware or drivers.  This change affects the slcan application's interaction with the CAN driver.
* Impact on documentation (is update required / provided)? NO -  No documentation updates required.
* Impact on security (any sort of implications)? NO - No security implications.
* Impact on compatibility (backward/forward/interoperability)? YES -  This change restores compatibility with a recent change in NuttX (https://github.com/apache/nuttx/pull/16199) that broke the bitrate setting functionality in slcan. This change is backward compatible with previous versions of slcan that were working correctly before the NuttX change.
* Anything else to consider?  The maximum length of the CAN interface name supported by slcan has been increased (addressed in a separate commit).

By being more thorough and explicit in the Impact and Testing sections, the PR will be easier to review and merge.

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.

Thank you @csanchezdll :-)

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.

Whoops, some code formatting issues, please fix (see https://nuttx.apache.org/docs/latest/contributing/coding_style.html). Thanks :-)

…racters.

Signed-off-by: Carlos Sanchez <carlossanchez@geotab.com>
@csanchezdll csanchezdll force-pushed the SIOCSCANBITRATE_no_ifup branch from 81610c3 to 670badf Compare April 16, 2025 07:50
@csanchezdll
Copy link
Contributor Author

I fixed the format errors and used checkpatch from the nuttx repo.

I found the initialization of ifr = {0} that I had fails because format rules want the braces on separate lines. Too much code for little meaning, so I remove the initialization altogether and used = instead of |= when assigning flags. The result is the same.

A recent change (apache/nuttx#16199) has made the bitrate setting no
longer bring the interface up. Moreover, it is now no longer possible to change bitrate
of a CAN interface if it is up. Therefore, slcan needs to bring the interface down
to change it. Fortunately, it already had commands to open and close the interface
which map nicely to this.

Signed-off-by: Carlos Sanchez <carlossanchez@geotab.com>
@csanchezdll csanchezdll force-pushed the SIOCSCANBITRATE_no_ifup branch from 670badf to a77b2d8 Compare April 23, 2025 09:14
@xiaoxiang781216 xiaoxiang781216 merged commit f94bccf into apache:master Apr 23, 2025
39 checks passed
@csanchezdll csanchezdll deleted the SIOCSCANBITRATE_no_ifup branch July 3, 2025 07:19
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.

6 participants