Fix RTT read that wraps around up buffer. Do retries #157
Conversation
rgrr
left a comment
rgrr
left a comment
There was a problem hiding this comment.
Thanks for the PR. See my comments
| unsigned int first_chunk_size; | ||
|
|
||
| if (ft_extRttBuf->aUp.WrOff > ft_extRttBuf->aUp.RdOff) { | ||
| bytes_available = ft_extRttBuf->aUp.WrOff - ft_extRttBuf->aUp.RdOff; |
There was a problem hiding this comment.
To be honest, I'm claiming that my code already considers wrap-around correctly. This would be almost immediately visible otherwise.
Please prove me wrong.
There was a problem hiding this comment.
You are probably right, but I don't know where the wrap-around logic is, can you show me?
There was a problem hiding this comment.
There it is:
if (ft_extRttBuf->aUp.WrOff > ft_extRttBuf->aUp.RdOff) {
ft_cnt = MIN(ft_cnt, ft_extRttBuf->aUp.WrOff - ft_extRttBuf->aUp.RdOff);
}
else {
ft_cnt = MIN(ft_cnt, ft_extRttBuf->aUp.SizeOfBuffer - ft_extRttBuf->aUp.RdOff);
}
ft_cnt = MIN(ft_cnt, sizeof(ft_buf));There was a problem hiding this comment.
So if WrOff < RdOff (write is below read), ft_cnt becomes the amount from RdOff to the end of the buffer, and doesn't copy until WrOff (does not wrap around). Why is it working, then? Is it reading the remainder (from start of buffer until WrOff) at a later execution of the loop? This requires an extra wake up from xEventGroupWaitBits, in that case?
There was a problem hiding this comment.
Yes, the transfer is split in at least two on wrap-around. I think this is nothing special and nothing dangerous. The code stays simple, that's more important (to me).
There was a problem hiding this comment.
When does the read of the remainder occurs? What if there is no more Event posted for a while, then the rest of the data is delayed? Did I get this right?
There was a problem hiding this comment.
On next iteration. Delay is neglectable.
| #define SWD_RETRY_MS 2 | ||
| #define SWD_MAX_RETRIES 10 | ||
|
|
||
| #define SWD_OPERATION_WITH_RETRY(result, operation) \ |
There was a problem hiding this comment.
do you experience problems requiring retry mechanism?
There was a problem hiding this comment.
Yes, but it is probably caused by a noisy line. Dealing with electrical motors here.
There was a problem hiding this comment.
Ok, understood. But in this case shouldn't the retry mechanism go into a deeper layer?
There was a problem hiding this comment.
Hmmm... checking the code in swd_host.c shows that there are already retries, see swd_transfer_retry(). Yet not sure if this is called by all relevant SWD routines. But I think this is the actual place for a lever.
There was a problem hiding this comment.
I'm wondering if your retry implementation solves your actual problem.
There was a problem hiding this comment.
What I've been seeing is that an erroneous read (possibly caused by noisy line, hard to do something about it 100 %), triggered a reconnect of the probe. That takes about half a second. The thought around this (which I am happy to see you challenge) was to lose less data.
There was a problem hiding this comment.
Question is: do you recognize any change?
2 participants
No description provided.