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

Ouverte
  • Quelque chose ne va pas ?
#11,834 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub

Les mainteneurs répondent en général sous 1 jour

Personne n'a encore pris cette issue.

Évaluation

Difficulté
5/5
Temps estimé
Plus d'une semaine
Accessibilité débutants
25/100
Type d'issue
Bug
Clarté
Plutôt claire
Activité
Active
Stack technique
c
Domaine
embedded-iot

Piste de recherche

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.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

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)
Langage dominant
C
Étoiles
4.6k
Forks
1.4k
Merge moyen
1 j 1 h
PR mergées (30 j)
208

Préparer son environnement

Ce projet ne fournit ni conteneur de développement, ni Dockerfile, ni guide de contribution : l'installation est à votre charge. Commencez par son README, et consultez notre guide de la première contribution pour les étapes générales.

Par où commencer

  1. Lisez l'issue en entier, puis le guide de contribution du projet.
  2. Signalez en commentaire que vous la prenez — cela évite que deux personnes fassent le même travail.
  3. Forkez le dépôt et travaillez sur une branche.
  4. Ouvrez une pull request qui référence le numéro de l'issue.

Autres issues de adafruit/circuitpython

Toutes les issues de adafruit/circuitpython

Issues similaires

Plus d'issues C

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.