nrf_rpc: improve UART transport stability #21013
Conversation
Damian-Nordic
commented
Mar 18, 2025
Damian-Nordic
commented
- Fix one possible racy scenario in which both nRF RPC peers start at the same time
- Fix HDLC decoding and handling too long frames
- Improve ACK handling to avoid spurious retransmissions
github-actions
bot
added
manifest
changelog-entry-required
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: daafadfd3f51a35894cc093b3a8659b6fb7749eb more detailssdk-nrf:
nrfxlib:
Github labels
List of changed files detected by CI (3)Outputs:ToolchainVersion: 4ffa2202d5 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
subsys/nrf_rpc/nrf_rpc_uart.c
Outdated
| @@ -176,39 +184,49 @@ static bool crc_compare(uint16_t rx_crc, uint16_t calc_crc) | |||
| return rx_crc == calc_crc; | |||
| } | |||
|
|
|||
| static void hdlc_decode_byte(struct nrf_rpc_uart *uart_tr, uint8_t byte) | |||
| static void hdlc_decode_byte(enum hdlc_state *state, uint8_t *out, size_t *len, size_t capacity, | |||
There was a problem hiding this comment.
please stick with struct nrf_rpc_uart *uart_tr.
Returning through pointers is bug prone and you (as in this case) quickly go beyond 4 function args
There was a problem hiding this comment.
Yeah, but I wanted to reuse this function for 2 decoding states (for decoding ACKs in ISR and long packets in work queue). I thought about abstracting struct decoding_state and passing just one member instead of all these pointers, but this would result in a small RAM increase. Though I think the increase is acceptable in this case for the sake of cleaner code.
There was a problem hiding this comment.
Added struct hdlc_decode_ctx to reduce the number of input arguments.
subsys/nrf_rpc/nrf_rpc_uart.c
Outdated
| __fallthrough; | ||
| case HDLC_STATE_FRAME: | ||
| if (byte == HDLC_CHAR_DELIMITER) { | ||
| if (*len > 0) { |
There was a problem hiding this comment.
what if delimiter is found without prior data? is this empty frame or protocol does not allow this?
There was a problem hiding this comment.
HDLC defines that the delimiter should be present at the start and end of the frame but if 2 frames are sent back to back you can either send two delimiters (end of the previous frame and start of the next frame) or just one. I think this implies that empty frames cannot be supported.
1. Skip incoming bytes while in the unsync state. 2. Handle receive buffer overflow. 3. Prepare hdlc_decode_byte() for multiple simultaneous decoding states. Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
Damian-Nordic
force-pushed
the
nrf_rpc_ack_isr
branch
from
3f0ceaf to
78462b2
Compare
Currently, decoding both normal packets and ACKs happens in the receive thread. This imposes certain limitations, for example the receive thread shall not send any packets as this would introduce a chicken-egg problem: the thread is waiting for an ACK but the ACK can only be decoded by the current thread. More importantly, this can also introduce spurious ACK timeouts and retransissions because the receive thread may be blocked when trying to push a packet to the thead pool. Move the ACK decoding to ISR to overcome the mentioned issues. Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
Fix one missed scenario in which transport_initialized flag may be false because of the thread running nrf_rpc_init() being switched out by the scheduler before setting this flag. Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
Damian-Nordic
force-pushed
the
nrf_rpc_ack_isr
branch
from
78462b2 to
daafadf
Compare
|
|
You can find the documentation preview for this PR here. |
