fix improper reporting of DMA overruns when remaining bytes is outside the expected range #126
Conversation
…e the expected range I've witnessed zeros as well as values larger than the initial setting, albeit spuriously. Added a saturating subtraction and a modulo to the position calculation and overrun now seems to be working correctly There still seems to be more overruns than should strictly be happening, but I've been unable to pin them down and they disappear when clocking low enough. They might even be real but it is difficult to know. But this fix is solid
src/adc.rs
Outdated
| @@ -482,7 +482,7 @@ impl Buffer { | |||
|
|
|||
| // Let's translate what we got from the DMA peripheral into a write | |||
| // position that we can compare with our read position. | |||
| let pos = self.len - remaining; | |||
| let pos = self.len.saturating_sub(remaining) % self.len; | |||
There was a problem hiding this comment.
I don't understand what's happening here. First off, it's very weird that the hardware reports values for remaining that are too large, but okay, hardware is often quite weird :-)
Still, it's not clear to me what should happen in that case. The saturating sub will prevent an underflow, which is good, but are we certain that it will lead to correct behavior? Maybe there's an errata note somewhere that can shed some light on the hardware behavior.
I also don't understand what the modulo is doing. Will self.len.saturating_sub(remaining) not always result in a value that's smaller or equal to self.len? In that case the % self.len won't do anything, or am I missing something obvious?
|
Thank you, @electroCutie. I've left a comment in your commit. Some additional notes:
|
…ting subtract I can't get it to do load an erroneous value into the DNR register anymore, so I will chaulk that one up to potential operator error unless it manifests again and I get a solid repro
| let pos = self.len.saturating_sub(remaining) % self.len; | ||
| // Zero can be witnessed in remaining before the DMA reloads | ||
| // the register so we wrap the result of the subtraction around | ||
| let pos = (self.len - remaining) % self.len; |
There was a problem hiding this comment.
Okay, so the thing I missed before is that if remaining is zero, the % self.len will result in pos = 0 instead of pos = self.len. Makes sense.
What I don't understand yet is the wider context of this change. Under which circumstances will remaining == 0 actually cause an overrun, and why? How will this affect overrun detection?
I'm not saying this is wrong, but I'm also not convinced of the following things:
- That this is correct, given the complexity of the surrounding code.
- That the overruns you're seeing are actually caused by
remaining == 0, given the known issue with ADC/DMA. - If you're experiencing overruns not caused by Faulty DMA Buffer Overflow in adc_trig example #104, that this is the right place to fix them. Maybe it should be fixed in the overrun detection code, for example.
This code is quite complex, and we're really suffering from the lack of a test suite here. I think the best way to get this merged is to do a more thorough analysis and write up the results in a comment in the appropriate place. As it is, I don't understand this fix (and its underlying problem) well enough to merge it.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sounds good!
Please note that my latest conclusion was, that this API can't be supported given the hardware behavior (#104 (comment)), and I currently favor just adopting @apgoetz's new API to replace this one (#105 (review)).
But if you can come up with a fix for the current API, that would of course be great.
I've witnessed zeros as well as values larger than the initial setting,
albeit spuriously. Added a saturating subtraction and a modulo to the
position calculation and overrun now seems to be working correctly
There still seems to be more overruns than should strictly be happening,
but I've been unable to pin them down and they disappear when clocking
low enough. They might even be real but it is difficult to know. But
this fix is solid
Fix for #125