From 255c2b57b29e7db10b42274e2e6cdef27f04a182 Mon Sep 17 00:00:00 2001
From: Adam Green <adamgreen@users.noreply.github.com>
Date: Mon, 19 Nov 2018 01:31:38 -0800
Subject: [PATCH] Move "last_state = state" assignment

I believe that I have found a bug in the samples found in the
concurrency chapter where the value of the `last_state` variable
should be updated on each loop iteration but it is only updated
on the detection of the first leading edge. For example:

    loop {
        let state = read_signal_level();
        if state && !last_state {
            last_state = state;
            // DANGER - Not actually safe! Could cause data races.
            unsafe { COUNTER += 1 };
        }
    }

I think that loop should actually be written as:

    loop {
        let state = read_signal_level();
        if state && !last_state {
            // DANGER - Not actually safe! Could cause data races.
            unsafe { COUNTER += 1 };
        }
        last_state = state;
    }

As the code exists today, I think it would detect the first leading
edge, set last_state to true and then never be updated again which
would also mean that it never detects another leading edge.
---
 src/concurrency/index.md | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/concurrency/index.md b/src/concurrency/index.md
index 208c05d..949f2d6 100644
--- a/src/concurrency/index.md
+++ b/src/concurrency/index.md
@@ -68,10 +68,10 @@ fn main() -> ! {
     loop {
         let state = read_signal_level();
         if state && !last_state {
-            last_state = state;
             // DANGER - Not actually safe! Could cause data races.
             unsafe { COUNTER += 1 };
         }
+        last_state = state;
     }
 }
 
@@ -109,12 +109,12 @@ fn main() -> ! {
     loop {
         let state = read_signal_level();
         if state && !last_state {
-            last_state = state;
             // New critical section ensures synchronised access to COUNTER
             cortex_m::interrupt::free(|_| {
                 unsafe { COUNTER += 1 };
             });
         }
+        last_state = state;
     }
 }
 
@@ -172,10 +172,10 @@ fn main() -> ! {
     loop {
         let state = read_signal_level();
         if state && !last_state {
-            last_state = state;
             // Use `fetch_add` to atomically add 1 to COUNTER
             COUNTER.fetch_add(1, Ordering::Relaxed);
         }
+        last_state = state;
     }
 }
 
@@ -254,10 +254,10 @@ fn main() -> ! {
     loop {
         let state = read_signal_level();
         if state && !last_state {
-            last_state = state;
             // No unsafe here!
             interrupt::free(|cs| COUNTER.increment(cs));
         }
+        last_state = state;
     }
 }
 
@@ -353,10 +353,10 @@ fn main() -> ! {
     loop {
         let state = read_signal_level();
         if state && !last_state {
-            last_state = state;
             interrupt::free(|cs|
                 COUNTER.borrow(cs).set(COUNTER.borrow(cs).get() + 1));
         }
+        last_state = state;
     }
 }
 
@@ -448,14 +448,13 @@ fn main() -> ! {
         });
 
         if state && !last_state {
-            last_state = state;
-
             // Set PA1 high if we've seen a rising edge on PA0.
             interrupt::free(|cs| {
                 let gpioa = MY_GPIO.borrow(cs).borrow();
                 gpioa.as_ref().unwrap().odr.modify(|_, w| w.odr1().set_bit());
             });
         }
+        last_state = state;
     }
 }