stm32: Refactor pyb.CAN implementation, add tests. #15989

projectgus · 2024-10-10T04:23:24Z

Summary

I've been gradually working on stm32 as the first implementation of the new CAN API (#12337), and first step was to refactor the low-level part of the pyb.CAN driver to be reusable from machine.CAN as well. In the course of doing this I also wrote some simple automated tests for pyb.CAN.

After doing the refactors and adding the tests I discovered some small bugs in various STM32 boards. This PR contains bugfix commits also submitted in #16543, #16545, #16546, #16547. Only the top two commits in this branch are unique to this PR (but it's difficult to run the tests without the other fixes).

can.h API changes (only relevant for anyone writing C code against this API):

Peripheral argument is now CAN_HandleTypeDef * instead of pyb_can_obj_t *, to decouple the two layers.
Adds a new can_rx_fifo_t enum for the RX FIFO selection. The numeric value of this enum is the same as the previous values used by bxCAN (0, 1), but they are different to the enums previously used by FDCAN. The FDCAN ST HAL enums are buffer offset values but these are now moved into the implementation and are no longer exposed in the API.
Responsibility for re-enabling RX interrupts has been moved up to the caller of can_receive(). See pyb_can.c for the necessary code.

As part of the refactoring some pyb.CAN behaviour changed on FDCAN, to make it the same as bxCAN:

FDCAN peripherals no longer send rx callbacks with ids 3,4,5 when the bus error state changes (this behaviour wasn't documented).
FDCAN peripherals now increment the error counters the same as bxCAN-based peripherals.

Testing

Ran the new multi-tests via command line:

./run-multitests.py -i pyb:/dev/ttyACM0 -i pyb:/dev/ttyACM1 -p2 multi_pyb_can/*.py

Tests pass on various pairings of the following boards:

PyBoard V1.1 (bxCAN).
NUCLEO_H723ZG board (FDCAN).
ARDUINO_NICLA_VISION (STM32H747, FDCAN)
NUCLEO_G474RE board (FDCAN, but G series has different peripheral design to H5+H7).

Trade-offs and Alternatives

I think the only real alternative would have been to ignore the existing pyb.CAN driver and make the STM32 machine.CAN driver from scratch. Given how much refactoring was needed here then that does look like a somewhat appealing alternative in hindsight, especially as I'm not done yet! However, this way the code size will be smaller for as long as both pyb.CAN and the future machine.CAN drivers are included in MicroPython.

Follow-up work (not in this PR)

Add multi-tests around CAN-FD mode.
Finish the stm32 machine.CAN implementation based on this lower layer.

codecov · 2024-10-10T04:26:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (96ce08e) to head (dbda43b).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #15989   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21877    21877           
=======================================
  Hits        21558    21558           
  Misses        319      319

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

projectgus · 2024-10-10T04:29:32Z

@iabdalkader @H-Grobben It's been a while, but this PR touches a bunch of driver code you've both worked on. Would appreciate any comments if you have the time.

github-actions · 2024-10-10T04:35:47Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +48 +0.012% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

projectgus · 2024-10-10T04:43:25Z

stm32: +0 +0.000% PYBV10

🤨 It seems very coincidental that this PR refactors so much code and ends up with exactly the same binary size...

iabdalkader · 2024-10-15T07:17:49Z

@iabdalkader @H-Grobben It's been a while, but this PR touches a bunch of driver code you've both worked on. Would appreciate any comments if you have the time.

It's been years since I worked on FDCAN and CAN, so I don't remember much, but I'll try to take a look as soon as I have some free time. @kwagyeman is very interested in CAN support he might also want to take a look.

kwagyeman · 2024-10-15T14:35:32Z

@projectgus - I'll update my PR for CAN on the IMX after an API is approved and merged.

projectgus · 2024-10-16T03:45:35Z

@projectgus - I'll update my PR for CAN on the IMX after an API is approved and merged.

Sounds great. Appreciate your patience with the CAN support, @kwagyeman.

dpgeorge · 2024-12-19T00:35:29Z

I didn't get time for a full review of this yet, but one thing I suggest is renaming the multi_pyb_can directory to just multi_pyb. It can potentially include tests for other thing in the pyb module in the future.

projectgus · 2025-01-07T06:54:01Z

I didn't get time for a full review of this yet, but one thing I suggest is renaming the multi_pyb_can directory to just multi_pyb. It can potentially include tests for other thing in the pyb module in the future.

I can change this, but I wanted to check you're sure about it? From a usability perspective, it seems fiddly: Once other tests are added, it'd be necessary to run ./run-multitests.py multi_pyb/can_*.py most of the time as the PyBoard CAN tests require CAN transceivers connected to the boards in the test. Other pyb module tests may have very different environment requirements.

H-Grobben · 2025-02-05T15:04:30Z

User modules can not access the 'pyb_can_obj_t' struct if it is defined in the pyb_can.c file. Is the struct also defined as a machine.CAN struct, in a .h file, which then be used by user modules?

If it's put in the pyb_can.h file, it is still usable.

I will try to review the code and run some tests on the can changes if I can find the time.

-- update: need more time, but initial test locks up the micropython on the G4, with our custom c module, with this PR

projectgus · 2025-02-10T23:57:09Z

Thanks for taking a look, @H-Grobben.

User modules can not access the 'pyb_can_obj_t' struct if it is defined in the pyb_can.c file.

Ah, sorry - I actually hadn't thought of the use case that a user C module might be using pyb_can_obj_t. I'll move it to pyb_can.h to minimise headaches on upgrade.

Is the struct also defined as a machine.CAN struct, in a .h file, which then be used by user modules?

Everything in pyb_can.* is to-be-eventually-deprecated code for pyb module compatibility. The new machine.CAN driver is still pending (unfortunately), but it will have a different implementation that uses can.h but not pyb_can.h.

I don't think any of the current machine module hardware drivers expose a C interface, so probably the new CAN driver won't do this either (apart from the new can.h). Can explain more about your user C module's use case, please? Is the code published anywhere?

-- update: need more time, but initial test locks up the micropython on the G4, with our custom c module, with this PR

It wouldn't surprise me if there are other subtle behaviour changes that impact a custom C module calling into pyb module functions. If you figure out what's happening then I'm happy to try and tweak things to keep compatibility intact.

kwagyeman · 2025-02-10T23:59:05Z

@projectgus - Is CAN for the machine module ever going to be completed?

Just providing a gentle push :)

projectgus · 2025-02-11T00:18:14Z

@projectgus - Is CAN for the machine module ever going to be completed?

I know it's frustrating, I'm sorry it's dragging so long. I have not forgotten about it, but there's been a lot of other things to take care of as well. There has been some progress behind the scenes, hopefully will be visible in a PR draft soon.

projectgus · 2025-02-12T02:44:57Z

@H-Grobben Have rebased this PR and moved the pyb_can_obj_t struct into pyb_can.h

projectgus · 2025-02-18T23:28:21Z

(Rebased, no other changes.)

dpgeorge · 2025-02-21T03:20:39Z

I have run the tests from this PR on PYBV10+PYBV10 and PYBV10+PYBD_SF6 (so just standard CAN, not FDCAN). They pass before the refactoring in this PR and with this PR. Very good!

I do like how the can.h API is now operate on CAN_HandleTypeDef* rather than pyb_can_obj_t*. That's a good refactoring.

ports/stm32/can.c

ports/stm32/pyb_can.c

dpgeorge

I've now done a full review of this PR. Apart from the above comments, it looks good.

projectgus · 2025-02-26T03:20:18Z

@H-Grobben I've now noted two C API changes in the PR description above, it may help you debug your user C module.

projectgus · 2025-02-26T05:29:49Z

Marked as draft again as I need to add a bit more functionality to the lower layer for machine.CAN to work.

projectgus · 2025-03-04T03:22:22Z

Marked as draft again as I need to add a bit more functionality to the lower layer for machine.CAN to work.

Actually this is probably OK, can always do a second refactor PR if needed.

IhorNehrutsa · 2025-03-13T13:26:47Z

ports/stm32/pyb_can.h

+    uint16_t num_error_warning;
+    uint16_t num_error_passive;
+    uint16_t num_bus_off;


Is it necessary to count hardware errors?
It may be enough to perform bus recovering inside the driver.

See RM0008 Reference manual 24.7.6 Error management
See https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/twai.html#error-states-and-counters

This is useful for the user to get an idea of if/when the bus is failing. They can access these "performance counters" via can.info().

(This is part of the Python API of pyb.CAN, so can't really be changed.)

This is necessary for the machine.CAN implementation to use the same low-level functions. Includes some refactoring around FIFO selection as there was a footgun where CAN_FIFO0/1 are 0/1 but FDCAN_RX_FIFO0/1 are not. Added an explicit type for non-hardware-specific FIFO numbering. Also moved responsibility for re-enabling CAN receive interrupts into the higher layer (pyb_can.c layer) after calling can_receive(). Also includes this behaviour change for FDCAN boards: - Fix for boards with FDCAN not updating error status counters (num_error_warning, num_error_passive, num_bus_off). These are now updated the same as on boards with CAN Classic controllers, as documented. - Previously FDCAN boards would trigger the RX callback function on error events instead (passing undocumented irq numbers 3, 4, 5). This behaviour has been removed in favour of the documented behaviour of updating the status counters. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>

Currently only classic CAN, but tests run on both the stm32 classic CAN controller and the FD-CAN controller with the same results. Signed-off-by: Angus Gratton <angus@redyak.com.au>

dpgeorge

I retested on PYBV10+PYBV10. The new multitests still pass.

projectgus added port-stm32 drivers labels Oct 10, 2024

projectgus requested a review from dpgeorge October 10, 2024 04:23

projectgus force-pushed the refactor/stm32_can branch from c985e46 to cbc443b Compare October 10, 2024 04:28

This was referenced Jan 8, 2025

stm32: Support 'FDCAN' in board pin CSVs. #16543

Merged

stm32/can: Fix clearing filters on CAN3 (bxCAN). #16545

Merged

stm32/fdcan: Fix extended CAN ID filtering for stm32g4. #16546

Merged

stm32: Fix ARDUINO_NICLA_VISION CAN pins. #16547

Merged

This comment was marked as outdated.

Sign in to view

projectgus force-pushed the refactor/stm32_can branch from cbc443b to 4ff44ce Compare January 8, 2025 02:31

projectgus force-pushed the refactor/stm32_can branch from 4ff44ce to a9b9974 Compare January 17, 2025 03:31

projectgus changed the title ~~stm32: Refactor pyb.CAN implementation, add tests, fix some related bugs.~~ stm32: Refactor pyb.CAN implementation, add tests. Jan 17, 2025

projectgus force-pushed the refactor/stm32_can branch from a9b9974 to 67cd5b6 Compare February 12, 2025 02:43

projectgus force-pushed the refactor/stm32_can branch from 67cd5b6 to a038b9b Compare February 18, 2025 23:27

dpgeorge reviewed Feb 21, 2025

View reviewed changes

ports/stm32/can.c Show resolved Hide resolved

dpgeorge reviewed Feb 21, 2025

View reviewed changes

ports/stm32/can.c Outdated Show resolved Hide resolved

dpgeorge reviewed Feb 21, 2025

View reviewed changes

ports/stm32/pyb_can.c Outdated Show resolved Hide resolved

dpgeorge reviewed Feb 21, 2025

View reviewed changes

projectgus force-pushed the refactor/stm32_can branch 3 times, most recently from 6b72c1c to ea09d2f Compare February 26, 2025 03:16

projectgus marked this pull request as draft February 26, 2025 05:29

projectgus marked this pull request as ready for review March 4, 2025 03:21

IhorNehrutsa reviewed Mar 13, 2025

View reviewed changes

projectgus added 2 commits March 14, 2025 14:52

tests/multi_pyb_can: Add multitests for pyboard CAN controller.

dbda43b

Currently only classic CAN, but tests run on both the stm32 classic CAN controller and the FD-CAN controller with the same results. Signed-off-by: Angus Gratton <angus@redyak.com.au>

dpgeorge force-pushed the refactor/stm32_can branch from ea09d2f to dbda43b Compare March 14, 2025 03:53

dpgeorge approved these changes Mar 14, 2025

View reviewed changes

dpgeorge merged commit dbda43b into micropython:master Mar 14, 2025
31 checks passed

IhorNehrutsa mentioned this pull request Mar 14, 2025

docs: Add machine.CAN low-level API docs. #13149

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

stm32: Refactor pyb.CAN implementation, add tests. #15989

stm32: Refactor pyb.CAN implementation, add tests. #15989

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stm32: Refactor pyb.CAN implementation, add tests. #15989

stm32: Refactor pyb.CAN implementation, add tests. #15989

Uh oh!

Conversation

Uh oh!

Summary

Testing

Trade-offs and Alternatives

Follow-up work (not in this PR)

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!