Skip to content

Commit b19a861

Browse files
Brandon-HurstBrandon-Hurst
committed
Fix error reporting on ports/analog BUSIO for PR 10413
- Use existing error messages from locale/circuitpython.pot for most error messages - Replace ConnectionError (networking specific) calls with others (usually RuntimeError) - Remove '\n' from mp error calls (since they add it already) - Remove some redundant validation code that repeats checks done by shared-bindings/common-hal/busio code. e.g. pin validation Signed-off-by: Brandon-Hurst <[email protected]>
1 parent 814047d commit b19a861

File tree

6 files changed

+39
-57
lines changed

6 files changed

+39
-57
lines changed

‎ports/analog/common-hal/busio/I2C.c‎

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
5252
// Check for NULL Pointers && valid I2C settings
5353
assert(self);
5454

55+
/* NOTE: The validate_obj_is_free_pin() calls in shared-bindings/busio/I2C.c
56+
will ensure that scl and sda are both pins and cannot be null
57+
ref: https://github.com/adafruit/circuitpython/pull/10413
58+
*/
59+
5560
// Assign I2C ID based on pins
5661
temp = pinsToI2c(sda, scl);
5762
if (temp == -1) {
@@ -65,25 +70,6 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
6570
assert((self->i2c_id >= 0) && (self->i2c_id < NUM_I2C));
6671
assert(!(i2c_active & (1 << self->i2c_id)));
6772

68-
// Init I2C as main / controller node (0x00 is ignored)
69-
if ((scl != NULL) && (sda != NULL)) {
70-
MXC_GPIO_Config(&i2c_maps[self->i2c_id]);
71-
err = MXC_I2C_Init(self->i2c_regs, 1, 0x00);
72-
if (err) {
73-
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("Failed to init I2C. ERR: %d\n"), err);
74-
}
75-
err = MXC_I2C_SetFrequency(self->i2c_regs, frequency);
76-
if (err < 0) {
77-
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("Failed to set I2C frequency. ERR: %d\n"), err);
78-
}
79-
} else if (scl != NULL) {
80-
mp_raise_NotImplementedError(MP_ERROR_TEXT("I2C needs SDA & SCL"));
81-
} else if (sda != NULL) {
82-
mp_raise_NotImplementedError(MP_ERROR_TEXT("I2C needs SDA & SCL"));
83-
} else {
84-
// Should not get here, as shared-bindings API should not call this way
85-
}
86-
8773
// Attach I2C pins
8874
self->sda = sda;
8975
self->scl = scl;
@@ -94,8 +80,8 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
9480
i2c_active |= (1 << self->i2c_id);
9581

9682
// Set the timeout to a default value
97-
if (((timeout < 0.0) || (timeout > 100.0))) {
98-
self->timeout = 1.0;
83+
if (timeout > 100) {
84+
self->timeout = 1;
9985
} else {
10086
self->timeout = timeout;
10187
}
@@ -212,7 +198,6 @@ uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr,
212198
}
213199

214200
// Read into buffer from the device selected by address
215-
// todo test
216201
uint8_t common_hal_busio_i2c_read(busio_i2c_obj_t *self,
217202
uint16_t addr,
218203
uint8_t *data, size_t len) {
@@ -229,14 +214,14 @@ uint8_t common_hal_busio_i2c_read(busio_i2c_obj_t *self,
229214
};
230215
ret = MXC_I2C_MasterTransaction(&rd_req);
231216
if (ret) {
232-
mp_raise_RuntimeError(MP_ERROR_TEXT("ERROR during I2C Transaction\n"));
217+
// Return I/O error
218+
return MP_EIO;
233219
}
234220

235221
return 0;
236222
}
237223

238224
// Write the bytes from out_data to the device selected by address,
239-
// todo test
240225
uint8_t common_hal_busio_i2c_write_read(busio_i2c_obj_t *self, uint16_t addr,
241226
uint8_t *out_data, size_t out_len,
242227
uint8_t *in_data, size_t in_len) {
@@ -253,7 +238,7 @@ uint8_t common_hal_busio_i2c_write_read(busio_i2c_obj_t *self, uint16_t addr,
253238
};
254239
ret = MXC_I2C_MasterTransaction(&wr_rd_req);
255240
if (ret) {
256-
mp_raise_RuntimeError(MP_ERROR_TEXT("ERROR during I2C Transaction\n"));
241+
return MP_EIO;
257242
}
258243

259244
return 0;

‎ports/analog/common-hal/busio/SPI.c‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
9191
1, 0x01, 1000000, spi_pins);
9292
MXC_GPIO_SetVSSEL(MXC_GPIO_GET_GPIO(sck->port), MXC_GPIO_VSSEL_VDDIOH, (sck->mask | miso->mask | mosi->mask | MXC_GPIO_PIN_0));
9393
if (err) {
94-
mp_raise_RuntimeError(MP_ERROR_TEXT("Failed to init SPI.\n"));
94+
// NOTE: Reuse existing messages from locales/circuitpython.pot to save space
95+
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("%q init failed"), MP_QSTR_SPI);
9596
}
9697
} else {
9798
mp_raise_NotImplementedError(MP_ERROR_TEXT("SPI needs MOSI, MISO, and SCK"));
@@ -170,7 +171,7 @@ bool common_hal_busio_spi_configure(busio_spi_obj_t *self,
170171
clk_mode = MXC_SPI_CLKMODE_3;
171172
break;
172173
default:
173-
mp_raise_ValueError(MP_ERROR_TEXT("CPOL / CPHA must both be either 0 or 1\n"));
174+
// should not be reachable; validated in shared-bindings/busio/SPI.c
174175
return false;
175176
}
176177

‎ports/analog/common-hal/busio/UART.c‎

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -82,37 +82,37 @@ static volatile int uart_err;
8282
static uint8_t uart_never_reset_mask = 0;
8383
static busio_uart_obj_t *context;
8484

85-
static int isValidBaudrate(uint32_t baudrate) {
85+
static bool isValidBaudrate(uint32_t baudrate) {
8686
switch (baudrate) {
8787
case UART_9600:
88-
return 1;
88+
return true;
8989
break;
9090
case UART_14400:
91-
return 1;
91+
return true;
9292
break;
9393
case UART_19200:
94-
return 1;
94+
return true;
9595
break;
9696
case UART_38400:
97-
return 1;
97+
return true;
9898
break;
9999
case UART_57600:
100-
return 1;
100+
return true;
101101
break;
102102
case UART_115200:
103-
return 1;
103+
return true;
104104
break;
105105
case UART_230400:
106-
return 1;
106+
return true;
107107
break;
108108
case UART_460800:
109-
return 1;
109+
return true;
110110
break;
111111
case UART_921600:
112-
return 1;
112+
return true;
113113
break;
114114
default:
115-
return 0;
115+
return false;
116116
break;
117117
}
118118
}
@@ -126,7 +126,8 @@ static mxc_uart_parity_t convertParity(busio_uart_parity_t busio_parity) {
126126
case BUSIO_UART_PARITY_ODD:
127127
return MXC_UART_PARITY_ODD_0;
128128
default:
129-
mp_raise_ValueError(MP_ERROR_TEXT("Parity must be ODD, EVEN, or NONE\n"));
129+
// not reachable due to validation in shared-bindings/busio/SPI.c
130+
return MXC_UART_PARITY_DISABLE;
130131
}
131132
}
132133

@@ -188,7 +189,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
188189
if ((rx != NULL) && (tx != NULL)) {
189190
err = MXC_UART_Init(self->uart_regs, baudrate, MXC_UART_IBRO_CLK);
190191
if (err != E_NO_ERROR) {
191-
mp_raise_RuntimeError(MP_ERROR_TEXT("Failed to initialize UART.\n"));
192+
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("%q init failed"), MP_QSTR_UART);
192193
}
193194

194195
// attach & configure pins
@@ -211,7 +212,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
211212
common_hal_mcu_pin_claim(self->cts_pin);
212213
common_hal_mcu_pin_claim(self->rts_pin);
213214
} else if (cts || rts) {
214-
mp_raise_ValueError(MP_ERROR_TEXT("Flow Ctrl needs both CTS & RTS"));
215+
mp_raise_ValueError(MP_ERROR_TEXT("Both RX and TX required for flow control"));
215216
}
216217

217218
// Set stop bits & data size
@@ -338,7 +339,7 @@ size_t common_hal_busio_uart_read(busio_uart_obj_t *self,
338339
*errcode = err;
339340
MXC_UART_AbortAsync(self->uart_regs);
340341
NVIC_DisableIRQ(MXC_UART_GET_IRQ(self->uart_id));
341-
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("\nERR: Error starting transaction: %d\n"), err);
342+
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("UART read error"));
342343
}
343344

344345
// Wait for transaction completion or timeout
@@ -350,11 +351,11 @@ size_t common_hal_busio_uart_read(busio_uart_obj_t *self,
350351
if (uart_status[self->uart_id] != UART_FREE) {
351352
MXC_UART_AbortAsync(self->uart_regs);
352353
NVIC_DisableIRQ(MXC_UART_GET_IRQ(self->uart_id));
353-
mp_raise_RuntimeError(MP_ERROR_TEXT("\nERR: Uart transaction timed out.\n"));
354+
mp_raise_RuntimeError(MP_ERROR_TEXT("UART transaction timeout"));
354355
}
355356
// Check for errors from the callback
356357
else if (uart_err != E_NO_ERROR) {
357-
mp_printf(&mp_sys_stdout_print, "MAX32 ERR: %d\n", uart_err);
358+
// todo: indicate error?
358359
MXC_UART_AbortAsync(self->uart_regs);
359360
}
360361

@@ -399,7 +400,7 @@ size_t common_hal_busio_uart_write(busio_uart_obj_t *self,
399400
*errcode = err;
400401
MXC_UART_AbortAsync(self->uart_regs);
401402
NVIC_DisableIRQ(MXC_UART_GET_IRQ(self->uart_id));
402-
mp_raise_ConnectionError(MP_ERROR_TEXT("\nERR: Requested bus is busy\n"));
403+
mp_raise_ValueError(MP_ERROR_TEXT("All UART peripherals are in use"));
403404
}
404405

405406
// Wait for transaction completion or timeout
@@ -409,7 +410,7 @@ size_t common_hal_busio_uart_write(busio_uart_obj_t *self,
409410
// Call the handler and abort if errors
410411
uart_err = MXC_UART_AsyncHandler(self->uart_regs);
411412
if (uart_err != E_NO_ERROR) {
412-
mp_printf(&mp_sys_stdout_print, "MAX32 ERR: %d\n", uart_err);
413+
// todo: indicate error?
413414
MXC_UART_AbortAsync(self->uart_regs);
414415
}
415416
}
@@ -418,11 +419,11 @@ size_t common_hal_busio_uart_write(busio_uart_obj_t *self,
418419
if (uart_status[self->uart_id] != UART_FREE) {
419420
MXC_UART_AbortAsync(self->uart_regs);
420421
NVIC_DisableIRQ(MXC_UART_GET_IRQ(self->uart_id));
421-
mp_raise_ConnectionError(MP_ERROR_TEXT("\nERR: Uart transaction timed out.\n"));
422+
mp_raise_RuntimeError(MP_ERROR_TEXT("Uart transaction timed out."));
422423
}
423424
// Check for errors from the callback
424425
else if (uart_err != E_NO_ERROR) {
425-
mp_printf(&mp_sys_stdout_print, "MAX32 ERR: %d\n", uart_err);
426+
// todo: indicate error?
426427
MXC_UART_AbortAsync(self->uart_regs);
427428
}
428429

@@ -438,7 +439,7 @@ void common_hal_busio_uart_set_baudrate(busio_uart_obj_t *self, uint32_t baudrat
438439
if (isValidBaudrate(baudrate)) {
439440
self->baudrate = baudrate;
440441
} else {
441-
mp_raise_ValueError(MP_ERROR_TEXT("Baudrate invalid. Must be a standard UART baudrate.\n"));
442+
mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid %q"), MP_QSTR_baudrate);
442443
}
443444
}
444445

‎ports/analog/peripherals/max32690/max32_i2c.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@ int pinsToI2c(const mcu_pin_obj_t *sda, const mcu_pin_obj_t *scl) {
7070
}
7171
#endif
7272

73-
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("ERR: Unable to find an I2C matching pins...\nSCL: port %d mask %d\nSDA: port %d mask %d\n"),
74-
sda->port, sda->mask, scl->port, scl->mask);
73+
mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid %q"), MP_QSTR_pins);
7574
return -1;
7675
}

‎ports/analog/peripherals/max32690/max32_spi.c‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,6 @@ int pinsToSpi(const mcu_pin_obj_t *mosi, const mcu_pin_obj_t *miso,
3939
return i;
4040
}
4141
}
42-
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("ERR: Unable to find an SPI matching pins... \
43-
\nMOSI: port %d mask %d\nMISO: port %d mask %d\n"),
44-
mosi->port, mosi->mask, miso->port, miso->mask,
45-
sck->port, sck->mask);
42+
mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid %q"), MP_QSTR_pins);
4643
return -1;
4744
}

‎ports/analog/peripherals/max32690/max32_uart.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ int pinsToUart(const mcu_pin_obj_t *rx, const mcu_pin_obj_t *tx) {
3131
return i;
3232
}
3333
}
34-
mp_raise_RuntimeError_varg(MP_ERROR_TEXT("ERR: Unable to find a uart matching pins...\nTX: port %d mask %d\nRX: port %d mask %d\n"),
35-
tx->port, tx->mask, rx->port, rx->mask);
34+
mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid %q"), MP_QSTR_pins);
3635
return -1;
3736
}

0 commit comments

Comments
 (0)