Busy loop is optimized out, preventing blinking

[pusherofbrooms]

After setting up my cross compiler as per the avr-rust/rust readme, and compiling the unmolested blink program, I upload to my arduino uno clone, and the LED (PORTB 5 or arduino pin 13) doesn't blink, or even come on. reversing the order of the writes doesn't seem to help, but if I add a "loop {}" after setting PORTB to 0xFF, the led will stay lit as I expect.

Is it possible that small_delay is getting optimized away? If I do something like

    loop {
        for _ in 0..100000 {
            unsafe { write_volatile(PORTB, 0x00) }
        }
        for _ in 0..100000 {
            unsafe { write_volatile(PORTB, 0xFF) }
        }
    }

I get a proper fast blink.

[pusherofbrooms]

I was reading rust-lang/rust#28728 about some loops being optimized away in LLVM. One comment suggested adding unsafe { asm!("" :::: "volatile")} to the loop to prevent its removal, and this seems to work:

    loop {
        for _ in 0..400000 {
            unsafe { asm!("" :::: "volatile")}
        }
        unsafe { write_volatile(PORTB, out) }
        out = out ^ 0xff;
    }

[shepmaster]

Hmm. Can you post the LLVM IR that your version of the program generates? The write_volatile should be marking the operations in a similar fashion to the volatile marker.

[pusherofbrooms]

I built with RUSTFLAGS="--emit=llvm-ir" XARGO_RUST_SRC=/home/jjorgens/dev/rust/avr/rust rustup run avr-toolchain xargo build --target avr-atmega328p --release
The IIR output is pretty short:

; ModuleID = 'blink.cgu-0.rs'
source_filename = "blink.cgu-0.rs"
target datalayout = "e-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
target triple = "avr-unknown-unknown"

; Function Attrs: norecurse noreturn nounwind uwtable
define void @main() unnamed_addr #0 {
start:
  store volatile i8 -1, i8* inttoptr (i16 36 to i8*), align 4
  br label %bb1

bb1:                                              ; preds = %bb1, %start
  store volatile i8 -1, i8* inttoptr (i16 37 to i8*), align 1
  store volatile i8 0, i8* inttoptr (i16 37 to i8*), align 1
  br label %bb1
}

; Function Attrs: norecurse nounwind readnone uwtable
define void @rust_eh_personality({}* nocapture, {}* nocapture) unnamed_addr #1 {
start:
  ret void
}

attributes #0 = { norecurse noreturn nounwind uwtable }
attributes #1 = { norecurse nounwind readnone uwtable }

!llvm.module.flags = !{!0}

!0 = !{i32 1, !"PIE Level", i32 2}

[pusherofbrooms]

If I were inclined to quick fix without digging into the root of the problem, the diff might look like

diff --git a/src/main.rs b/src/main.rs
index 9e01951..3165c9a 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,5 +1,5 @@
 #![feature(lang_items, unwind_attributes)]
-
+#![feature(asm)]
 #![no_std]
 #![no_main]
 
@@ -28,7 +28,9 @@ pub extern fn main() {
 
 /// A small busy loop.
 fn small_delay() {
-    for _ in 0..10000 { }
+    for _ in 0..400000 {
+        unsafe { asm!("" :::: "volatile")}
+    }
 }
 
 // These do not need to be in a module, but we group them here for clarity.

I changed 10000 to 400000 so I actually see the blink.

[shepmaster]

Oh! I totally confused this with my own blink repo which uses timers instead of a busy loop. No wonder I was so confused.

[shepmaster]

I think the root cause here is that I suggested that people start compiling in release mode, as opposed to the original debug mode. Of course the optimizer sees that loop and throws it away 😇However, we weren't able to build in debug mode due to the inherent code bloat. Hmm.

I don't think a busy loop is really the correct end solution, but if we want to prevent it from being removed, your solution is closest.

I think you can submit your change (moving the feature with the others in #![feature(lang_items, unwind_attributes)]) as a fix for now!

[pusherofbrooms]

#4

[pusherofbrooms]

Busy loop probably isn't right, but it's a pretty common first step in embedded land as an instructional aid as far as i can tell.

[dylanmckay]

I've raised avr-rust/rust-legacy-fork#85 so that we can develop a proper delay library.

Now that #4 has been merged, can this issue be closed?

[pusherofbrooms]

Closing is fine. I have my blink.