fpagliughi / phidget-rs

Rust library for phidget sensors
MIT License
1 stars 4 forks source link

add example Sync trait problem in Callbacks #10

Open aurorameccanicadev opened 2 months ago

aurorameccanicadev commented 2 months ago

(Sorry for empty issue at first, pressed enter by mistake, I'm now editing the issue to add what I wanted to write)

Obviously if I manage to solve it, I can try to make a pull request with some documentation for the library, or tell me if I can help in some other way :)

I can't figure out how to change the state of a digital output inside a digitalInput callback. I made a stripped down example. I'm trying to make a LED turn on and off when I press a button.

I attach the stripped down test project phidget_test_rust.tar.gz

let mut button = DigitalInput::new();
let mut led = DigitalOutput::new();
// Make the button alternate the LED state
button.set_on_state_change_handler(|_, s: i32|
    {
    led.set_state(
        !led.state().unwrap()
    );
}).unwrap();

However, the compiler complains about

error[E0277]: `*mut c_void` cannot be shared between threads safely
   --> src/main.rs:15:40
    |
15  |       button.set_on_state_change_handler(|_, s: i32|
    |  ____________---------------------------_^
    | |            |
    | |            required by a bound introduced by this call
16  | |         {
17  | |         led.set_state(
18  | |             !led.state().unwrap()
19  | |         );
20  | |     }).unwrap();
    | |_____^ `*mut c_void` cannot be shared between threads safely

I tried to wrap it into an Arc and tried also to to Box::leak() to pass a static reference to it, without success. I tried adding move to see if I could give the ownership of the value to the callback, but that didn't work either.

fpagliughi commented 2 months ago

Yeah, the "handle" that the Phidget C library gives us is just a pointer. It probably points to a C struct in their library with whatever information that it tracks and maintains.

By default, Rust makes all raw pointers !Send and !Sync by default, meaning you can't share them between threads, or even send them from one thread to another.

My thinking is that it should be OK to send a handle from one thread to another, so I made the Rust structs containing a handle Send.

But I have no idea what the library is doing when it updates its internal structs. So for thread safety we really need to keep the handles !Sync. So that's the error you're getting:

^ `*mut c_void` cannot be shared between threads safely

So then our objects/devices can not be shared across threads... at least not without some locking.

You're led variable is visible to multiple threads. The state change handler is possibly being called by a thread in the library. And even if its in the same thread as main(), there's no way for the compiler to know that.

So anyway, you need to lock the led one way or another. The safest thing, which would work for either case is to wrap it in a Mutex.

use std::sync::Mutex;

let led = Mutex::new(DigitalOutput::new());

button.set_on_state_change_handler(|_, s: i32| {
    let led = led.lock().unwrap();
    let val = led.state().unwrap();
    led.set_state(!val);
}).unwrap();

Something like that.

aurorameccanicadev commented 2 months ago

Thanks, I made some small changes and it seems to work. I've just tested on real hardware. I added the move so it moves it there instead of moving the reference only.

I'll write an example and send a pull request, thank you!

// Open a LED connected to a digital output
    let led = Mutex::new(DigitalOutput::new());
    {
        let mut led = led.lock().unwrap();
        led.set_channel(1).unwrap();
        led.open_wait_default().unwrap();
    }
// Make the button alternate the LED state
    button.set_on_state_change_handler(move |_, s: i32|
        {
            if s==0
            {
                return;
            }
            let led = led.lock().unwrap();
            led.set_state(
                !led.state().unwrap()
            ).unwrap();
        }
    ).unwrap();