Skip to content

Conversation

@Finwood
Copy link
Contributor

@Finwood Finwood commented Jul 24, 2025

The existing PCAN interface implementation disregarded the configured bitrate preferences, and used static magic-numbers instead. This PR changes the timing calculation by utilizing can.BitTimingFd.from_sample_point(), which leads to a cleaner implementation.

I am unsure regarding the target sample point location: my setup works well with 87.5%, and does not work with 75%, but that might be an edge case (low clock precision). Does anyone have recommendations on sample point location?

@Finwood Finwood force-pushed the fix-pcan-bitrate-calculation branch from 54b9783 to d5eb647 Compare July 24, 2025 12:55
@coveralls
Copy link

coveralls commented Jul 24, 2025

Coverage Status

coverage: 93.896% (+0.009%) from 93.887%
when pulling aa1b088 on Finwood:fix-pcan-bitrate-calculation
into 9514f24 on OpenCyphal:master.

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 3, 2025

I am unsure regarding the target sample point location: my setup works well with 87.5%, and does not work with 75%, but that might be an edge case (low clock precision). Does anyone have recommendations on sample point location?

Could be caused by long wiring, or large uncompensated transceiver delays, if we're talking about CAN FD. This might or might not be relevant: https://forum.opencyphal.org/t/on-can-bus-topology-and-termination/1685

EDIT: Sorry I cited an old document here; disregard that. I can only say that CiA 1301 recommends 80% for the arbitration phase, and for the data phase the value depends on the bitrate, going toward lower values with higher bit rates.

Here's a relevant chart from Holger Zeltwanger's presentation, although it's not CANopen-specific:

image

Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Please bump the patch version in __version__.py

Finwood added 3 commits August 4, 2025 08:21
Set the BRS bit for pythoncan transports when the interface operates in CAN FD mode.

Fixes OpenCyphal#368
@Finwood
Copy link
Contributor Author

Finwood commented Aug 12, 2025

@pavel-kirienko, is there anything that prevents this PR from being merged?

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 12, 2025

@Finwood I don't think so. A CI has failed for a non-code-related reason so I restarted it; I am going to enable auto-merge now. In the future it is best to request a new review explicitly so that I know when it's ready.

@pavel-kirienko pavel-kirienko merged commit d0f68e6 into OpenCyphal:master Aug 12, 2025
15 of 16 checks passed
@Finwood Finwood deleted the fix-pcan-bitrate-calculation branch August 12, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants