Fix race condition causing ADCs to read stale values #203
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was a *fun* bug to track down. Initially, it appeared as an out of
order read of multiple ADC channels (the second would read the first
channel, the third from the second, and so on). This only happened with
a prescaler value of `8`, for other values, the corect behaviour was
observed.
`git bisect` traced the issue back to a version bump of the `pac` crate
which shouldn't have affected things. Further git bisection hinted at
the commit bumping version number in the PAC was the issue. This is of
course nonsensical, and the key change that caused the **symptoms** was
a bump of the svd2rust crate, from 0.14 to 0.15.
Dumping the memory in the ADC and RCC peripherals showed barely any
difference, apart from the newer `pac` version randomly having the
`swstart` and `eoc` bits set after the second line of the convert
function. After further single stepping, those bits were set after a
write of `0001e0001` to `cr2`. This was the value that was already in
the register, so why would that start a conversion?
Well, someone decided that it would be a good idea to make one way of
starting the ADC conversion to write a `1` to the `adon` bit if the
`adon` bit is already one. But that might trigger a read when you want
to modify other parts of the register, so this behaviour only happens if
no other bits are modified. Thus, `modify` on the `cr2` register will
start a conversion unless at least one bit is *actually* modified.
This was accidentally done as the first part of the `convert` function,
which started a conversion which would be ready roughly by the time the
CPU starts checking for `eoc` bits.
The race condition occurs if the CPU gets to the `EOC` check, before the
ADC has time to reset the bit as a result of the `swstart` trigger. This
relies on the CPU being faster than the ADC clock which explains the
behaviour at high prescalers.
Finally, it seems like the line
```rust
while self.rb.cr2.read().swstart().bit_is_set() {}
```
*should* have prevented this as the ADC should have taken care of the
`swstart` and reset the `EOC` before that line. For whatever reason,
that did not happen
Co-authored-by: Adam Greig <[email protected]>
|
I forgot to mention that we think this commit fixes the symptoms of this issue, but it is still possible for it to occur. There is no way to tell if the ADC is currently doing a conversion, so if a conversion is accidentally started right before calling |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #201
This was a fun bug to track down.
Some decided that it would be a good idea to make one way of starting the ADC conversion to write a
1to theadonbit if theadonbit is already one. But that might trigger a read when you want to modify other parts of the register, so this behaviour only happens if no other bits are modified. Thus,modifyon thecr2register will start a conversion unless at least one bit is actually modified.This was accidentally done as the first part of the
convertfunction, which started a conversion which would be ready roughly by the time the CPU starts checking foreocbits.The race condition occurs if the CPU gets to the
EOCcheck, before the ADC has time to reset the bit as a result of theswstarttrigger. This relies on the CPU being faster than the ADC clock which explains the behaviour at high prescalers.Finally, it seems like the line
should have prevented this as the ADC should have taken care of the
swstartand reset theEOCbefore that line. For whatever reason, that did not happenThnaks @adamgreig for helping debug this :), and for a more detailed log of the steps we used to track it down, see the commit message.