stmhal: UART(..., timeout=0) is broken for stream protocol #1533
Comments
@dpgeorge : Is it ok to apply such fix, or do you have something else in mind (I'd suggest just applying workarounds for issues like this, instead saving effort to do it better in new machine module). |
|
I think read can be fixed by setting timeout_char to 1 (or actually proportional to inverse baudrate), since that's what it waits upon within the read loop. It really depends what semantics you want. If timeout=0 and timeout_char=0 then that really means that read() will only ever read the currently buffered chars, and no more. So it's up to the user to specify non-zero timeout_char to get more sensible non-blocking behaviour. So my feeling is that read is actually not broken. Maybe write should also use timeout_char instead of timeout? |
I don't think that would classify as "fix". Otherwise, I propose to workaround the issue by adjusting timeout setting behind the scenes, whereas you - timeout_char. I tested my workaround, if you tested yours and think it's better, can go for that too.
Yes, I started this report with talking about it: "UART(..., timeout=0) is supposed to put UART in fully non-blocking mode, where operations on byte strings either complete almost immediately (possibly partially), or fail with EAGAIN."
Yes, that's how it should work, and how it does not work, see testcases above - do you get different results when trying them?
Non-blocking behavior means not waiting on any timeouts at all, with obvious way to signify that by timeout=0. |
|
It would be nice to be able to see how many bytes are in the UART RX buffer so they can be immediately read out without causing any delays, rather than relying on readall() and waiting for the timeout. I acknowledge this is just another work around for the above issue but could also be used in other cases. This should be rather simple to implement (or does it exist and I'm just not seeing it?) Perhaps .any() can return the number of bytes in buffer instead of true/false? EDIT:
Perhaps if timeout=0 put the UART in to aync mode then the TX would be async too rather than returning an error? |
+1 |
|
+1 from me too. |
|
Actually, that's how it is defined by the new hardware API: https://github.com/micropython/micropython/wiki/Hardware-API#the-uart-class
|
|
I looked at this issue. I agree that uart.write() is totally broken when timeout=0. So in my tests I changed self->timeout to self->timeout_char in pyb_uart_write in the call to HAL_UART_Transmit (ie it uses timeout_char instead of timeout for the time between transmitted characters). Then I took a pyboard with X1 connected to Y2 (tx of one uart to rx of another) and tried: So that seems to work. I do not find any issue with uart.read() as it is, even when timeout=0 and timeout_char=0. |
|
@dpgeorge : Can you please also test: And make that work well (because relying on some obscure params to make it work is well ..., inconvenient.) |
uy will work reading bytes, but ux won't send them with default timeout_char=0. I think the proper way to fix/workaround/close this issue is to make timeout_char have a default value which is 1/baudrate. That's what people expect (arguably we don't even need to expose the timeout_char parameter, it can be a completely internal detail). |
|
So, I can just repeat what I wrote above (without reading, so using other words and hopefully clarifying it): failing any other obvious means to set UART to non-blocking mode, that should be done with "timeout=0" argument. That argument should overrule and override any other arguments (like timeout_char; you can treat "incompatible" values for them as error, or just ignore those values). The main point is that "timeout=0" should be the only argument to set UART to non-blocking mode, without and other magic arguments. That was interface part. Implementation-wise, "timeout=0" ideally should lead to behavior when no busy-loops are ever used. But if that's not possible due to broken underlying vendor lib, any workaround is suitable, as long as interface works in the way described above. |
|
And some UARTS (the ones on STM32F7) support the generation of an IDLE interrupt, which corresponds to the timeout_char=1/baud (if my very brief testing is correct). Personally, I wouldn't take away timeout_char. Lots of people write code which writes a header, writes a body, writes a footer, and these wind up with tiny gaps between the header/body/footer, so being able to have a larger timeout_char makes sense. Specifying timeout_char in units of character times might make sense. |
In non-blocking mode (timeout=0), uart.write() can now transmit all of its data without raising an exception. uart.read() also works correctly in this mode. As part of this patch, timout_char now has a minimum value which is long enough to transfer 1 character. Addresses issue #1533.
|
Should now work as expected. |
|
@dpgeorge : Thanks! Closing then, if later testing will show issues, will reopen. |
|
For new HW API we need a good definition of what non-blocking mode means. Implementation is then easy :) |
UART(..., timeout=0) is supposed to put UART in fully non-blocking mode, where operations on byte strings either complete almost immediately (possibly partially), or fail with EAGAIN. This semantics most realistically requires buffering.
Currently, stmhal with timeout=0 doesn't behave reasonably: .write() always fails with ETIMEDOUT, even though it actually transmits 1st byte. .read() returns only first available byte, and appear to lost next bytes. Issue with .write() is due to fact that its implemented without buffering, and timeout is just passed to HAL_UART_Transmit() which has rather different semantics for timeout:
micropython/stmhal/hal/f2/src/stm32f2xx_hal_uart.c
Line 622 in 9e0a3d4
Don't know how to explain issues with .read(), but:
For comparison:
So, as proper fix will require a lot of rework, I propose to work that around by checking for timeout=0 in constructor and replacing it with timeout=1.
The text was updated successfully, but these errors were encountered: