Skip to content

Add 'Wake CPU' functionality #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2025
Merged

Conversation

RossPorter506
Copy link
Contributor

Following on from the discussion here and here, This PR adds an optional argument to the #[interrupt] proc-macro that returns the CPU to Active Mode after the interrupt completes.
This behaviour is usually controlled by the status register, which can be freely written to outside of an interrupt. However, if the CPU is turned off then the only time the CPU wakes up again is during an interrupt. Unfortunately the status register can't be naively written to during an interrupt, as the previous state of the status register is pushed onto the stack before an interrupt begins and this value is restored after the interrupt returns, making any changes to the status register itself during an interrupt temporary.
To make permanent changes to the system state we must modify the copy of the status register that was pushed to the stack. In msp430-gcc this is handled by a compiler intrinsic, which isn't available to us here. Instead, we can use a naked function to ensure that nothing is pushed to the stack before our code runs (proof of concept), modify the item most recently pushed to the stack (the copy of the status register), then run the user interrupt handler.

This is implemented as an optional argument to the #[interrupt] proc-macro. The behaviour of #[interrupt] is untouched, and the new functionality is enabled when #[interrupt(wake_cpu)] is used.

As a refresher, consider the existing interrupt handler macro:

#[interrupt]
fn PORT2() {
    // User code here
}

This expands to the following:

#[export_name = "PORT2"]
extern "msp430-interrupt" fn pts49692f61y5co8() {
    interrupt::PORT2;
    #[inline(always)]
    fn pts49692f61y5co8<'a>() {
        // User code here
    }
    { pts49692f61y5co8() }
}

With the new wake_cpu option:

#[interrupt(wake_cpu)]
fn PORT2() {
    // User code here
}

expands to:

#[export_name = "PORT2"]
#[unsafe(naked)]
unsafe extern "msp430-interrupt" fn ot1o618qmf07394p() {
    #[inline(always)]
    extern "msp430-interrupt" fn ot1o618qmf07394p<'a>() {
        interrupt::PORT2;
        // User code here
    }
    {
        // Clear SCG1, SCG0, OSC_OFF, CPU_OFF in saved copy of SR register on stack
        const MASK: u8 = (1 << 7) + (1 << 6) + (1 << 5) + (1 << 4);
        asm!("
            bic.b #{1}, 0(r1)
            jmp {0}", 
            sym ot1o618qmf07394p, 
            const MASK
        );
    }
}

All the bits related to low power modes are cleared, as only clearing CPU_OFF may put the MSP430 into one of the undocumented power modes.

This isn't an ideal solution, as we still can't conditionally write to the copy of the status register on the stack like you could with the compiler intrinsics (e.g. GCC has __bic_sr_register_on_exit() which can be called anywhere within an interrupt handler)

A full working example can be seen here, which runs on an MSP-EXP430FR2355 dev board (build with cargo run --example lpm0).

Any comments or modifications welcome.

Returns the CPU to active mode after the interrupt returns
@cr1901
Copy link
Contributor

cr1901 commented Jun 26, 2025

This looks fine to me (also, this is how I learn naked functions are stable), but I have at least one comment before I merge:

This isn't an ideal solution, as we still can't conditionally write to the copy of the status register on the stack like you could with the compiler intrinsics.

It's not a good API, but I think it would be possible to conditionally return whether to wake the CPU using a return argument or inout parameter. I have in mind a repr(transparent) newtype wrapping an enum with WakeCpu/SleepCpu (it has to be transparent, since when we return from the user portion of an interrupt handler). Perhaps massaging this into a good API is a useful future direction?

@RossPorter506
Copy link
Contributor Author

RossPorter506 commented Jun 27, 2025

Some sort of conditional logic would be nice, though for now it's easy enough to check for the wake up condition in the main function, and if not satisfied then go straight back to sleep. Waking up only to go back to sleep does waste a bit of power, but the system already woke up to deal with the interrupt, so it's probably not too bad.

Between an inout and a return value I think an inout would be more ergonomic for the user - if they only use it in one control path then that's the only place they need to touch it, unlike a return value which needs a value in every control path. It also leaves the door open for implementing other methods, like generic set_bits() and clear_bits() methods to allow for going between any two arbitrary states, not just back to active mode (I suppose you could implement this with a return value too, but you'd have to do all of that logic in the naked assembly).
Having said that, I think a return value would be easier to implement, and construction of the return value can be deferred until after the user has done all the latency-sensitive things in their interrupt handler, unlike the inout which will need to be initialised before the inner handler is called.

@pftbest
Copy link
Contributor

pftbest commented Jun 27, 2025

I don't think it's possible to return anything from a function marked as "msp430-interrupt", because such functions end with RETI instead of RET instruction which directly end the interrupt. Also I don't remember if you can even pass an argument to such function.

@RossPorter506
Copy link
Contributor Author

Yeah, you'd have to make the inner handler not "msp430-interrupt", do a proper function call in the naked assembly (rather than jmp), then after any status register modifications based on the inout/return value you could add a reti to the bottom of the outer handler. Not to mention pushing the program counter, pushing/popping any clobbered registers, etc. It would definitely be a lot more involved than what we have here.

@cr1901
Copy link
Contributor

cr1901 commented Jun 27, 2025

Yea, I knew there was a reason my proposal wouldn't work as-is... I just forgot why :). Changing the inner function to be non-msp430-interrupt would indeed be involved.

though for now it's easy enough to check for the wake up condition in the main function, and if not satisfied then go straight back to sleep.

This'll do for now. At least LPM is now implemented :D!

@cr1901
Copy link
Contributor

cr1901 commented Jun 27, 2025

Ignore failed CI... I have to update the workflow, but lately I've not had bandwidth: #23 (comment)

@cr1901 cr1901 merged commit 258b6cf into rust-embedded:master Jun 27, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants