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.

Sign up for GitHub

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

Jump to bottom

drivers/can: add board_candev_set_power() hook #21203

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

drivers/can: add board_candev_set_power() hook #21203

wants to merge 10 commits into from
+146 −6

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 10, 2025

Contribution description

This provides a default (weak) implementation of

void board_candev_set_power(candev_t *dev, bool active);

that is a no-op, and adds calls to this function to each and every CAN controller driver. The idea is that a board can provide this, if it can power up and down the CAN transceivers, freeing applications from having to add board specific hacks to enable CAN transceivers.

In addition, the following two helpers are added:

can_t *candev2periph(candev_t *candev);
candev_mcp2515_t *candev2mcp2515(candev_t *candev);

These safely convert a candev_t pointer back to the underlying type, return NULL if the given candev_t * is not of the requested type. This aids in implementing board_candev_set_power(), as it provides a board with the means to figure out for which CAN controller the CAN transceiver needs to be enabled/disabled for.

Finally, the new board hook is added for the same54-xpro and the board specific hacks from tests/drivers/candev are dropped.

Testing procedure

$ make BOARD=same54-xpro flash term -C tests/drivers/candev 
make: Entering directory '/home/[email protected]/Repos/software/RIOT/master/tests/drivers/candev'
Building application "tests_candev" for "same54-xpro" with CPU "samd5x".

"make" -C /home/[email protected]/Repos/software/RIOT/master/pkg/cmsis/ 
"make" -C /home/[email protected]/Repos/software/RIOT/master/boards/common/init
"make" -C /home/[email protected]/Repos/software/RIOT/master/boards/same54-xpro
"make" -C /home/[email protected]/Repos/software/RIOT/master/core
"make" -C /home/[email protected]/Repos/software/RIOT/master/core/lib
"make" -C /home/[email protected]/Repos/software/RIOT/master/cpu/samd5x
"make" -C /home/[email protected]/Repos/software/RIOT/master/cpu/cortexm_common
"make" -C /home/[email protected]/Repos/software/RIOT/master/cpu/cortexm_common/periph
"make" -C /home/[email protected]/Repos/software/RIOT/master/cpu/sam0_common
"make" -C /home/[email protected]/Repos/software/RIOT/master/cpu/sam0_common/periph
"make" -C /home/[email protected]/Repos/software/RIOT/master/cpu/samd5x/periph
"make" -C /home/[email protected]/Repos/software/RIOT/master/drivers
"make" -C /home/[email protected]/Repos/software/RIOT/master/drivers/periph_common
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/auto_init
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/can
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/div
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/isrpipe
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/libc
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/malloc_thread_safe
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/memarray
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/newlib_syscalls_default
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/pm_layered
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/preprocessor
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/shell
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/stdio
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/stdio_uart
"make" -C /home/[email protected]/Repos/software/RIOT/master/sys/tsrb
   text	  data	   bss	   dec	   hex	filename
  20368	   128	 13432	 33928	  8488	/home/[email protected]/Repos/software/RIOT/master/tests/drivers/candev/bin/same54-xpro/tests_candev.elf
/home/[email protected]/Repos/software/RIOT/master/dist/tools/edbg/edbg.sh flash /home/[email protected]/Repos/software/RIOT/master/tests/drivers/candev/bin/same54-xpro/tests_candev.bin
### Flashing Target ###
Debugger: ATMEL EDBG CMSIS-DAP ATML2748051800006032 03.25.01B6 (S)
Clock frequency: 16.0 MHz
Target: SAM E54P20A (Rev A)
Verification.......................................
at address 0x4853 expected 0x35, read 0x33
Error: verification failed
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
/home/[email protected]/Repos/software/RIOT/master/dist/tools/pyterm/pyterm -p "/dev/ttyACM1" -b "115200" -ln "/tmp/[email protected]" -rn "2025-02-10_21.15.00-tests_candev-same54-xpro"  
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2025-02-10 21:15:00,457 # Connect to serial port /dev/ttyACM1
Welcome to pyterm!
Type '/exit' to exit.
send
2025-02-10 21:15:02,933 # send
> receive
2025-02-10 21:15:05,356 # receive
2025-02-10 21:15:05,358 # Reading from Rxbuf...
2025-02-10 21:15:08,966 # id: 1 dlc: hx data: 0xCA 0xFE 

$ candump any            
  can0  001   [3]  AB CD EF
  can0  001   [2]  CA FE

$ cansend can0 '001#cafe'

Issues/PRs references

None

@maribu
drivers/can: add board_candev_set_power() hook
c5cb729 
This provides a default (weak) implementation of

    void board_candev_set_power(candev_t *dev, bool active);

that is a no-op, and adds calls to this function to each and every CAN
controller driver. The idea is that a board can provide this, if it
can power up and down the CAN transceivers, freeing applications from
having to add board specific hacks to enable CAN transceivers.

In addition, the following two helpers are added:

    can_t *candev2periph(candev_t *candev);
    candev_mcp2515_t *candev2mcp2515(candev_t *candev);

These safely convert a `candev_t` pointer back to the underlying type,
return `NULL` if the given `candev_t *` is not of the requested type.
This aids in implementing `board_candev_set_power()`, as it provides
a board with the means to figure out for which CAN controller the
CAN transceiver needs to be enabled/disabled for.
@maribu
boards/same54-xpro: implement board_candev_set_power()
61f58f9
@maribu
fixup! boards/same54-xpro: implement board_candev_set_power()
9c5a750
@maribu
fixup! boards/same54-xpro: implement board_candev_set_power()
6c81919
@maribu
fixup! drivers/can: add board_candev_set_power() hook
876167b
@maribu
fixup! 9c5a750
4ac94f4
@maribu
fixup! boards/same54-xpro: implement board_candev_set_power()
4eeb26f
@maribu
tests/drivers/candev: drop same54-xpro hack
64b2ad6 
Now that the CAN transceiver is controlled by the board hook in the
CAN driver, we don't need the board specific hack any more.
@maribu maribu requested a review from kfessel February 10, 2025 20:16
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: boards Area: Board ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports Area: sys Area: System labels Feb 10, 2025
@maribu
Copy link
Member Author

maribu commented Feb 10, 2025

@gdoffe: Would you mind to take a look at this as well, since you just now worked on the STM32 CAN driver?

@maribu maribu added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 10, 2025
@riot-ci
Copy link

riot-ci commented Feb 10, 2025 •
edited
Loading

Murdock results

✔️ PASSED

c0ade8d Update sys/include/can/device.h

Success Failures Total Runtime
10271 0 10271 10m:30s

Artifacts

dylad
dylad reviewed Feb 11, 2025
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

I like the overall idea but I don't have the hardware to test.

boards/same54-xpro/board.c Outdated Show resolved Hide resolved
sys/include/can/device.h Outdated Show resolved Hide resolved
maribu and others added 2 commits February 11, 2025 10:39
@maribu @dylad
Update boards/same54-xpro/board.c
440c806 
Co-authored-by: Dylan Laduranty <[email protected]>
@maribu @dylad
Update sys/include/can/device.h
c0ade8d 
Co-authored-by: Dylan Laduranty <[email protected]>
@benpicco
Copy link
Contributor

benpicco commented Feb 11, 2025 •
edited
Loading

This reminds me a bit of can_trx_set_mode() - maybe that one is too generic for it's own good, but I prefer if we can just declare hardware in the board.h and don't have to write custom code for it.

@maribu maribu marked this pull request as draft February 11, 2025 14:06
@maribu
Copy link
Member Author

maribu commented Feb 11, 2025

This reminds me a bit of can_trx_set_mode()

Indeed, this is trying to solve the same problem. What a mess the CAN stack in RIOT is 😅

I guess the correct thing to do would be to hook up that instead.

but I prefer if we can just declare hardware in the board.h and don't have to write custom code for it.

That would be nice, but non-trivial to do.

Deep integration into the CAN driver by adding a some gpio_t enable_pin; and some bool enable_pin_active_low to the foocan_params_t would be an option to do that.

I wonder if the complexity can_trx is actually needed; especially since the only user of that interfaces uses normal and sleep mode.

@benpicco
Copy link
Contributor

I wonder if the complexity can_trx is actually needed; especially since the only user of that interfaces uses normal and sleep mode

I agree - the fact that it's so overengeneered that nobody is using it should be telling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad left review comments

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

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

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

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

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

@kfessel kfessel Awaiting requested review from kfessel

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework 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 Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @riot-ci @benpicco @dylad