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

Offen
  • Stimmt etwas nicht?
#11,834 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Maintainer antworten meist innerhalb von 1 Tag

Dieses Issue hat noch niemand übernommen.

Bewertung

Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Anfängerfreundlichkeit
25/100
Issue-Typ
Bug
Klarheit
Größtenteils klar
Aktivitätsstatus
Aktiv
Tech-Stack
c
Bereich
embedded-iot

Rechercherichtung

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.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

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)
Vorherrschende Sprache
C
Sterne
4.6k
Forks
1.4k
Ø Merge
1 T. 1 Std.
Gemergte PRs (30 T.)
208

Entwicklungsumgebung

Dieses Projekt bietet weder Dev-Container noch Dockerfile noch Beitragsleitfaden – die Einrichtung liegt bei Ihnen. Beginnen Sie mit der README; die allgemeinen Schritte stehen in unserem Leitfaden für den ersten Beitrag.

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus adafruit/circuitpython

Alle Issues in adafruit/circuitpython

Ähnliche Issues

Weitere Issues zu C

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.