analog: pin reset and busio cleanup bugs found in an audit

Abierto
  • ¿Algo no está bien?
#11,834 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
5/5
Tiempo estimado
Más de una semana
Aptitud para principiantes
25/100
Tipo de issue
Error
Claridad
Bastante claro
Estado de actividad
Activo
Stack tecnológico
c
Área
embedded-iot

Línea de trabajo

Start with the listed cleanup paths in microcontroller/Pin.c, busio/I2C.c, busio/SPI.c, busio/UART.c, and peripherals/max32690/max32_uart.c and max32_spi.c; compare pin handling with digitalio/DigitalInOut.c. The report is based on reading code at commit 7cda456884 and notes that fixes cannot currently be tested on analog hardware. Done means correcting the listed reset, claim, deinit, UART context and NULL pin issues, with validation on suitable hardware.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

bug

Claude wrote this:

An audit by Claude of cleanup paths across ports (follow-up to #8960) found these bugs in the analog port. They are recorded here and are not being worked on for now, because no analog board is available to test fixes on, and the pin helpers need a rewrite. Each was confirmed by reading the code on main (7cda456884); none was run on hardware. The port uses CIRCUITPY_BULK_RESET = 0, so finalisers are the only cleanup path, and DEBUG defaults to 1, so assert()s are live.

microcontroller/Pin.c

PIN(port, n) stores .mask = 1UL << n, but the pin helpers treat mask as the pin number in several places.

Line Problem
78 common_hal_reset_pin() passes pin->mask to reset_pin_number(), which shifts it again (1 << pin_pad, with pin_pad a uint8_t). Pins 0-4 reset pin 1, 2, 4, 8 or 16 of the same port; pins 8 and up reset pin 0; pins 5-7 shift by 32 or more, which is undefined. The pin being released is never reset.
49 reset_pin_number() does outen |= mask under the comment "Output mode disable GPIOn_OUTEN = 0", so the pin it resets becomes an output. With line 78, every deinit drives some other pin as an output.
85 common_hal_mcu_pin_claim() sets 1 << pin->mask, while common_hal_mcu_pin_is_free() tests pin->mask, so a claimed pin still reads as free and another pin reads as claimed.
17-85 claimed_pins[] is never cleared.
89 common_hal_mcu_pin_reset_number() uses pin_no & 32 where pin_no % 32 is meant.
26 pin_port > NUM_GPIO_PORTS should be >=.
63 common_hal_mcu_pin_number() returns port * 32 + mask in a uint8_t, using the mask, not the pin number.

Port 4 also needs the MXC_MCR handling that digitalio/DigitalInOut.c does, which reset_pin_number() doesn't.

Example: d = digitalio.DigitalInOut(<P0.08>); d.deinit() leaves P0.08 as it was and drives P0.00 as an output.

busio
File Problem Trigger
busio/I2C.c:68, 87, 105-113 i2c_active is set in construct and asserted clear, but never cleared, so the second I2C on a peripheral fails the assert(). i = busio.I2C(SCL, SDA); i.deinit(); busio.I2C(SCL, SDA)
busio/I2C.c:106 Deinit has no deinited check, so after a construct that raises in pinsToI2c(), the finaliser calls MXC_I2C_Shutdown(NULL). busio.I2C() with an invalid pin pair, then gc.collect().
busio/SPI.c:127-140 Deinit has no deinited check and doesn't clear spi_regs. After a failed construct the finaliser calls MXC_SPI_Shutdown(NULL); a deinited SPI's finaliser shuts down a newer SPI on the same peripheral. a = SPI(...); a.deinit(); b = SPI(...); a = None; gc.collect(): b stops.
busio/UART.c:292-298 deinited() tests a per-peripheral bit in uarts_active, not the object, so a failed or deinited UART looks live while another UART uses that peripheral; its deinit then shuts that UART down and fails assert(self->rx_pin && self->tx_pin) (line 271). a = UART(P); a.deinit(); b = UART(P); a.deinit()
busio/UART.c:247, 142, 352 The global context is set to the last UART constructed and never cleared; the RX callback and read() use context->ringbuf, not self, so with two UARTs one reads the other's data, and after context is collected the callback writes to freed memory. Two UARTs, read from the first.
busio/UART.c:309, 367 read() and write() set the uarts_active bit again, so they make a deinited UART look live.
busio/UART.c:172 assert() that receiver_buffer_size is a power of 2; the binding accepts 1-65535. busio.UART(..., receiver_buffer_size=100)
busio/UART.c:203-209 The rts/cts check and mp_arg_validate_int(bits, 8, ...) raise after MXC_UART_Init() and the pin claims, but before uarts_active is set, so deinit skips the cleanup. busio.UART(TX, RX, bits=7)
peripherals/max32690/max32_uart.c, max32_spi.c pinsToUart() and pinsToSpi() use every pin without a NULL check, but the bindings allow a single TX or RX, or a missing MOSI or MISO, so they could try to use a NULL pointer. busio.UART(tx=TX), busio.SPI(SCK, MOSI=MOSI)
Lenguaje dominante
C
Estrellas
4.6k
Forks
1.4k
Merge medio
1 d 1 h
PR fusionados (30 d)
208

Preparar el entorno

Este proyecto no incluye contenedor de desarrollo, Dockerfile ni guía de contribución, así que la configuración corre por tu cuenta: empieza por su README y consulta nuestra guía para la primera contribución para los pasos generales.

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de adafruit/circuitpython

Todos los issues de adafruit/circuitpython

Issues similares

Más issues de C

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.