feat(dvc): expose dynamic channel accessors

[uchouT (uchouT)]

Add mutable dynamic channel lookup APIs for drdynvc client / server, so that we can use the public API with signature &mut self in dvc processor, such as UrbdrcDeviceClient and UrbdrcControlClient in #1365

[Marc-Andre Lureau (elmarco)]

lgtm Benoît Cortier (@CBenoit)

[Benoît Cortier (CBenoit)]

Thank you!

This API feels a bit implementation-shaped to me. The use case we want to support is "given a DVC channel id, recover my typed processor so I can call its domain-specific API mutably." But exposing DynamicChannel directly seems to expose the current storage and lifecycle wrapper as part of the public contract.

Just thinking out loud, but maybe we could make the public API narrower and more intention-shaped instead, e.g. typed processor accessors on DrdynvcClient / DrdynvcServer, while keeping the channel wrapper types private?

Also, the new DvcClientProcessor bound seems inconsistent with registration: client APIs still accept T: DvcProcessor and listeners return Box<dyn DvcProcessor>, but downcasting now requires T: DvcClientProcessor. Maybe registration/listener APIs should also use DvcClientProcessor, or the downcast bound should remain DvcProcessor?

[uchouT (uchouT)]

Thank you!

This API feels a bit implementation-shaped to me. The use case we want to support is "given a DVC channel id, recover my typed processor so I can call its domain-specific API mutably." But exposing DynamicChannel directly seems to expose the current storage and lifecycle wrapper as part of the public contract.

Thanks, that makes sense to me. I exposed DynamicChannel directly on the server mostly to mirror the existing client-side DynamicVirtualChannel API, but I agree that we don't need to make the server wrapper public.

Just thinking out loud, but maybe we could make the public API narrower and more intention-shaped instead, e.g. typed processor accessors on DrdynvcClient / DrdynvcServer, while keeping the channel wrapper types private?

Agreed, so the API looks like get_dvc_by_channel_id_mut::<T>(&mut self, channel_id: u32) -> Option<&mut T>, which will internally handle downcasting without exposing channel wrapper. The client-side DynamicVirtualChannel is no need to be public as well, but I hesitate to break it in this PR, let me know your idea!

Also, the new DvcClientProcessor bound seems inconsistent with registration: client APIs still accept T: DvcProcessor and listeners return Box<dyn DvcProcessor>, but downcasting now requires T: DvcClientProcessor. Maybe registration/listener APIs should also use DvcClientProcessor, or the downcast bound should remain DvcProcessor?

Thanks for pointing out, will be resolved in this PR.
[EDIT]: I'll modify DynamicVirtualChannel's visibility and change listener's return trait bound in a non-blocking PR.

[uchouT (uchouT)]

Benoît Cortier (@CBenoit) I reserve my perspective after I tried to modify the client DynamicVirtualChannel's visibility.

I think there is a valid public use case for a channel handle: callers sometimes need the channel id and the typed processor together. Splitting that into separate APIs such as get_channel_id<T>() and get_processor<T>() would require two lookups and would not express that the id and processor belong to the same active channel.

That said, I agree with the concern that exposing the current channel wrapper exposes implementation shape:

But exposing DynamicChannel directly seems to expose the current storage and lifecycle wrapper as part of the public contract.

The storage itself is not exposed while the fields stay private, but the current wrapper still exposes lifecycle/storage shape through the public type and the downcast-based accessor methods. Also, mutable processor access is the reason this PR exists, so some controlled mutable access to the underlying processor is unavoidable. I mentioned the original need here: #1137 (comment)

Maybe the better API is to keep DynamicVirtualChannel / DynamicChannel private as the internal containers, and expose typed channel handles instead:

pub struct DvcAccessor<'a, T: DvcProcessor> {
    channel_id: u32,
    processor: &'a T,
}

pub struct DvcAccessorMut<'a, T: DvcProcessor> {
    channel_id: u32,
    processor: &'a mut T,
}

Then DrdynvcClient / DrdynvcServer can return these handles from typed accessors. This keeps the single-lookup channel_id + processor use case, while avoiding making the internal channel wrappers part of the public API.