HAL read/peek timeout with native freeRTOS blocking mechanism #3004
Conversation
| int timedRead(); // private method to read stream with timeout | ||
| int timedPeek(); // private method to peek stream with timeout | ||
| virtual int timedRead(); // private method to read stream with timeout | ||
| virtual int timedPeek(); // private method to peek stream with timeout |
There was a problem hiding this comment.
this may break compatibility with libraries.
There was a problem hiding this comment.
@atanisoft can you be more precise and tell how will it break things? any usage when timedRead should be called directly on Stream.h without virtual inheritance?
There was a problem hiding this comment.
Stream.h is defined by the Arduino core specification, you can see the declaration of these methods here. By adding virtual to these methods it will potentially break compatibility with any library which overrides these methods without the declaration of virtual. A similar problem has been seen in #2755
There was a problem hiding this comment.
I cannot imagine any viable scenario about how virtual can break the things. I've got your points about UART HAL methods, and will workaround them. But it is useless if you discard this PR because of virtual methods. Let's decide if this have chance to be merged?
@me-no-dev, what's your opinion about this ?
There was a problem hiding this comment.
I'm not against the addition of virtual to the declaration but it may have unintended breakages in libraries which extend this class
| @@ -62,6 +62,8 @@ class HardwareSerial: public Stream | |||
| int availableForWrite(void); | |||
| int peek(void); | |||
| int read(void); | |||
| virtual int timedRead(); // private method to read uart with timeout | |||
| virtual int timedPeek(); // private method to peek uart with timeout | |||
|
|
||
| if (xHigherPriorityTaskWoken) { | ||
| portYIELD_FROM_ISR(); | ||
| } |
There was a problem hiding this comment.
Fix the indent for this block
| if(xQueueReceive(uart->queue, &c, 0)) { | ||
| return c; | ||
| if(xQueueReceive(uart->queue, data, to)) { | ||
| return 1; |
There was a problem hiding this comment.
this alters the expected behavior of uartRead() which returns the character that was read from the uart device.
There was a problem hiding this comment.
@atanisoft mention 0 timeout in original code. It will return 0 even if there's nothing to read in uart. That's weird behavior and breaks POLA per se and should be fixed.
There was a problem hiding this comment.
while it may be weird and break POLA, it is likely going to break a number of existing libraries which rely on the current method signatures and behaviors of the methods. I know I have a library which uses the UART HAL (I'll be moving it over to IDF API instead soon).
There was a problem hiding this comment.
how do you determine case if UART empty or not in your library?
Moving to idf API was the first solution we've considered. But it's not perfect too. If you carefully look into uart api implementation you'll notice that it's impossible to set RX buffer size, according to example given in documentation. So, we've discarded that solution in favor to fix HAL
There was a problem hiding this comment.
uartAvailable() is used to check for presence of data.
As for RX buffer size, that is allocated at time of initialization of the uart via IDF API
There was a problem hiding this comment.
So, what do you recommend? add overloaded functions?
There was a problem hiding this comment.
You could add a new method with the updated signature with the existing one calling into the new method for compatibility. Since this is C code and not C++ I'm not sure if an overloaded method with the same name is possible.
There was a problem hiding this comment.
Oh, yes. It's C. I'll think how to implement this.
| if(xQueuePeek(uart->queue, &c, 0)) { | ||
| return c; | ||
| if(xQueuePeek(uart->queue, data, to)) { | ||
| return 1; |
There was a problem hiding this comment.
same for this method, the uartPeek() method returns the next character available to read
There was a problem hiding this comment.
@atanisoft same here
|
|
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Using any timed function to read uart causes warnings in IDLE task. This happens because of busy loop wait for reading uart. We can gracefully utilize FreeRTOS timed read from UART RX queue for same purpose and let CPU use less power/cycles while reading UART