Skip to content

cpu/samd5x/periph_can: fix RX [backport 2025.01]#21184

Merged
maribu merged 1 commit intoRIOT-OS:2025.01-branchfrom
maribu:backport/2025.01/cpu/samd5x/can-fix-rx
Feb 3, 2025
Merged

cpu/samd5x/periph_can: fix RX [backport 2025.01]#21184
maribu merged 1 commit intoRIOT-OS:2025.01-branchfrom
maribu:backport/2025.01/cpu/samd5x/can-fix-rx

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Feb 3, 2025 •
edited by kfessel
Loading

Backport of #21181

Contribution description

CAN required CLK_CANx_APB and CLK_CANx_APB to be running and will not request any clock by itself. We can ensure both clocks to be running by preventing the MCU from entering IDLE state.

The SAMD5x/SAME5x Family Data Sheet says in Section "39.6.9 Sleep Mode Operation" says:

The CAN can be configured to operate in any idle sleep mode. The CAN
cannot operate in Standby sleep mode.

[...]

To leave low power mode, CLK_CANx_APB and GCLK_CANx must be active
before writing CCCR.CSR to '0'. The CAN will acknowledge this by
resetting CCCR.CSA = 0. Afterwards, the application can restart CAN
communication by resetting bit CCCR.INIT.

tl;dr: At most SAM0_PM_IDLE is allowed while not shutting down the CAN controller, but even that will pause communication (including RX).

Apparently, the CAN controller was never tested without also using the USB peripheral, which kept the clocks running as side effect.

Testing procedure

Set up USB CAN stick

$ sudo slcand /dev/ttyACM0 # <-- only needed for serial CAN adapters such as https://canable.io/ (or it's cheap clones from AliExpress)
$ sudo ip link set can0 up type can bitrate 500000

Sending to the SAME54-XPRO from Linux

$ cansend can0 '001#cafe'

Output of candump any

  can0  001   [3]  AB CD EF
  can0  001   [2]  CA FE

RIOT output with master

$ make BOARD=same54-xpro -C tests/drivers/candev flash term
[...]
   text	  data	   bss	   dec	   hex	filename
  20276	   128	 13432	 33836	  842c	/home/[email protected]/Repos/software/RIOT/master/tests/drivers/candev/bin/same54-xpro/tests_candev.elf
[...]
Debugger: ATMEL EDBG CMSIS-DAP ATML2748051800006032 03.25.01B6 (S)
Clock frequency: 16.0 MHz
Target: SAM E54P20A (Rev A)
Programming...... done.
Verification........................................... done.
Done flashing
[...]
2025-01-31 22:13:40,156 # main(): This is RIOT! (Version: 2025.04-devel-56-g775f59)
2025-01-31 22:13:40,158 # candev test application
2025-01-31 22:13:40,158 # 
2025-01-31 22:13:40,161 # Initializing CAN periph device
> send
2025-01-31 22:13:43,438 # send
> receive
2025-01-31 22:13:45,208 # receive
2025-01-31 22:13:45,210 # Reading from Rxbuf...

RIOT output with this PR

$ make BOARD=same54-xpro -C tests/drivers/candev flash term
[...]
2025-01-31 22:17:17,065 # main(): This is RIOT! (Version: 2025.04-devel-57-gd93f0-cpu/samd5x/can-fix-rx)
2025-01-31 22:17:17,067 # candev test application
2025-01-31 22:17:17,067 # 
2025-01-31 22:17:17,070 # Initializing CAN periph device
> send
2025-01-31 22:17:19,297 # send
> receive
2025-01-31 22:17:20,831 # receive
2025-01-31 22:17:20,833 # Reading from Rxbuf...
2025-01-31 22:17:22,548 # id: 1 dlc: hx data: 0xCA 0xFE

Conclusion

In both master and this PR sending from RIOT was picked up on the Linux side. But sending from Linux to RIOT was not picked up by RIOT in master, but with this PR.

Issues/PRs references

#21181

@maribu maribu requested review from benpicco and dylad as code owners February 3, 2025 09:34
@maribu maribu added Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 3, 2025
@maribu maribu mentioned this pull request Feb 3, 2025
Merged
@maribu maribu enabled auto-merge February 3, 2025 09:34
@riot-ci
Copy link
Copy Markdown

riot-ci commented Feb 3, 2025 •
edited
Loading

Murdock results

✔️ PASSED

97006a8 cpu/samd5x/periph_can: fix RX

Success Failures Total Runtime
10271 0 10271 09m:47s

Artifacts

MrKevinWeiss
MrKevinWeiss approved these changes Feb 3, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

ACK. I guess we add it and do a basic retest...

@maribu maribu added this pull request to the merge queue Feb 3, 2025
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Feb 3, 2025

I will also manually add it to the release notes

kfessel
kfessel approved these changes Feb 3, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

this is a backport and fixes a realy anoing bug and therefor should be moved in even though it has issues

@MrKevinWeiss MrKevinWeiss mentioned this pull request Feb 3, 2025
Merged
@maribu maribu removed this pull request from the merge queue due to a manual request Feb 3, 2025
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 3, 2025

I removed this from the merge queue for now. The issue is that Github merged an old state of the PR backported here. But this backport did indeed correctly pick the commit that should have been merged to master.

@maribu
cpu/samd5x/periph_can: fix RX
97006a8 
CAN required CLK_CANx_APB and CLK_CANx_APB to be running and will not
request any clock by itself. We can ensure both clocks to be running
by preventing the MCU from entering IDLE state.

The SAMD5x/SAME5x Family Data Sheet says in Section
"39.6.9 Sleep Mode Operation" says:

> The CAN can be configured to operate in any idle sleep mode. The CAN
> cannot operate in Standby sleep mode.
>
> [...]
>
> To leave low power mode, CLK_CANx_APB and GCLK_CANx must be active
> before writing CCCR.CSR to '0'. The CAN will acknowledge this by
> resetting CCCR.CSA = 0. Afterwards, the application can restart CAN
> communication by resetting bit CCCR.INIT.

tl;dr: At most SAM0_PM_IDLE is allowed while not shutting down the CAN
controller, but even that will pause communication (including RX).

Apparently, the CAN controller was never tested without also using the
USB peripheral, which kept the clocks running as side effect.
@maribu maribu force-pushed the backport/2025.01/cpu/samd5x/can-fix-rx branch from 1ea14e8 to 97006a8 Compare February 3, 2025 13:29
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 3, 2025

I updated this to the commit that actually got merged into master

@maribu maribu enabled auto-merge February 3, 2025 13:34
@maribu maribu added this pull request to the merge queue Feb 3, 2025
Merged via the queue into RIOT-OS:2025.01-branch with commit a3627b7 Feb 3, 2025
@mguetschow mguetschow added this to the Release 2025.01 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfessel kfessel kfessel approved these changes

@MrKevinWeiss MrKevinWeiss MrKevinWeiss approved these changes

@benpicco benpicco Awaiting requested review from benpicco benpicco is a code owner

@dylad dylad Awaiting requested review from dylad dylad is a code owner

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2025.01

Development

Successfully merging this pull request may close these issues.

5 participants

@maribu @riot-ci @MrKevinWeiss @kfessel @mguetschow