Make async RegisterOperation functions take self by value

[peterkrull]

Hey,

I have started playing around with this library, and inspired by your blog post I wanted to make my own driver do most async operations using the pass-through pattern you suggest. However that is currently not easy to do with this library, since RegisterOperation::{read, write, modify, etc..}_async take &mut self rather than just self. So the returned future would borrow the RegisterOperation that is constructed locally in this function.

The result is that I am unable to do this:

pub fn read_acc_raw(&mut self) -> impl Future<Output = Result<[i16; 3], I::Error>> {
    self.inner
        .acc_data()
        .read_async()
        .map_ok(|data| [data.acc_x(), data.acc_y(), data.acc_z()])
}

but rather have to resort to async+awaiting:

pub fn read_acc_raw(&mut self) -> impl Future<Output = Result<[i16; 3], I::Error>> {
    async {
        self.inner
            .acc_data()
            .read_async()
            .map_ok(|data| [data.acc_x(), data.acc_y(), data.acc_z()])
            .await
    }
}

// Or just: pub async fn read_acc_raw(&mut self) -> Result<[i16; 3], I::Error>

This PR just change those x_async functions to take self rather than &mut self and all tests still seem to pass, and the first snippet above compiles. Of course this means one cannot construct a RegisterOperation once and re-use it multiple times, but I am also not sure if there is any benefit to that over just constructing it again. I have not touched the sync functions here, but perhaps they should also take self just for symmetry?

[diondokter]

Hey there!

Thanks for the contribution. I'm in two minds about it.

If we do this somewhere, we should do it everywhere. Principle of least surprise.

So what are the ramifications?

• This needs to happen to both blocking and async ops
• This needs to happen for commands and buffers too (and potential future operations)
  • Though it's already inconsistent since the bulk ops apparently already use self by move
• It means you can use an operation just once
  • But creating an operation is pretty much free. Worst case is adding some numbers together for the addresses.
  • But buffers use the embedded-io interface, and so you want to be able to call them multiple times
    • But the driver where I used buffers recreates the operation anyways and so wouldn't be affected

So... I don't really know what's best. W.r.t. the size optimization, I hope (and am trying) to make the compiler just do the right thing so people don't have to do that optimization by hand anymore. So in a while the optimization shouldn't be necessary anymore.

I'll take some time to think about it

[peterkrull]

I also think it should be consistent between sync and async, I just wanted to hear you out first.

The question is whether there are any legitimate patterns which are not possible when the RegisterOperation is taken by value, that are not solved by just cheaply reconstructing the operation.

You know best what the potential timelines are for you landing device-driver 2.0, and your work with the compiler (which I am also looking forward to!) So whether these changes make sense in terms of the compiler making it redundant in the near term is your call.

[Wassasin]

The only regression in the generated code will be that the address computation will be performed multiple times, whereas previously users had the opportunity to store this value. (which 99% of the users will not do) I think this benefit is inconsequential, especially in the light that a HAL will perform the same kind of address computation possibly dozens of times when interacting with the interface for the same device-driver register operation.

Code-size-wise this change does not change anything, and opens the possibility of async future erasure as demonstrated.

I say this is a meaningful improvement overall.

[diondokter]

Ok, then I'm inclined to merge, but I do think it should take ownership across the board then. @peterkrull can you do that? If not I'll pick it up when I get to it

[peterkrull]

Sync RegisterOperation functions now also take self by value. I have left buffer operations as-is, since their byte-stream semantics are different enough to warrant &mut self and their documentation comments also say why, as they mirror embedded_io.