Skip to content

stm32: Add machine.CAN implementation - #35

Open
mattytrentini wants to merge 5 commits into
masterfrom
feature/machine_can
Open

stm32: Add machine.CAN implementation#35
mattytrentini wants to merge 5 commits into
masterfrom
feature/machine_can

Conversation

@mattytrentini

Copy link
Copy Markdown
Owner

Summary

Add a machine.CAN implementation for STM32, along with supporting infrastructure changes:

  • py/objlist, stm32, esp32: Add helpers for creating/ensuring list args
  • stm32: Extract can_get_state() function, reuse from pyb.CAN
  • extmod, docs: Add generic machine.CAN helpers & documentation
  • stm32: Implement index-aware STM32G4 FDCAN HAL TX functions
  • stm32: Full machine.CAN implementation

Test plan

  • Build for STM32 targets with CAN support (e.g. PYBD_SF6, NUCLEO_G474RE)
  • Verify pyb.CAN still works as expected (no regressions)
  • Test machine.CAN basic operations: init, send, recv, filters
  • Test on STM32G4 FDCAN targets specifically
  • Review docs render correctly

Simplifies the pattern of an optional arg which can be a list of at
least a certain length, otherwise one is lazily initialised.

Modify pyb.CAN and ESP-NOW APIs to use the helper. Note this changes
the return type of pyb.CAN.recv() from tuple to list.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
API is different to the original machine.CAN proposal, as numerous
shortcomings were found during initial implementation.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
These are oddly missing from the STM32G4 HAL, but the
reference manual describes being able to use them and
the implementations seem to work as expected.

Note that unlike STM32H7 it doesn't seem like we must use this approach,
because HAL_FDCAN_AddMessageToTxFifoQ() does seem to not have the issues
with priority inversion seen on the H7. However it's simpler to use the
same API for both...

Signed-off-by: Angus Gratton <angus@redyak.com.au>
Signed-off-by: Angus Gratton <angus@redyak.com.au>
Implemented according to API docs in a parent comment.

Adds new multi_extmod/machine_can_* tests which pass when testing between
NUCLEO_G474RE, NUCLEO_H723ZG and PYBDV11.

This work was mostly funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@mattytrentini

Copy link
Copy Markdown
Owner Author

/review

@mpy-reviewer mpy-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR is too large to review properly in one sitting — it mixes py/objlist helpers, a new extmod CAN framework, STM32 bxCAN and FDCAN port implementations, a pyb.CAN refactor, and unrelated ESP8266/ESP32 ESPNow changes. These should be separate PRs. The Testing section has only unchecked boxes; please report what was actually tested and on which boards before asking for review. Several correctness bugs below need fixing first.

Comment thread ports/stm32/machine_can.c
void machine_can_irq_handler(uint can_id, can_int_t interrupt) {
assert(can_id > 0);
machine_can_obj_t *self = MP_STATE_PORT(machine_can_objs)[can_id - 1];
struct machine_can_port *port = self->port;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

self->port is dereferenced here before the null check on self four lines below. If self is NULL (no machine.CAN object allocated for this peripheral), this crashes in ISR context.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a correct analysis.

In practice it probably won't hit the case self == NULL, but it would be good to fix this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, this bug does affect FDCAN devices (eg STM32H7) running existing tests/ports/stm32/can.py tests and needs to be fixed.

Comment thread extmod/machine_can.c

machine_can_obj_t *self = MP_STATE_PORT(machine_can_objs)[can_idx];
if (self == NULL) {
self = mp_obj_malloc(machine_can_obj_t, &machine_can_type);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mp_obj_malloc doesn't zero-initialise the struct. On the first call self->port is garbage, so if (self->port != NULL) in machine_can_init_helper is likely true, calling machine_can_port_deinit with an uninitialised port pointer. Add self->port = NULL; self->mp_irq_obj = NULL; (and any other pointer/flag fields) after the malloc.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a reasonable analysis. But since MICROPY_GC_CONSERVATIVE_CLEAR is always enabled, mp_obj_malloc() does in fact zero out the new memory. Probably a lot of code in MicroPython relies on MICROPY_GC_CONSERVATIVE_CLEAR, but that's something we need to address separately.

Comment thread ports/stm32/machine_can.c
// associated with only one FIFO.
.FilterFIFOAssignment = filter_idx % 2,
// CAN2 needs BankNumber set
.BankNumber = self->can_idx == 1 ? CAN_HW_MAX_FILTER : 0,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For CAN1 (can_idx == 0) this sets BankNumber = 0, which assigns all 28 filter banks to CAN2 and leaves CAN1 with zero filter banks. Should be CAN_HW_MAX_FILTER (14) for CAN1 and 0 for CAN2... but that's also wrong: the HAL BankNumber field is the start bank for CAN2, so CAN1 always needs BankNumber = CAN_HW_MAX_FILTER regardless of which instance is being configured.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This analysis looks correct. But testing shows that it doesn't really matter what BankNumber is set to, because this driver only supports CAN1.

This needs to eventually be fixed when CAN2 is supported.

Comment thread ports/stm32/pyb_can.c
if (!can_init(&self->can, self->can_id, args[ARG_mode].u_int, args[ARG_prescaler].u_int, args[ARG_sjw].u_int,
args[ARG_bs1].u_int, args[ARG_bs2].u_int, args[ARG_auto_restart].u_bool)) {
// Note: undoing the 4-bit right shift from the constant definitions
if (!can_init(&self->can, self->can_id, CAN_TX_FIFO, args[ARG_mode].u_int << 4, args[ARG_prescaler].u_int, args[ARG_sjw].u_int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For FDCAN targets, pyb.CAN.LOOPBACK etc. are stored as raw FDCAN_MODE_* values (no >> 4 shift — see the #if MICROPY_HW_ENABLE_FDCAN branch of the constants table). Applying << 4 here produces the wrong HAL mode value. This shift must be conditional on !MICROPY_HW_ENABLE_FDCAN.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This analysis is correct, and this bug needs to be fixed. Eg doing pyb.CAN(1, pyb.CAN.LOOPBACK) fails on FDCAN.

Comment thread ports/stm32/pyb_can.c
if (ret_obj == mp_const_none) {
ret_obj = mp_obj_new_tuple(5, NULL);
items = ((mp_obj_tuple_t *)MP_OBJ_TO_PTR(ret_obj))->items;
mp_obj_list_t *list = mp_obj_list_optional_arg(args[ARG_list].u_obj, 5);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This changes pyb.CAN.recv() to return a list when called without the optional list argument, where it previously returned a tuple. That's an undiscussed API break for existing user code.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was discussed, the docs were updated and it was mentioned in the commit message.

It could possibly warrant a note in the docs that it was changed, but I think it's probably enough to just mention it in the release notes.

Comment thread extmod/extmod.mk
SRC_EXTMOD_C += \
extmod/machine_adc.c \
extmod/machine_adc_block.c \
extmod/machine_can.c \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

machine_can.c is out of order — should come after machine_bitstream.c (alphabetically 'bitstream' < 'can').

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Indeed it is.

Comment thread docs/library/machine.rst
machine.I2C.rst
machine.I2CTarget.rst
machine.I2S.rst
machine.CAN.rst

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

machine.CAN.rst should be before machine.I2C.rst (alphabetical order).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The list here isn't alphabetical.

Comment thread ports/stm32/machine_can.c
*
* The MIT License (MIT)
*
* Copyright (c) 2020-2021 Damien P. George

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Wrong copyright: this is new code by Angus Gratton, not Damien P. George.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, this could be changed.

Comment thread extmod/machine_can_port.h
#define MP_CAN_IRQ_TX_FAILED (1 << 2)
#define MP_CAN_IRQ_STATE (1 << 3)

// Transmit buffer incex is encoded into the irq().flags() response for MP_CAN_IRQ_TX

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"incex" → "index"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes.

Comment thread extmod/machine_can.c
}
MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(machine_can_get_timings_obj, 1, 2, machine_can_get_timings);


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove extra blank line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes.

@github-actions

Copy link
Copy Markdown

Code size report:

Reference:  README: Add a section describing MicroPython's values. [4625f97]
Comparison: stm32: Add machine.CAN implementation. [merge of fc680e2]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32: +4448 +1.128% PYBV10[incl +4(bss)]
      esp32:   -28 -0.002% ESP32_GENERIC[incl -32(data)]
     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

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