Skip to content

Commit d871bf3

Browse files
TheZoq2adamgreig
andcommitted
Fix race condition causing ADCs to read stale values
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 <adam@adamgreig.com>
1 parent 1cb9a17 commit d871bf3

File tree

2 files changed

+24
-2
lines changed

2 files changed

+24
-2
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
2727
### Fixed
2828

2929
- Fix PWM on `TIM1`
30+
- Fix ADC race condition causing incorrect reads at certain frequencies
3031

3132
## [v0.5.3] - 2020-01-20
3233

src/adc.rs

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -349,13 +349,34 @@ macro_rules! adc_hal {
349349
self.rb.sqr1.modify(|_, w| w.l().bits((len-1) as u8));
350350
}
351351

352+
/**
353+
Performs an ADC conversion
354+
355+
NOTE: Conversions can be started by writing a 1 to the ADON
356+
bit in the `CR2` while it is already 1, and no other bits
357+
are being written in the same operation. This means that
358+
the EOC bit *might* be set already when entering this function
359+
which can cause a read of stale values
360+
361+
The check for `cr2.swstart.bit_is_set` *should* fix it, but
362+
does not. Therefore, ensure you do not do any no-op modifications
363+
to `cr2` just before calling this function
364+
*/
352365
fn convert(&mut self, chan: u8) -> u16 {
353-
self.rb.cr2.modify(|_, w| w.align().bit(self.align.into()));
366+
// Dummy read in case something accidentally triggered
367+
// a conversion by writing to CR2 without changing any
368+
// of the bits
369+
self.rb.dr.read().data().bits();
370+
354371
self.set_channel_sample_time(chan, self.sample_time);
355372
self.rb.sqr3.modify(|_, w| unsafe { w.sq1().bits(chan) });
356373

357374
// ADC start conversion of regular sequence
358-
self.rb.cr2.modify(|_, w| w.swstart().set_bit());
375+
self.rb.cr2.modify(|_, w|
376+
w
377+
.swstart().set_bit()
378+
.align().bit(self.align.into())
379+
);
359380
while self.rb.cr2.read().swstart().bit_is_set() {}
360381
// ADC wait for conversion results
361382
while self.rb.sr.read().eoc().bit_is_clear() {}

0 commit comments

Comments
 (0)