-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
Fix #10804 by running continuous ADC DMA in endless loop instead of restarting after each run to avoid losing samples (IDFGH-10235) #11500
Conversation
…tead of restarting after each run (descriptor chain) to avoid losing samples. Use descriptor error callback for GDMA to check for DMA buffer overrun.
DMA EOF may happens per multiple dma descriptors, instead of only one. Closes #11500
DMA EOF may happens per multiple dma descriptors, instead of only one. Closes #11500
@Icarus113 I think the changes introduced in 9bec423 don't guarantee consistency of the DMA buffers and contain a race condition, i.e. #10804 would not be fully fixed:
E.g. when I think this can't be properly implemented without providing a function Since a descriptor error callback is not used, we don't get notified if DMA attempts to overwrite the buffer before setting It could be fixed like this; adc_hal_dma_desc_status_t adc_hal_get_reading_result(adc_hal_dma_ctx_t *hal, const intptr_t eof_desc_addr, uint8_t **buffer, uint32_t *len)
{
...
//Find the eof list start
eof_desc = eof_desc->next;
buffer_start = eof_desc->buffer;
eof_len += eof_desc->dw0.length;
if ((intptr_t)eof_desc == eof_desc_addr) {
goto valid;
}
//Find the eof list end
for (int i = 1; i < hal->eof_step; i++) {
eof_desc = eof_desc->next;
eof_len += eof_desc->dw0.length;
if ((intptr_t)eof_desc == eof_desc_addr) {
goto valid;
}
}
valid:
*buffer = buffer_start;
*len = eof_len;
return ADC_HAL_DMA_DESC_VALID;
}
void adc_hal_read_desc_finish(adc_hal_dma_ctx_t *hal, const intptr_t eof_desc_addr) {
dma_descriptor_t *eof_desc = hal->cur_desc_ptr;
//Find the eof list start
eof_desc = eof_desc->next;
eof_desc->dw0.owner = 1;
//Find the eof list end
for (int i = 1; ((intptr_t)eof_desc != eof_desc_addr) && i < hal->eof_step; i++) {
eof_desc = eof_desc->next;
eof_desc->dw0.owner = 1;
}
hal->cur_desc_ptr = eof_desc;
} |
Hi @Erlkoenig90 , yes, this is actually the competition between the A. software (dealing with the ready dma desc) and B. hardware (moving new dma data). If the A is less than B then
We adopted the DMA rx desc err event PR. For ADC side we didn't adopt the error log logic yet. On chips later than S2, the ADC sampling speed is not very fast, so this shouldn't be an issue. May I know if you observe that under certain condition A is less than B? |
Hi @Icarus113 , I did not observe this in practice (yet). I just thought this might potentially be an issue from looking at the code and that it might be worth fixing before an issue arises. This also depends on CPU load and interrupt load; an application with high CPU load might experience this problem (when the interrupt is processed right before the DMA attempts to re-use the buffer). This would cause data corruption which would be hard to debug. |
The downside for your solution of delaying the
So if it's really in an extreme condition for example
Another condition is:
So changing the descriptor update timing isn't a good way for such condition. A better solution is to avoid such extreme condition. There are few solutions when designing the driver:
So by increasing these we can avoid the extreme condition. On the other hand,
Having a tip for this condition is acceptable. We can try to plan it then and give some hints accordingly. |
I think it is better to get a DMA error rather than having the DMA causing data corruption by overwriting a buffer that is still being copied to the ringbuffer ("silent error").
Yes, but my suggestion does not attempt to solve the issue, it just allows the application developer to notice the issue (by getting a DMA error) at all. |
This fixes #10804 by running the ADC DMA in a continuous loop. A function
adc_hal_read_desc_finish
is introduced to set a DMA descriptor'sowner
field to1
such that it can be re-used by DMA.As discussed in #11411, this requires PR #11499 to be merged too as it relies on the new
on_descr_err
callback to detect/debug GDMA buffer overruns.