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

Aberta
  • Algo errado?
#11,834 0 comentários 0 reações 0 responsáveis Ver no GitHub

Mantenedores costumam responder em até 1 dia

Ninguém assumiu esta issue ainda.

Avaliação

Dificuldade
5/5
Tempo estimado
Mais de uma semana
Facilidade para iniciantes
25/100
Tipo de issue
Bug
Clareza
Razoavelmente clara
Status de atividade
Ativa
Stack de tecnologia
c
Domínio
embedded-iot

Direção de pesquisa

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.

Escrita pelo modelo de indexação a partir do texto da issue.

Descrição

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)
Linguagem predominante
C
Estrelas
4.6k
Forks
1.4k
Merge médio
1d 1h
PRs com merge (30d)
208

Preparar o ambiente

Este projeto não oferece contêiner de desenvolvimento, Dockerfile nem guia de contribuição, então a configuração fica por sua conta: comece pelo README e veja nosso guia da primeira contribuição para os passos gerais.

Primeiros passos

  1. Leia a issue inteira e depois o guia de contribuição do projeto.
  2. Comente na issue dizendo que vai assumir — evita que duas pessoas façam o mesmo trabalho.
  3. Faça um fork do repositório e trabalhe em uma branch.
  4. Abra um pull request que referencie o número da issue.

Mais de adafruit/circuitpython

Todas as issues de adafruit/circuitpython

Issues semelhantes

Mais issues de C

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.