[UART] Add frame and parity error handling #341
Conversation
Introduced Frame Error check for the received byte
There was a problem hiding this comment.
@Rocketct I've left some comments.
Please also try to respect the code style of the source you are editing, for example:
- spacing after assignment
- if statements
What kinds of testing have you done with the changes? Are there any performance impacts?
cores/arduino/Uart.cpp
Outdated
| @@ -91,8 +91,11 @@ void Uart::flush() | |||
| void Uart::IrqHandler() | |||
| { | |||
| if (sercom->availableDataUART()) { | |||
| rxBuffer.store_char(sercom->readDataUART()); | |||
|
|
|||
| sercom->isFrameErrorUART(); | |||
There was a problem hiding this comment.
This line doesn't do anything useful, just read the error and ignore.
Should we also clear the frame error?
cores/arduino/Uart.cpp
Outdated
|
|
||
| sercom->isFrameErrorUART(); | ||
| uint8_t c=sercom->readDataUART(); | ||
| if( !sercom->isUARTError()){ |
There was a problem hiding this comment.
I think this should also check the error type here, from the data sheet not all errors will have data associated with them.
This fix allow to avoid the error related to the issue arduino-libraries/ArduinoModbus#2, i had tested it using the example of ArduinoRS485 and ArduinoModbus library, this fix allow to receive alligned all packet sented to the board from the shield MKR 485, without this the percentage of the packet loosed was of the 50%.
cores/arduino/Uart.cpp
Outdated
| uint8_t c=sercom->readDataUART(); | ||
| if( !sercom->isUARTError()){ | ||
| uint8_t c = sercom->readDataUART(); | ||
| if (!sercom->isUARTError()) { |
There was a problem hiding this comment.
I still think this condition is incorrect, please look at the data sheet again, not all error situations should result in us dropping the byte.
This fix allow to avoid the error related to the issue arduino-libraries/ArduinoModbus#2, i had tested it using the example of ArduinoRS485 and ArduinoModbus library, this fix allow to receive alligned all packet sented to the board from the shield MKR 485, as test i had use two boards with two mkr845 shield, one that blasting the other with pakcet, and the other that receive and show the byte received the sketch are the following: - receiver: https://github.com/bcmi-labs/ArduinoRS485/tree/master/examples/RS485Receiver; - Sender: https://github.com/bcmi-labs/ArduinoRS485/tree/master/examples/RS485Sender. the highest speed reachable was 500KHz that is unchangend respect the previous version.
cores/arduino/SERCOM.cpp
Outdated
| @@ -135,6 +135,11 @@ bool SERCOM::isUARTError() | |||
| return sercom->USART.INTFLAG.bit.ERROR; | |||
| } | |||
|
|
|||
| bool SERCOM:: isParityOrFrameError() | |||
| { | |||
| return sercom->USART.STATUS.bit.FERR | sercom->USART.STATUS.bit.PERR ; | |||
There was a problem hiding this comment.
here i think we can optimize this by doing a single read of .reg instead of .bit and comparing the bit mask
cores/arduino/SERCOM.h
Outdated
| @@ -164,6 +164,8 @@ class SERCOM | |||
| int writeDataUART(uint8_t data) ; | |||
| bool isUARTError() ; | |||
| void clearFrameErrorUART() ; | |||
| void clearParityErrorUART() ; | |||
| bool isParityOrFrameError(); | |||
There was a problem hiding this comment.
nit pick: extra space here
cores/arduino/Uart.cpp
Outdated
| } else { | ||
| sercom->readDataUART(); |
There was a problem hiding this comment.
I think we should add a comment above this live, saying read and discard the data, as it's invalid
cores/arduino/Uart.cpp
Outdated
| if (sercom->isFrameErrorUART()) { | ||
| sercom->clearFrameErrorUART(); | ||
| } | ||
| if (sercom->isFrameErrorUART()) { | ||
| sercom->clearFrameErrorUART(); |
There was a problem hiding this comment.
there's some weird spacing here? tabs vs spaces?
minor fix in Uart library
minor spacing
update comments
sandeepmistry
changed the title
More spacing
More spacing
|
@cmaglie can we please try to get this in before the next SAMD core release. Let me know if you have any comments. |
more spacing
| sercom->clearFrameErrorUART(); | ||
| } | ||
|
|
||
| if (sercom->isFrameErrorUART()) { |
There was a problem hiding this comment.
I think this block should check for parity error, not frame error again.
|
Closing in favour of #349. |
Introduced Frame Error check for the received byte