Skip to content

Commit 3bca93b

Browse files
mvds00dpgeorge
mvds00
authored and
dpgeorge
committed
ports: Fix sys.stdout.buffer.write() return value.
MicroPython code may rely on the return value of sys.stdout.buffer.write() to reflect the number of bytes actually written. While in most scenarios a write() operation is successful, there are cases where it fails, leading to data loss. This problem arises because, currently, write() merely returns the number of bytes it was supposed to write, without indication of failure. One scenario where write() might fail, is where USB is used and the receiving end doesn't read quickly enough to empty the receive buffer. In that case, write() on the MicroPython side can timeout, resulting in the loss of data without any indication, a behavior observed notably in communication between a Pi Pico as a client and a Linux host using the ACM driver. A complex issue arises with mp_hal_stdout_tx_strn() when it involves multiple outputs, such as USB, dupterm and hardware UART. The challenge is in handling cases where writing to one output is successful, but another fails, either fully or partially. This patch implements the following solution: mp_hal_stdout_tx_strn() attempts to write len bytes to all of the possible destinations for that data, and returns the minimum successful write length. The implementation of this is complicated by several factors: - multiple outputs may be enabled or disabled at compiled time - multiple outputs may be enabled or disabled at runtime - mp_os_dupterm_tx_strn() is one such output, optionally containing multiple additional outputs - each of these outputs may or may not be able to report success - each of these outputs may or may not be able to report partial writes As a result, there's no single strategy that fits all ports, necessitating unique logic for each instance of mp_hal_stdout_tx_strn(). Note that addressing sys.stdout.write() is more complex due to its data modification process ("cooked" output), and it remains unchanged in this patch. Developers who are concerned about accurate return values from write operations should use sys.stdout.buffer.write(). This patch might disrupt some existing code, but it's also expected to resolve issues, considering that the peculiar return value behavior of sys.stdout.buffer.write() is not well-documented and likely not widely known. Therefore, it's improbable that much existing code relies on the previous behavior. Signed-off-by: Maarten van der Schrieck <[email protected]>
1 parent 91ee8ac commit 3bca93b

File tree

22 files changed

+136
-40
lines changed

22 files changed

+136
-40
lines changed

‎ports/cc3200/hal/cc3200_hal.c

+7-2
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,15 @@ void mp_hal_delay_ms(mp_uint_t delay) {
140140
}
141141
}
142142

143-
void mp_hal_stdout_tx_strn(const char *str, size_t len) {
144-
mp_os_dupterm_tx_strn(str, len);
143+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, size_t len) {
144+
mp_uint_t ret = len;
145+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
146+
if (dupterm_res >= 0) {
147+
ret = dupterm_res;
148+
}
145149
// and also to telnet
146150
telnet_tx_strn(str, len);
151+
return ret;
147152
}
148153

149154
int mp_hal_stdin_rx_chr(void) {

‎ports/esp32/mphalport.c

+12-2
Original file line numberDiff line numberDiff line change
@@ -125,24 +125,34 @@ int mp_hal_stdin_rx_chr(void) {
125125
}
126126
}
127127

128-
void mp_hal_stdout_tx_strn(const char *str, size_t len) {
128+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, size_t len) {
129129
// Only release the GIL if many characters are being sent
130+
mp_uint_t ret = len;
131+
bool did_write = false;
130132
bool release_gil = len > 20;
131133
if (release_gil) {
132134
MP_THREAD_GIL_EXIT();
133135
}
134136
#if CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG
135137
usb_serial_jtag_tx_strn(str, len);
138+
did_write = true;
136139
#elif CONFIG_USB_OTG_SUPPORTED
137140
usb_tx_strn(str, len);
141+
did_write = true;
138142
#endif
139143
#if MICROPY_HW_ENABLE_UART_REPL
140144
uart_stdout_tx_strn(str, len);
145+
did_write = true;
141146
#endif
142147
if (release_gil) {
143148
MP_THREAD_GIL_ENTER();
144149
}
145-
mp_os_dupterm_tx_strn(str, len);
150+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
151+
if (dupterm_res >= 0) {
152+
did_write = true;
153+
ret = MIN((mp_uint_t)dupterm_res, ret);
154+
}
155+
return did_write ? ret : 0;
146156
}
147157

148158
uint32_t mp_hal_ticks_ms(void) {

‎ports/esp8266/esp_mphal.c

+8-2
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,14 @@ void mp_hal_debug_str(const char *str) {
9494
}
9595
#endif
9696

97-
void mp_hal_stdout_tx_strn(const char *str, uint32_t len) {
98-
mp_os_dupterm_tx_strn(str, len);
97+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, uint32_t len) {
98+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
99+
if (dupterm_res < 0) {
100+
// no outputs, nothing was written
101+
return 0;
102+
} else {
103+
return dupterm_res;
104+
}
99105
}
100106

101107
void mp_hal_debug_tx_strn_cooked(void *env, const char *str, uint32_t len) {

‎ports/mimxrt/mphalport.c

+14-3
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,12 @@ int mp_hal_stdin_rx_chr(void) {
112112
}
113113
}
114114

115-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
115+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
116+
mp_uint_t ret = len;
117+
bool did_write = false;
116118
if (tud_cdc_connected()) {
117-
for (size_t i = 0; i < len;) {
119+
size_t i = 0;
120+
while (i < len) {
118121
uint32_t n = len - i;
119122
if (n > CFG_TUD_CDC_EP_BUFSIZE) {
120123
n = CFG_TUD_CDC_EP_BUFSIZE;
@@ -125,17 +128,25 @@ void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
125128
MICROPY_EVENT_POLL_HOOK
126129
}
127130
if (ticks_us64() >= timeout) {
131+
ret = i;
128132
break;
129133
}
130134

131135
uint32_t n2 = tud_cdc_write(str + i, n);
132136
tud_cdc_write_flush();
133137
i += n2;
134138
}
139+
did_write = true;
140+
ret = MIN(i, ret);
135141
}
136142
#if MICROPY_PY_OS_DUPTERM
137-
mp_os_dupterm_tx_strn(str, len);
143+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
144+
if (dupterm_res >= 0) {
145+
did_write = true;
146+
ret = MIN((mp_uint_t)dupterm_res, ret);
147+
}
138148
#endif
149+
return did_write ? ret : 0;
139150
}
140151

141152
uint64_t mp_hal_time_ns(void) {

‎ports/minimal/uart_core.c

+7-2
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,14 @@ int mp_hal_stdin_rx_chr(void) {
2929
}
3030

3131
// Send string of given length
32-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
32+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
33+
mp_uint_t ret = len;
3334
#if MICROPY_MIN_USE_STDOUT
3435
int r = write(STDOUT_FILENO, str, len);
35-
(void)r;
36+
if (r >= 0) {
37+
// in case of an error in the syscall, report no bytes written
38+
ret = 0;
39+
}
3640
#elif MICROPY_MIN_USE_STM32_MCU
3741
while (len--) {
3842
// wait for TXE
@@ -41,4 +45,5 @@ void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
4145
USART1->DR = *str++;
4246
}
4347
#endif
48+
return ret;
4449
}

‎ports/nrf/drivers/bluetooth/ble_uart.c

+4-2
Original file line numberDiff line numberDiff line change
@@ -110,10 +110,11 @@ int mp_hal_stdin_rx_chr(void) {
110110
return (int)byte;
111111
}
112112

113-
void mp_hal_stdout_tx_strn(const char *str, size_t len) {
113+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, size_t len) {
114114
// Not connected: drop output
115-
if (!ble_uart_enabled()) return;
115+
if (!ble_uart_enabled()) return 0;
116116

117+
mp_uint_t ret = len;
117118
uint8_t *buf = (uint8_t *)str;
118119
size_t send_len;
119120

@@ -134,6 +135,7 @@ void mp_hal_stdout_tx_strn(const char *str, size_t len) {
134135
len -= send_len;
135136
buf += send_len;
136137
}
138+
return ret;
137139
}
138140

139141
void ble_uart_tx_char(char c) {

‎ports/nrf/drivers/usb/usb_cdc.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -231,12 +231,12 @@ int mp_hal_stdin_rx_chr(void) {
231231
return 0;
232232
}
233233

234-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
235-
234+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
236235
for (const char *top = str + len; str < top; str++) {
237236
ringbuf_put((ringbuf_t*)&tx_ringbuf, *str);
238237
usb_cdc_loop();
239238
}
239+
return len;
240240
}
241241

242242
void mp_hal_stdout_tx_strn_cooked(const char *str, mp_uint_t len) {

‎ports/nrf/mphalport.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -196,10 +196,12 @@ int mp_hal_stdin_rx_chr(void) {
196196
return 0;
197197
}
198198

199-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
199+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
200200
if (MP_STATE_VM(dupterm_objs[0]) != MP_OBJ_NULL) {
201201
uart_tx_strn(MP_STATE_VM(dupterm_objs[0]), str, len);
202+
return len;
202203
}
204+
return 0;
203205
}
204206

205207
void mp_hal_stdout_tx_strn_cooked(const char *str, mp_uint_t len) {

‎ports/pic16bit/pic16bit_mphal.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,12 @@ void mp_hal_stdout_tx_str(const char *str) {
7575
mp_hal_stdout_tx_strn(str, strlen(str));
7676
}
7777

78-
void mp_hal_stdout_tx_strn(const char *str, size_t len) {
78+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, size_t len) {
79+
mp_uint_t ret = len;
7980
for (; len > 0; --len) {
8081
uart_tx_char(*str++);
8182
}
83+
return ret;
8284
}
8385

8486
void mp_hal_stdout_tx_strn_cooked(const char *str, size_t len) {

‎ports/powerpc/uart_lpc_serial.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,13 @@ int mp_hal_stdin_rx_chr(void) {
108108
}
109109

110110

111-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
111+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
112112
int i;
113113
for (i = 0; i < len; i++) {
114114
while (lpc_uart_tx_full()) {
115115
;
116116
}
117117
lpc_uart_reg_write(REG_RBR, str[i]);
118118
}
119+
return len;
119120
}

‎ports/powerpc/uart_potato.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ int mp_hal_stdin_rx_chr(void) {
118118
return (char)(val & 0x000000ff);
119119
}
120120

121-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
121+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
122122
int i;
123123

124124
for (i = 0; i < len; i++) {
@@ -129,4 +129,5 @@ void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
129129
}
130130
potato_uart_reg_write(POTATO_CONSOLE_TX, val);
131131
}
132+
return len;
132133
}

‎ports/renesas-ra/mphalport.c

+15-3
Original file line numberDiff line numberDiff line change
@@ -154,16 +154,20 @@ int mp_hal_stdin_rx_chr(void) {
154154
}
155155

156156
// Send string of given length
157-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
157+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
158+
mp_uint_t ret = len;
159+
bool did_write = false;
158160
#if MICROPY_HW_ENABLE_UART_REPL
159161
if (MP_STATE_PORT(pyb_stdio_uart) != NULL) {
160162
uart_tx_strn(MP_STATE_PORT(pyb_stdio_uart), str, len);
163+
did_write = true;
161164
}
162165
#endif
163166

164167
#if MICROPY_HW_USB_CDC
165168
if (tud_cdc_connected()) {
166-
for (size_t i = 0; i < len;) {
169+
size_t i = 0;
170+
while (i < len) {
167171
uint32_t n = len - i;
168172
if (n > CFG_TUD_CDC_EP_BUFSIZE) {
169173
n = CFG_TUD_CDC_EP_BUFSIZE;
@@ -180,12 +184,20 @@ void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
180184
tud_cdc_write_flush();
181185
i += n2;
182186
}
187+
ret = MIN(i, ret);
188+
did_write = true;
183189
}
184190
#endif
185191

186192
#if MICROPY_PY_OS_DUPTERM
187-
mp_os_dupterm_tx_strn(str, len);
193+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
194+
if (dupterm_res >= 0) {
195+
did_write = true;
196+
ret = MIN((mp_uint_t)dupterm_res, ret);
197+
}
188198
#endif
199+
200+
return did_write ? ret : 0;
189201
}
190202

191203
void mp_hal_ticks_cpu_enable(void) {

‎ports/rp2/mphalport.c

+15-3
Original file line numberDiff line numberDiff line change
@@ -151,14 +151,18 @@ int mp_hal_stdin_rx_chr(void) {
151151
}
152152

153153
// Send string of given length
154-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
154+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
155+
mp_uint_t ret = len;
156+
bool did_write = false;
155157
#if MICROPY_HW_ENABLE_UART_REPL
156158
mp_uart_write_strn(str, len);
159+
did_write = true;
157160
#endif
158161

159162
#if MICROPY_HW_USB_CDC
160163
if (tud_cdc_connected()) {
161-
for (size_t i = 0; i < len;) {
164+
size_t i = 0;
165+
while (i < len) {
162166
uint32_t n = len - i;
163167
if (n > CFG_TUD_CDC_EP_BUFSIZE) {
164168
n = CFG_TUD_CDC_EP_BUFSIZE;
@@ -173,18 +177,26 @@ void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
173177
mp_usbd_task();
174178
}
175179
if (timeout >= MICROPY_HW_USB_CDC_TX_TIMEOUT) {
180+
ret = i;
176181
break;
177182
}
178183
uint32_t n2 = tud_cdc_write(str + i, n);
179184
tud_cdc_write_flush();
180185
i += n2;
181186
}
187+
ret = MIN(i, ret);
188+
did_write = true;
182189
}
183190
#endif
184191

185192
#if MICROPY_PY_OS_DUPTERM
186-
mp_os_dupterm_tx_strn(str, len);
193+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
194+
if (dupterm_res >= 0) {
195+
did_write = true;
196+
ret = MIN((mp_uint_t)dupterm_res, ret);
197+
}
187198
#endif
199+
return did_write ? ret : 0;
188200
}
189201

190202
void mp_hal_delay_ms(mp_uint_t ms) {

‎ports/samd/mphalport.c

+14-3
Original file line numberDiff line numberDiff line change
@@ -200,9 +200,12 @@ int mp_hal_stdin_rx_chr(void) {
200200
}
201201
}
202202

203-
void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
203+
mp_uint_t mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
204+
mp_uint_t ret = len;
205+
bool did_write = false;
204206
if (tud_cdc_connected()) {
205-
for (size_t i = 0; i < len;) {
207+
size_t i = 0;
208+
while (i < len) {
206209
uint32_t n = len - i;
207210
if (n > CFG_TUD_CDC_EP_BUFSIZE) {
208211
n = CFG_TUD_CDC_EP_BUFSIZE;
@@ -213,14 +216,22 @@ void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) {
213216
MICROPY_EVENT_POLL_HOOK_WITH_USB;
214217
}
215218
if (timeout >= MICROPY_HW_USB_CDC_TX_TIMEOUT) {
219+
ret = i;
216220
break;
217221
}
218222
uint32_t n2 = tud_cdc_write(str + i, n);
219223
tud_cdc_write_flush();
220224
i += n2;
221225
}
226+
ret = MIN(i, ret);
227+
did_write = true;
222228
}
223229
#if MICROPY_PY_OS_DUPTERM
224-
mp_os_dupterm_tx_strn(str, len);
230+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
231+
if (dupterm_res >= 0) {
232+
did_write = true;
233+
ret = MIN((mp_uint_t)dupterm_res, ret);
234+
}
225235
#endif
236+
return did_write ? ret : 0;
226237
}

‎ports/stm32/mphalport.c

+10-2
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,22 @@ MP_WEAK int mp_hal_stdin_rx_chr(void) {
5757
}
5858
}
5959

60-
MP_WEAK void mp_hal_stdout_tx_strn(const char *str, size_t len) {
60+
MP_WEAK mp_uint_t mp_hal_stdout_tx_strn(const char *str, size_t len) {
61+
mp_uint_t ret = len;
62+
bool did_write = false;
6163
if (MP_STATE_PORT(pyb_stdio_uart) != NULL) {
6264
uart_tx_strn(MP_STATE_PORT(pyb_stdio_uart), str, len);
65+
did_write = true;
6366
}
6467
#if 0 && defined(USE_HOST_MODE) && MICROPY_HW_HAS_LCD
6568
lcd_print_strn(str, len);
6669
#endif
67-
mp_os_dupterm_tx_strn(str, len);
70+
int dupterm_res = mp_os_dupterm_tx_strn(str, len);
71+
if (dupterm_res >= 0) {
72+
did_write = true;
73+
ret = MIN((mp_uint_t)dupterm_res, ret);
74+
}
75+
return did_write ? ret : 0;
6876
}
6977

7078
#if __CORTEX_M >= 0x03

0 commit comments

Comments
 (0)