Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CTRP stack might lose packets #681

Closed
whoenig opened this issue Feb 5, 2021 · 3 comments
Closed

CTRP stack might lose packets #681

whoenig opened this issue Feb 5, 2021 · 3 comments
Assignees
@whoenig
Milestone
2021.03

Comments

@whoenig
Copy link
Contributor

whoenig commented Feb 5, 2021 •
edited by ataffanel
Loading

The current CRTP stack implementation has several potential places in the firmware, where packets can be lost if an internal queue is full, because return codes are not always checked or blocking calls are not used. This issue is for tracking changes that improve this handling in the STM firmware.
@whoenig whoenig self-assigned this Feb 5, 2021
whoenig added a commit that referenced this issue Feb 5, 2021
@whoenig
Enable uart-syslink asserts in release builds.
03e17e7 
We need to be notified if this is a problem during real operation. If
we ever see cases where this is hit, proper flow control between NRF51
and STM32 are needed.

Part of fixes for issue #681.
@whoenig whoenig mentioned this issue Feb 5, 2021
Merged
whoenig added a commit that referenced this issue Feb 6, 2021
@whoenig
Switch crtp service and platform service to be tasks
d6fe422 
These services respond to request and therefore should use the blocking
version of sendPacket that waits if the outgoing queue is full.

There are two other files that use the CRTP callback functionality
(commander and location service), but neither of them requires blocking
calls.

Part of fixes for issue #681.
@whoenig whoenig mentioned this issue Feb 6, 2021
Merged
whoenig added a commit that referenced this issue Feb 6, 2021
@whoenig
Change crtpSendPacket->crtpSendPacketBlock for request/respond CRTP msg
677a314 
For many CRTP calls, the client expects a response eventually. Using
crtpSendPacket has the risk that a response might not be send if the
queue is full. In these cases, it is better to temporarily block the
task and wait for the queue to become empty.

Part of fixes for issue #681.
@whoenig whoenig mentioned this issue Feb 6, 2021
Merged
whoenig added a commit that referenced this issue Feb 6, 2021
@whoenig
Use blocking enqueue calls for radiolink and crtp to avoid packet loss
761834f 
Part of fixes for issue #681.
@whoenig whoenig mentioned this issue Feb 6, 2021
Merged
@ataffanel ataffanel changed the title Safelink might lose packets CTRP stack might lose packets Feb 8, 2021
@ataffanel
Copy link
Member

Thanks for the issue and the PRs! I renamed the title and ticket to reflect that packets are lost in the CRTP stack and not in safelink (safelink only protects the radio packets, and so far it works).

ataffanel added a commit that referenced this issue Feb 9, 2021
@whoenig @ataffanel
Use blocking enqueue calls for radiolink and crtp to avoid packet loss (
d3d0044 
#687)

* Use blocking enqueue calls for radiolink and crtp to avoid packet loss

Part of fixes for issue #681.

* Assert in radiolink that we are not dropping pks

Co-authored-by: Wolfgang Hoenig <[email protected]>
Co-authored-by: Arnaud Taffanel <[email protected]>
whoenig added a commit that referenced this issue Feb 12, 2021
@whoenig
Add crtpsrv.echoDelay parameter
c16ad49 
When echoDelay is greater than 0, a slow CRTP task can be simulated for
system tests of the link.

Helps debugging/testing of issue #681.
@whoenig whoenig mentioned this issue Feb 12, 2021
Merged
@jonasdn
Copy link
Contributor

jonasdn commented Mar 18, 2021

Is this still an active issue? @ataffanel @whoenig ?

@whoenig
Copy link
Contributor Author

whoenig commented Mar 18, 2021

We can close this. The remaining work/bug is captured in #703.

@whoenig whoenig closed this as completed Mar 18, 2021
@knmcguire knmcguire added this to the next-release milestone Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@whoenig whoenig

Labels
None yet
Projects
None yet
Milestone
2021.03
Development

No branches or pull requests
4 participants
@jonasdn @whoenig @ataffanel @knmcguire