Closures and FnMut

Forgive my bringing up a perennial and not figuring it out myself.

I'm playing with GPIO and the rppal crate on a Raspberry Pi. Until I got stalled. I have simplified my code to the point I know it doesn't make sense, but it reproduces my error.

Cargo.toml has in it:

[dependencies]
rppal = "0.16.0"

main.rs is:

use std::sync::{Arc, Mutex};
use rppal::gpio::{Gpio, Level, Trigger};

pub struct Keybow {}
impl Keybow {
    fn debounce_key(key_state: Arc<Mutex<KeyStateData>>, down: bool) {
    }
}

pub struct KeyStateData {}

fn main() {
    let one_key_state = &Arc::new(Mutex::new(KeyStateData {}));
    let one_key_state_cloned = Arc::clone(one_key_state);

    let one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();
    
    let mut my_closure = move |level| Keybow::debounce_key(
        one_key_state_cloned,
        level_to_bool(level));
    //my_closure(rppal::gpio::Level::High); // I can call this directly

    one_key.set_async_interrupt(Trigger::Both, my_closure); // Registering to be called in another thread.
}

fn level_to_bool(level: Level) -> bool {
    match level {
        rppal::gpio::Level::High => false,
        rppal::gpio::Level::Low => true,
    }
}

And my error is:

kentborg@tinpan:~/programming/rust-code/fnmut-puzzle$ cargo build
   Compiling fnmut-puzzle v0.1.0 (/home/kentborg/programming/rust-code/fnmut-puzzle)
error[E0525]: expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce`
   --> src/main.rs:18:26
    |
18  |     let mut my_closure = move |level| Keybow::debounce_key(
    |                          ^^^^^^^^^^^^ this closure implements `FnOnce`, not `FnMut`
19  |         one_key_state_cloned,
    |         -------------------- closure is `FnOnce` because it moves the variable `one_key_state_cloned` out of its environment
...
23  |     one_key.set_async_interrupt(Trigger::Both, my_closure);
    |             -------------------                ---------- the requirement to implement `FnMut` derives from here
    |             |
    |             required by a bound introduced by this call
    |
note: required by a bound in `InputPin::set_async_interrupt`
   --> /home/kentborg/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rppal-0.16.1/src/gpio/pin.rs:523:12
    |
521 |     pub fn set_async_interrupt<C>(&mut self, trigger: Trigger, callback: C) -> Result<()>
    |            ------------------- required by a bound in this associated function
522 |     where
523 |         C: FnMut(Level) + Send + 'static,
    |            ^^^^^^^^^^^^ required by this bound in `InputPin::set_async_interrupt`

For more information about this error, try `rustc --explain E0525`.
error: could not compile `fnmut-puzzle` (bin "fnmut-puzzle") due to 1 previous error

As far as I understand the error is because being mutable and shared is a problem. So I figure the data being shared being in an Arc<Mutex<>> would let me pass a clone of it into that callback.

But I'm so confused I don't know whether I am approaching it all wrong or whether I am just missing a & somewhere.

Thanks,

-kb

This is just an ownership problem, at least so far. debounce_key needs ownership, which means the closure can only be called once. In order to satisfy debounce_key, you need to create a new value every time the closure is called.

fn main() {
    let one_key_state = Arc::new(Mutex::new(KeyStateData {}));

    let one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();
    
    let mut my_closure = move |level| {
        let one_key_state_cloned = Arc::clone(&one_key_state);
        Keybow::debounce_key(
            one_key_state_cloned,
            level_to_bool(level)
        )
    };
    //my_closure(rppal::gpio::Level::High); // I can call this directly

    one_key.set_async_interrupt(Trigger::Both, my_closure); // Registering to be called in another thread.
}

If you also need one_key_state outside the closure, you'll need to clone it both outside (to make one to keep) and inside (to make one each time the closure is called).

fn main() {
    let one_key_state = Arc::new(Mutex::new(KeyStateData {}));
    let one_key_state_cloned = Arc::clone(&one_key_state);

    let one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();
    
    let mut my_closure = move |level| {
        let one_key_state_cloned_again = Arc::clone(&one_key_state_cloned);
        Keybow::debounce_key(
            one_key_state_cloned_again,
            level_to_bool(level)
        )
    };
    //my_closure(rppal::gpio::Level::High); // I can call this directly

    one_key.set_async_interrupt(Trigger::Both, my_closure); // Registering to be called in another thread.
}

Note that unless debounce_key needs to be in charge of keeping the Arc alive, it's better to have it take &Mutex<KeyStateData>. Then you don't need to clone inside the closure. The closure will generate a new reference each time it's called using the one Arc. Right now, the closure is already keeping the Arc alive, so this is probably sufficient.

let mut my_closure = move |level| {
    Keybow::debounce_key(
        &one_key_state_cloned,
        level_to_bool(level)
    )
};

You could also have debounce_key take &Arc if you only sometimes need to keep the Arc alive. That also doesn't require cloning in the closure (you'd clone it in debounce_key instead).

3 Likes

The error message becomes easier to grasp if you inline the closure into the call. If it’s provided inline it’s no longer a case of

  • compiler infers FnOnce,
  • then function call requires FnMut

but instead the information that it’s an FnMut closure is present from the beginning, you get a nice error pointing into the closure body:

    one_key.set_async_interrupt(Trigger::Both, move |level| {
        Keybow::debounce_key(one_key_state_cloned, level_to_bool(level))
    }); // Registering to be called in another thread.

The error becomes

error[E0507]: cannot move out of `one_key_state_cloned`, a captured variable in an `FnMut` closure
  --> src/main.rs:23:30
   |
18 |     let one_key_state_cloned = Arc::clone(one_key_state);
   |         -------------------- captured outer variable
...
22 |     one_key.set_async_interrupt(Trigger::Both, move |level| {
   |                                                ------------ captured by this `FnMut` closure
23 |         Keybow::debounce_key(one_key_state_cloned, level_to_bool(level))
   |                              ^^^^^^^^^^^^^^^^^^^^ move occurs because `one_key_state_cloned` has type `Arc<Mutex<KeyStateData>>`, which does not implement the `Copy` trait

Admitted, the original error also pointed to the same place in the closure body, with the hint about closure is `FnOnce` because it moves the variable `one_key_state_cloned` out of its environment, but that’s less clear - requiring more background knowledge about inference of closure types to fully grasp, and quite simply it’s not displaying a (in the terminal also very much bright red and clear) error message pointing directly to one_key_state_cloned.

For closures that are used passed to one single function that expects a callback (like set_async_interrupt in this case), I would generally recommend inlining the closure expression directly into the funciton call that receives the callback (at least if there’s no other good reason against it) since it not only improves error messages, but also helps avoiding errors about ambiguous types in other cases, or errors about closures not being general enough when the arguments and/or return types involve lifetimes.

5 Likes

Yes.

In this case I defined out alone so I could easily call it directly as I was trying to debug things.

Thanks,

-kb

That seems to be the key! (I have not made these changes back to real code, so I don't actually know that this works, yet.)

Let me see if I can give a short explanation that might have been useful to the Kent of a few days ago to understand. Does this make sense:

Rust wants to do compile-time analysis of ownership, but that doesn't cover all cases. For some problems some runtime help is need in the form of an Arc<Mutex<>>, which is way of wrapping data so that it can be shared and be policed for only ever one access going on at once.

Sharing only happens if the data is passed to some other code, and the simple rule is that one always needs to share a clone() of the Arc<Mutex<>>, never the original copy.

My problem was that for the same reason that I needed a clone() to hand to my closure, it also needed a clone() to hand to the callback. Inside that callback the data protected by the Arc<Mutex<>> will actually be accessed and we need the Arc<Mutex<>> to protect that shared access, because the compiler can't figure it out. Similarly, even through the closure doesn't access the protected data, I still need an Arc<Mutex<>> because the compiler can't be sure*.

* "can't be sure" I know, a big handwave over details there.

Am I close?

Here is my code modified to no longer error, nor warn:

use std::sync::{Arc, Mutex};
use rppal::gpio::{Gpio, Level, Trigger};

pub struct Keybow {}
impl Keybow {
    fn debounce_key(_key_state: Arc<Mutex<KeyStateData>>, _down: bool) {
    }
}

pub struct KeyStateData {}

fn main() {
    let one_key_state = &Arc::new(Mutex::new(KeyStateData {}));
    let one_key_state_cloned = one_key_state.clone();
    
    let mut one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();
    
    let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
        let one_key_state_cloned_again = (&one_key_state_cloned).clone();
        Keybow::debounce_key(
            one_key_state_cloned_again,
            level_to_bool(level));
    });
}

fn level_to_bool(level: Level) -> bool {
    match level {
        rppal::gpio::Level::High => false,
        rppal::gpio::Level::Low => true,
    }
}

Thank you for the patient, prompt, and useful reply! You are helping Rust maintain its reputation for being nice and constructive people.

-kb, the Kent who needs to start making his code do real stuff now.

The OP problem is just about ownership period, not about Rust's limitations in reasoning about it. Here's something analogous to the OP that doesn't use Arc or Mutex. The problem is that the call to debounce_key gives away ownership of a captured, non-Copy value (this time, a String). It's not possible to do this more than once, so the closure can't implement FnMut (which can be called more than once).

2 Likes

This is correct.

This isn't really true. Rust doesn't differentiate between originals and clones[1]. Both of these end up with identical a and b.

let a = Arc::new(5);
let b = a.clone();
let b = Arc::new(5);
let a = b.clone();

The only thing that you've done that would make the clones look different from the original is that you are using temporary lifetime extension.

let one_key_state = &Arc::new(Mutex::new(KeyStateData {}));

Rust turns this code into something like this:

let temp = Arc::new(Mutex::new(KeyStateData {}));
let one_key_state = &temp;

You're essentially making the owned version inaccessible.

This is sometimes useful if you only ever need a reference, but since you need owned values, it doesn't make sense to do this. Method calls like .clone() automatically add or remove references when needed, so you don't even need the reference for that.

fn main() {
    // no reference
    let one_key_state = Arc::new(Mutex::new(KeyStateData {}));
    let one_key_state_cloned = one_key_state.clone();
    
    let mut one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();
    
    let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
        // no reference
        let one_key_state_cloned_again = one_key_state_cloned.clone();
        Keybow::debounce_key(
            one_key_state_cloned_again,
            level_to_bool(level));
    });
}

And again, unless you also need one_key_state outside the closure, you don't need to clone it. This doesn't work when you use temporary lifetime extension because the closure needs to move an owned value, not a reference.

fn main() {
    // no reference
    let one_key_state = Arc::new(Mutex::new(KeyStateData {}));
    
    let mut one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();
    
    let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
        // no reference
        let one_key_state_cloned = one_key_state.clone();
        Keybow::debounce_key(
            one_key_state_cloned,
            level_to_bool(level));
    });
}

After all that, the original issue with having a FnOnce instead of FnMut is pretty simple.

move |level| {
    Keybow::debounce_key(one_key_state, level_to_bool(level))
}

This closure captures one_key_state. It then sees that calling the closure will consume one_key_state, and since it only has one to give, the closure knows it can only be called once (FnOnce). The only thing about one_key_state that matters for this is that it doesn't implement Copy. It doesn't matter that it's Arc, or that it has interior mutability.

move |level| {
    let one_key_state_cloned = one_key_state.clone();
    Keybow::debounce_key(one_key_state_cloned, level_to_bool(level))
}

This closure captures one_key_state. It then sees that clone only needs a reference to one_key_state, so calling the closure doesn't consume one_key_state, and the closure can be called multiple times (FnMut).


  1. The values may be different if clone was implemented to create different values, but most types, including Arc, don't do that. The types will always be exactly the same. ↩︎

I am not understanding. Are you saying I don't need two clone calls? What does that finished code look like? (I tried following along step-by-step but coudn't make it work with one clone.)

Thanks,

-kb

This is the whole thing with one clone.

use rppal::gpio::{Gpio, Level, Trigger};
use std::sync::{Arc, Mutex};

pub struct Keybow {}
impl Keybow {
    fn debounce_key(_key_state: Arc<Mutex<KeyStateData>>, _down: bool) {}
}

pub struct KeyStateData {}

fn main() {
    let one_key_state = Arc::new(Mutex::new(KeyStateData {}));

    let mut one_key = Gpio::new().unwrap().get(22).unwrap().into_input_pullup();

    let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
        let one_key_state_cloned = one_key_state.clone();
        Keybow::debounce_key(one_key_state_cloned, level_to_bool(level));
    });
}

fn level_to_bool(level: Level) -> bool {
    match level {
        rppal::gpio::Level::High => false,
        rppal::gpio::Level::Low => true,
    }
}

Grrrr.

Yes, that works. And my real code isn't like my example code as much as I would like. My real code is trying to loop and that is confusing me.

This fragment:

…
        for (index, one_key) in keybow.gpio_keys.iter_mut().enumerate() {
            let one_key_state = &keybow.key_state_data[index];
            let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
                let one_key_state_cloned = one_key_state.clone();
                Keybow::debounce_key(one_key_state_cloned, level_to_bool(level));
            });
        };
…

Complains:

error[E0597]: `keybow.key_state_data[_]` does not live long enough
   --> src/keybow.rs:185:33
    |
83  |           let mut keybow = Keybow {
    |               ---------- binding `keybow` declared here
...
185 |               let one_key_state = &keybow.key_state_data[index];
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
186 |               let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
    |  _____________________-
187 | |                 let one_key_state_cloned = one_key_state.clone();
188 | |                 Keybow::debounce_key(one_key_state_cloned, level_to_bool(level));
189 | |             });
    | |______________- argument requires that `keybow.key_state_data[_]` is borrowed for `'static`
...
210 |       }
    |       - `keybow.key_state_data[_]` dropped here while still borrowed

error[E0505]: cannot move out of `keybow` because it is borrowed
   --> src/keybow.rs:209:9
    |
83  |           let mut keybow = Keybow {
    |               ---------- binding `keybow` declared here
...
185 |               let one_key_state = &keybow.key_state_data[index];
    |                                   ----------------------------- borrow of `keybow.key_state_data[_]` occurs here
186 |               let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
    |  _____________________-
187 | |                 let one_key_state_cloned = one_key_state.clone();
188 | |                 Keybow::debounce_key(one_key_state_cloned, level_to_bool(level));
189 | |             });
    | |______________- argument requires that `keybow.key_state_data[_]` is borrowed for `'static`
...
209 |           keybow
    |           ^^^^^^ move out of `keybow` occurs here

But this compiles:

…
        for (index, one_key) in keybow.gpio_keys.iter_mut().enumerate() {
            let one_key_state = keybow.key_state_data[index].clone();
            let _ = one_key.set_async_interrupt(Trigger::Both, move |level| {
                let one_key_state_cloned = one_key_state.clone();
                Keybow::debounce_key(one_key_state_cloned, level_to_bool(level));
            });
        };
…

Yeah, in that one the original is staying inside key_state_data, so you need to clone it. In order to not clone it, you would need to remove it from key_state_data, which is probably not what you want.

1 Like

Thanks for your help!

-kb