juliangaal / mpu6050

MPU6050 embedded-hal driver written in Rust
https://crates.io/crates/mpu6050
MIT License
52 stars 31 forks source link

Use device without delay #7

Closed jounathaen closed 3 years ago

jounathaen commented 4 years ago

Hi,

this crate has great features and runs nicely on my embedded controller (stm32f103).

However, I cannot use it, because it borrows a delay object. However, on my embedded controller I only have one delay object and using it with the mpu6050 struct prevents me from using simple delay_ms, even though I only want to use e.g. get_acc_avg.

It would be great if the functions that don't require a timer can be executed without a delay object.

(But maybe it's only me not knowing the trick to share a delay...)

Greetings!

juliangaal commented 4 years ago

this crate has great features and runs nicely on my embedded controller (stm32f103) great to hear!

my mpu6050 was faulty, so at the moment I cannot test it or change it :/ Maybe i'll get a hold on one during the christmas time.

I honestly have never thought about your issue. I only use the delay object once internally during startup (wait 100ms, according to data sheet). In the example I provide, I should probably use it during the loop to limit the rate, as well.

Could you educate me further?

jounathaen commented 4 years ago

Hi and thanks for your reply. As already mentioned, I do not have very good insights into the embedded hal either, but I'll try

why do you need a separate delay object and can't reuse your existing one? I haven't read too deep into the embeddel_hal docs regarding this

Let's assume I have the following code:

// ...
// Create delay object. This can only be done once due to hardware restrictions
let mut delay = Delay::new(cp.SYST, clocks);
// use delay
delay.delay_ms(100 as u32);
// initialize and use mpu
let mut mpu = Mpu6050::new(i2c_bus, delay);
let acc = mpu.get_acc().unwrap();

// later on use the delay again. For whatever reason
delay.delay_ms(100 as u32);

This does not compile, because the mpu object now "owns" the delay object:

error[E0382]: borrow of moved value: `delay`
   --> src/main.rs:186:5
    |
101 |     let mut delay = Delay::new(cp.SYST, clocks);
    |         --------- move occurs because `delay` has type `stm32f1xx_hal::delay::Delay`, which does not implement the `Copy` trait
...
173 |     let mut mpu = Mpu6050::new(i2c_bus, delay);
    |                                         ----- value moved here
...
186 |     delay.delay_ms(100 as u32);
    |     ^^^^^ value borrowed here after move

On a microcontroller, I only have one delay object. This is probably due to the inherent seqential nature of microcontroller code without an OS. So I cannot use this crate in my project, because I also need access to the delay. AFAIK, there is no module, that provides "sub-delays" (yet). Maybe something like RTFM does this.

I could imagine, a solution would be to pass the delay as a function parameter to the init function and return it after the initialization, so that ownership can be transferred back to the caller.

BTW: you wait twice in a row in your wake function (probably by accident): Here: https://github.com/juliangaal/mpu6050/blob/39f441d4dec903919f4dea026f54d38514ebe9ae/src/lib.rs#L353 and here: https://github.com/juliangaal/mpu6050/blob/39f441d4dec903919f4dea026f54d38514ebe9ae/src/lib.rs#L553

Best Regards!

juliangaal commented 4 years ago

This does not compile, because the mpu object now "owns" the delay object

The borrow checker :D I understand. I looked at this crate for bigger brother of the chip and you can see that they only use a mutable reference &mut delay in the self::init_mpu(...) method.


https://github.com/juliangaal/mpu6050/blob/39f441d4dec903919f4dea026f54d38514ebe9ae/src/lib.rs#L553

Actually, this wasn't a mistake. It was recommended to put in a delay after writing in the datasheet. But I guess it has to work without it as well, and isn't entirely my responsibility :D


As I said I don't have access to the chip right now. Would you be willing to send a PR? https://github.com/juliangaal/mpu6050/blob/39f441d4dec903919f4dea026f54d38514ebe9ae/src/lib.rs#L327

would have to be changed to pub fn new(i2c: I, &mut delay: D) but can't be sideffect free anymore, because https://github.com/juliangaal/mpu6050/blob/39f441d4dec903919f4dea026f54d38514ebe9ae/src/lib.rs#L358 must be called and pass the &mut delay to https://github.com/juliangaal/mpu6050/blob/39f441d4dec903919f4dea026f54d38514ebe9ae/src/lib.rs#L351

juliangaal commented 4 years ago

any update, thoughts? ;)

jounathaen commented 4 years ago

I hope I find some time this weekend. Then I'll try to create a PR

jounathaen commented 4 years ago

Related: https://github.com/stm32-rs/stm32f1xx-hal/issues/131

jounathaen commented 4 years ago

I started to implement this feature, but somehow I have problems on the embedded board. One reason might be the absence of a fpu on the uC... But I don't know when I can find enough spare time soon to finish it.

juliangaal commented 4 years ago

I'll order a new unit before christmas ;) I'll be able to test it as well

juliangaal commented 4 years ago

I couldn't test the updated version on branch non-owning-delay bcs of family and other things. Could you checkout this branch? You can add the branch to your Cargo.toml, for example

[dependencies]
rand = { git = "https://github.com/rust-lang-nursery/rand", branch = "next" }

Have a look at the updated example for the API change. I think I'll get rid of the side effect new(...) constructor in the future, but for now, would you mind testing like shown in the example?

Nivek92 commented 4 years ago

Hey, I can confirm that the code in the non-owning-delay branch works.

juliangaal commented 4 years ago

perfect, thanks. I'll merge into master then