Provide explicit getters for inner sync objects.

[fpagliughi]

This is another usability nit.

When using the asynchronous objects, like tokio::AsyncRequest, it you put them behind a pointer like an Arc, it becomes difficult to access the underlying synchronous object through get_ref() since the pointer itself implements AsRef.

Like:

use gpiodev::tokio::AsyncRequest;
use std::sync::Arc;

let req = AsyncRequest::new(Request::builder()...);
let req = Arc::new(req);

If you do req.as_ref(), you get the reference of the pointer, and thus an &AsyncRequest, and not a &Request as you might have expected. To get to the Request you need to dereference the ponter, like:

let mut values = Values::from_offsets(&[23]);
(*req).as_ref().values(&mut values);

That works, but is a little confusing and gets more difficult if the req pointer is buried in a struct.

It might be nice to add an explicit call to get the underlying reference, perhaps like:

impl AsyncRequest {
    // ...

    pub fn request(&self) -> &Request {
        self.0.get_ref()
    }

Then you can just do:

req.request().values(&mut values);

Ditto for AsyncChip.

[fpagliughi]

BTW, if this is OK, I can put up a PR, unless there's a better/easier way to do it.

[warthog618]

Unraveling nesting can be a pain, but adding functions to skip over it is at least as confusing to me.
And I'm not keen on the stuttering.
Can't you just create a local alias and use that:

let req = Arc::new(req);
let sync_req = req.as_ref().as_ref();
_ = sync_req.values(&mut values);

That approach is clearer to me, as it highlights which request calls are synchronous.
If there is a helper to get to the inner synchronous request can we find a better name it to indicate that?

[fpagliughi]

Yeah, that would work, but it doesn’t seem like a clean/easy API.

I’m agnostic to the naming for a get function; maybe sync() or sync_request() would be better to remind you you’re getting the synchronous version?

My assumption in this is that since the async wrapper only exposes a few of the event reader functions it will likely be common for users to access the inner object to get to the rest of the functionality of them.

[fpagliughi]

I suppose the other option is to fill out the API of the asynchronous objects with the synchronous calls as well. Or at least the more common ones. In that way, users needing to go to the inner objects would be less common.

Like

impl AsyncRequest {
    pub fn value(&self, offset: Offset) -> Result<Value> {
        self.0.value(offset)
    }

    // ...
}

I effectively did something like this in socketcan-rs by making a trait for synchronous functions built on top of AsRaw with the trait functions implemented as defaults:

pub trait SocketOptions: AsRawFd {
    fn some_sync_function(&self) -> Result<()> {
        some_sync_call(self.as_raw_fd(), xxx, yyy)
    }

    // ...
}

Then the individual structs, both synchronous and async just implement the empty trait and get all the functions.

impl SocketOptions for CanSocket {}
impl SocketOptions for CanFdSocket {}

impl SocketOptions for AsyncCanSocket {}
impl SocketOptions for AsyncCanFdSocket {}

So these didn't implement the read/write functions which have different signatures in the different versions... just the common calls that are always synchronous (in this case wrapping the iotl calls to get/set socket options).

I actually would have never thought of this, but someone sent in a PR with that done for the two different synchronous socket types, and I just extended it for the async ones as well.

[warthog618]

Yeah, that would work, but it doesn’t seem like a clean/easy API.

Seems idiomatic to me.

I’m agnostic to the naming for a get function; maybe sync() or sync_request() would be better to remind you you’re getting the synchronous version?

Don't like sync() on it's own, as that can mean other things.
The more I think about it, the less I like xxx_request(), as that implies that an action is being performed, and it most assuredly is not.

My assumption in this is that since the async wrapper only exposes a few of the event reader functions it will likely be common for users to access the inner object to get to the rest of the functionality of them.

For sure - the bulk of the API is synchronous. And I prefer keeping it clear that it is. Hiding things is not always a good thing.

I suppose the other option is to fill out the API of the asynchronous objects with the synchronous calls as well. Or at least the more common ones. In that way, users needing to go to the inner objects would be less common.

Did that for gpiosim, with the Chip and Simpleton sharing an API - since they both basically drive a single chip.
Didn't do that with traits though - just manual wrappers.
Same with the Builder and Config here, come to think of it.
Might try the trait trick for those - would prevent me forgetting to add the wrapper when adding a new function to the core.

But, back to the point, I don't like doing that here, as it implies that the functions added to the AsyncRequest or AsyncChip are compatible with being run within a reactor thread, when that is not necessarily the case.

So I prefer keeping the sync and async APIs distinct. Then if we do ever want to add async versions we can.
Not that that is likely at the moment, cos that is a can of worms, but it keeps the option open.

TL;DR
Not keen on extending the async API.
Still considering an accessor for the inner synchronous request, but need a good name, and inner_synchronous_request() is sufficiently long as to defeat the purpose and I dislike ending in "request". inner_synchronous()?

[fpagliughi]

Yeah, I agree, just providing the accessor seems the best way to go. (Ditto for AsyncChip to be consistent).

And I agree that since the word "request" is a verb and a noun, it adds some potential for confusion. But I stick with my original suggestion. Really, who can complain about a function named request() that returns an object named Request? :-)

But whatever you decide is fine.

[warthog618]

Really, who can complain about a function named request() that returns an object named Request? :-)

That would be me :-D. I'm just as happy with .as_ref().as_ref() in this case.
Rather than adding something we may regret later, I think we should just sit at the moment.
We can always revisit it when we get more data as to how big a pain point it is.