-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Adds HardwareSerial::setRxTimeout() #6397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
af0dabe
ffe0882
fa717e7
22c5c71
fbc0734
951ad87
8db2400
1aff6b1
42fe78f
83ff56d
be50818
281ae37
807d750
d5de62c
c1b08e8
20ecdae
5574fc4
e67d1d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -128,7 +128,8 @@ HardwareSerial::HardwareSerial(int uart_nr) : | |||||
_uart_nr(uart_nr), | ||||||
_uart(NULL), | ||||||
_rxBufferSize(256), | ||||||
_onReceiveCB(NULL), | ||||||
_onReceiveCB(NULL), | ||||||
_onReceiveTimeout(10), | ||||||
_onReceiveErrorCB(NULL), | ||||||
_eventTask(NULL) | ||||||
#if !CONFIG_DISABLE_HAL_LOCKS | ||||||
|
@@ -191,10 +192,27 @@ void HardwareSerial::onReceive(OnReceiveCb function) | |||||
HSERIAL_MUTEX_LOCK(); | ||||||
// function may be NULL to cancel onReceive() from its respective task | ||||||
_onReceiveCB = function; | ||||||
|
||||||
// this can be called after Serial.begin(), therefore it shall create the event task | ||||||
if (function != NULL && _uart != NULL && _eventTask == NULL) { | ||||||
_createEventTask(this); | ||||||
_createEventTask(this); // Create event task | ||||||
} | ||||||
HSERIAL_MUTEX_UNLOCK(); | ||||||
} | ||||||
|
||||||
void HardwareSerial::onReceiveTimeout(uint8_t symbols_timeout) | ||||||
|
||||||
{ | ||||||
HSERIAL_MUTEX_LOCK(); | ||||||
|
||||||
if(symbols_timeout == 0) { | ||||||
_onReceiveTimeout = 1; // Never disable timeout | ||||||
|
||||||
} | ||||||
else { | ||||||
_onReceiveTimeout = symbols_timeout; | ||||||
} | ||||||
|
||||||
if(_uart != NULL) uart_set_rx_timeout(_uart_nr, _onReceiveTimeout); // Set new timeout | ||||||
|
||||||
HSERIAL_MUTEX_UNLOCK(); | ||||||
} | ||||||
|
||||||
|
@@ -210,15 +228,17 @@ void HardwareSerial::_uartEventTask(void *args) | |||||
if(xQueueReceive(uartEventQueue, (void * )&event, (portTickType)portMAX_DELAY)) { | ||||||
switch(event.type) { | ||||||
case UART_DATA: | ||||||
if(uart->_onReceiveCB && uart->available() > 0) uart->_onReceiveCB(); | ||||||
if(uart->_onReceiveCB && uart->available() > 0 && event.timeout_flag) uart->_onReceiveCB(); | ||||||
|
||||||
break; | ||||||
case UART_FIFO_OVF: | ||||||
log_w("UART%d FIFO Overflow. Consider adding Hardware Flow Control to your Application.", uart->_uart_nr); | ||||||
if(uart->_onReceiveErrorCB) uart->_onReceiveErrorCB(UART_FIFO_OVF_ERROR); | ||||||
xQueueReset(uartEventQueue); | ||||||
|
xQueueReset(uart->uart_event_queue); |
arduino-esp32/cores/esp32/esp32-hal-uart.c
Line 123 in c014eaf
xQueueReset(uart->uart_event_queue); |
If you think this should be deleted, I have no opposition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather discuss it, than just delete it :) @SuGlider want to pitch in an idea too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess this is a bit relative. It could happen that you have a very fast overflow and you will be called multiple times with the UART_FIFO_OVF event. But thinking about this more thoroughly, maybe that is not a "normal" or "frequent" situation. I added it back because I saw it on the original esp32-hal-uart code and when I was testing the overflow with the serial monitor, I noticed that behaviour. But as I said, probably is not a frequent situation and you could eventually lose something important on the event queue? (maybe an UART_DATA event after an overflow event?). Probably this should be deleted. But I will let @SuGlider give this opinion about this. I would love to have this on 2.0.3 because I will be using that version as the new core for my project (2.0.2 has many bugs with UART).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gonzabrusco - about your consideration:
One consideration. If an overflow occurs, and then you don't process the data on the buffer with onReceiveError callback, then onReceive will never get called again.
It is a real problem for most Arduino users.
onReceive Callback should be called also when RX FIFO is full and UART driver copies that data to the RX ringbuffer.
I think it should pass a parameter to the call back function informing if there was a Timeout instead of forcing only calling it when timeout occurs. This way the user can decide what to do in the callback and it is more flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding xQueueReset(uart->uart_event_queue);
, it shall be removed from the task.
I suggest that it may become a new function such as eventQueueReset()
in HardwareSerial API.
This way users can decide when to reset the queue instead of forcing it in the task loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last suggestion is about changing the name of the function void HardwareSerial::onReceiveTimeout(uint8_t symbols_timeout)
to void HardwareSerial::setOnReceiveTimeout(uint8_t symbols_timeout)
or some name like setRxTimeout()
.
Timeout mode shall not be forced in the code... it shall be an option. check
// from void HardwareSerial::onReceiveTimeout(uint8_t symbols_timeout) :::
if(symbols_timeout == 0) {
_onReceiveTimeout = 1; // Never disable timeout
}
// from void HardwareSerial::begin(...) :::
// Set UART RX timeout
if (_uart != NULL && _onReceiveCB != NULL) {
uart_set_rx_timeout(_uart_nr, _onReceiveTimeout);
}
// from void HardwareSerial::_uartEventTask(void *args)) ::: "&& event.timeout_flag"
case UART_DATA:
if(uart->_onReceiveCB && uart->available() > 0 && event.timeout_flag) uart->_onReceiveCB();
break;
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xQueueReset(uartEventQueue);
could go on a API function just for that, such as void HardwareSerial::eventQueueReset()
.
This way the sketch can decide when to flush the events.
Please remove it.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shall not be here. It shall be only in the setRxTimeout(...)
API function.
It is enforcing timeout usage to the sketch. That shall be optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before. The timeout behaviour is the default as per IDF (10 symbols defined in UART_TOUT_THRESH_DEFAULT). I think this is needed because the user could call setRxTimeout before or after the begin() . This is the same principle as you took with onReceive(). This is because uart_set_rx_timeout should be called once the uart is initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I agree. Let's set the default always.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,9 +73,21 @@ class HardwareSerial: public Stream | |
HardwareSerial(int uart_nr); | ||
~HardwareSerial(); | ||
|
||
// onReceive will setup a callback for whenever UART data is received | ||
// onReceive will setup a callback for whenever UART data is received and a later timeout occurs (no receiving data after specified time): | ||
// param function is the callback to be called after reception timeout | ||
|
||
// it will work as UART Rx interrupt -- Using C++ 11 std::fuction | ||
void onReceive(OnReceiveCb function); | ||
|
||
SuGlider marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
// onReceiveTimeout sets the timeout after which onReceive callback will be called (after receiving data, it waits for this time of UART rx inactivity to call the callback fnc) | ||
// param symbols_timeout defines a timeout threshold in uart symbol periods. | ||
// Minimum is 1 symbol timeout. Maximum is calculacted automatically by IDF. If set above the maximum, it is ignored and an error is printed on Serial0 (check console). | ||
// Examples: Maximum for 11 bits symbol is 92 (SERIAL_8N2, SERIAL_8E1, SERIAL_8O1, etc), Maximum for 10 bits symbol is 101 (SERIAL_8N1). | ||
// For example symbols_timeout=1 defines a timeout equal to transmission time of one symbol (~11 bit) on current baudrate. | ||
// For a baudrate of 9600, SERIAL_8N1 (10 bit symbol) and symbols_timeout = 3, the timeout would be 3 / (9600 / 10) = 3.125 ms | ||
void onReceiveTimeout(uint8_t symbols_timeout); | ||
|
||
// onReceive will be called on error events (see hardwareSerial_error_t) | ||
void onReceiveError(OnReceiveErrorCb function); | ||
|
||
void begin(unsigned long baud, uint32_t config=SERIAL_8N1, int8_t rxPin=-1, int8_t txPin=-1, bool invert=false, unsigned long timeout_ms = 20000UL, uint8_t rxfifo_full_thrhd = 112); | ||
|
@@ -138,6 +150,7 @@ class HardwareSerial: public Stream | |
uart_t* _uart; | ||
size_t _rxBufferSize; | ||
OnReceiveCb _onReceiveCB; | ||
uint8_t _onReceiveTimeout; | ||
|
||
OnReceiveErrorCb _onReceiveErrorCB; | ||
TaskHandle_t _eventTask; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it shall have as default value ZERO. Disabled. This way it may be enabled using the API.
This will force other changes in the code, mainly because of issues with RX Overflow.
When RX overflow happens, IDF driver disables RX Timeout interrupt flag!
Thus, onReceive will never be called again and the queue will continue full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only thing I disagree. By default now (as before my pull request), onReceive callback is being called by timeout (10 symbos by default) or by rx fifo full (120 bytes). That is the standard behaviour by IDF. I don't think that Arduino core should use another default. I would prefer to let the timeout on because if you receive 119 bytes, you will never get called (unless you receive on more byte that will trigger UART_INTR_RXFIFO_FULL interrupt)