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

Open
  • Is something wrong?
#11,834 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
25/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
c
Domain
embedded-iot

Research direction

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.

Written by the indexing model from the issue text.

Description

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)
Dominant language
C
Stars
4.6k
Forks
1.4k
Avg merge
1d 1h
Merged PRs (30d)
208

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from adafruit/circuitpython

All issues in adafruit/circuitpython

Similar issues

More C issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.