Examples: Added proper sleep mode control register config to uno-timer.rs

[jjstoo]

Previously in the example the sleep mode control register was never configured for sleep mode.

To do this, the sleep enable (SE) bit has to be set active. Due to the examples layout, it was not apparent that the controller didn't go to sleep, but it is easily proven by putting any kind of visible functionality inside the loop.

[Rahix]

Uff, thanks for catching this!

In the datasheet, I read

To avoid the MCU entering the sleep mode unless it is the programmer’s purpose, it is recommended to write the Sleep Enable (SE) bit to one just before the execution of the SLEEP instruction and to clear it immediately after waking up.

so maybe the register write should rather be next to the actual sleep instruction? It doesn't make a difference here, but we must expect that people just copy-paste the examples, so doing the "right thing" here is a good idea.

---

Going even one step further: Maybe avr-hal should provide a small helper which does the right sequence. So something like

pub fn enter_sleep(smcr: &pac::cpu::SMCR) {
    smcr.modify(|_, w| w.se().set_bit())
    avr_device::asm::sleep();
    smcr.modify(|_, w| w.se().clear_bit())
}

What do you think?

[jjstoo]

Sleep functionality being quite an important part of AVR programming, I really like the idea of having a helper function for it.

Having a bit of friendly abstraction for the different sleep modes wouldn't hurt either in the future :)

[Rahix]

Do you want to take a stab at adding the helper function or would you prefer me doing it?

Implementation wise, I think it needs to go into the atmega-hal, attiny-hal crates. Probably just into the top-level lib.rs for now. An important part is feature-gating it on #[cfg(feature = "device-selected")].

[jjstoo]

I'd be happy to try and implement the changes myself. I'll start by adding the "go to sleep" -helper to the crates mentioned by you.

[mayms]

I'm also interested in this. I have a working example with asm macro for using power-down with sleep and waking up via wdt interrupt. I tried to rewrite my asm code with your example but it didn't work.

You could find the whole example here: https://github.com/mayms/stuff/blob/main/nano-power-down/src/main.rs

Working asm code:

    let mut wdtcsr: u8 = 0; // read WDTCSR
    let mut smcr: u8 = 0; // read SMCR
    unsafe {
        asm!(
        "CLI",
        "WDR",
        "STS 0x60, {0}",    // WDTCSR
        "STS 0x60, {1}",    // WDTCSR
        "out 0x33, {2}",    // SMCR
        "SEI",
        "LDS {3}, 0x60",    // WDTCSR,
        "in {4}, 0x33",      // SMCR
        //========WWWW_WWWW
        //========DDDD_DDDD
        //========IIPC_EPPP
        //========FE3E__210
        in(reg) 0b0001_1000 as u8, // WDCE=1, WDE=1
        in(reg) 0b0110_0000 as u8, // WDIE 1, WDP3 1, WDE 0
        //--------XXXX_SSSS
        //--------XXXX_MMME
        //--------XXXX_210-
        in(reg) 0b0000_0101 as u8,  // Power down SM2=0, SM1=1, SM0=0, SE=1
        out(reg) wdtcsr,
        out(reg) smcr
        );
    }

Not working adaption:

    avr_device::interrupt::free(|_| {
        dp.WDT.wdtcsr.modify(|_, w| w
            .wdce().set_bit()
            .wde().set_bit());
        dp.WDT.wdtcsr.modify(|_, w| w
            .wdce().clear_bit()
            .wdie().set_bit()
            .wde().clear_bit()
            .wdph().set_bit());
        dp.CPU.smcr.modify(|_, w| w
            .sm().bits(0b010)// power-down sleep mode
            .se().set_bit());
    });
    unsafe { avr_device::interrupt::enable() };

Any idea what I missed?

[Rahix]

@mayms, the equivalent of your asm!() should be this:

    avr_device::interrupt::free(|_| {
        avr_device::asm::wdr();
        dp.WDT.wdtcsr.write(|w| w.wdce().set_bit().wde().set_bit());
        dp.WDT.wdtcsr.write(|w| {
            w.wdce()
                .clear_bit()
                .wdie()
                .set_bit()
                .wde()
                .clear_bit()
                .wdph()
                .set_bit()
        });
        dp.CPU.smcr.write(|w| w.sm().bits(0b010).se().set_bit());
    });
    let wdtcsr = dp.WDT.wdtcsr.read().bits();
    let smcr = dp.CPU.smcr.read().bits();

The generated assembly is slightly different, it looks like this:

in	r17, 0x3f	; 63
cli
wdr
ldi	r24, 0x18	; 24
sts	0x0060, r24	; 0x800060 <__TEXT_REGION_LENGTH__+0x7f8060>
ldi	r24, 0x60	; 96
sts	0x0060, r24	; 0x800060 <__TEXT_REGION_LENGTH__+0x7f8060>
ldi	r24, 0x02	; 2
call	0x12e	; 0x12e <<T as core::convert::From<T>>::from::heefc31865f28f97a>
add	r24, r24
andi	r24, 0x0E	; 14
ori	r24, 0x01	; 1
out	0x33, r24	; 51
out	0x3f, r17	; 63

I didn't check, but hopefully this works better?

A few notes about what is different:

• Your use of .modify(...) results in a read-modify-write cycle. You really want to just write a single value so using .write(...) is more appropriate.
• You'll notice the missing sei. That's by design - instead, the previous interrupt state is restored at the appropriate time. So you'll need a unsafe { avr_device::interrupt::enable() }; somewhere before or after this code to actually enable interrupts.
• I'm not sure why that SMCR write isn't using a literal. Seems some optimizations aren't triggering due to that convert::From call. If you need absolutely minimal code footprint, you can replace it with a bare bits write like

  dp.CPU.smcr.write(|w| unsafe { w.bits(0b0000_0101) });

[mayms]

@Rahix, thank you for taking time to solve my mistake and for giving me such a detailed response. Your solution works fine. Minimal code footprint is not a requirement for now. Next thing on my list is to deactivate BOD to bring consumption further down. What do you think about lib support. Maybe I can help there.