Rust::com Field APIs design and example of usage - #700
Conversation
70fbb6e to
f7cd859
Compare
6faaa6c to
21d958d
Compare
30e0af1 to
8589c65
Compare
darkwisebear
left a comment
There was a problem hiding this comment.
I mainly reviewed the concept part; I think we need to nail that part before it makes sense to look at the rest.
| // Must register handlers and initialize all fields before offer() is available | ||
| let offered = producer | ||
| .init_field() | ||
| .register_set_handler_left_tire(move |val: &Tire| { |
There was a problem hiding this comment.
I wonder whether &Tire is correct. Why not hand over ownership to the callable? This way, the implementer wouldn't have to clone the value in the likely case the value is written somewhere.
There was a problem hiding this comment.
Updated callable with Fn(T)
| println!("Received exhaust update"); | ||
| }) | ||
| .expect("Failed to register set handlers") | ||
| .update_left_tire(&initial_tire_value) |
There was a problem hiding this comment.
Why by-reference? This forces cloning as the implementation most likely has to clone the value to send it across the network. It might be a corner case, though.
There was a problem hiding this comment.
Agreed, i was trying to match with C++ semantic but this will add additional clone, will go with same approach of send for Event.
| use std::fmt::Debug; | ||
| use std::future::Future; | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Why is this dead code?
There was a problem hiding this comment.
Updated example app.
| #[allow(dead_code)] | ||
| // Temp for build test | ||
| // We will remove this once memory layout of same created in rust side like SamplePtr. | ||
| #[repr(C)] |
There was a problem hiding this comment.
Doesn't this trigger an "improper_ctypes" lint? Result isn't #[repr(C)], so it cannot be used here properly.
There was a problem hiding this comment.
This is temporary placeholder of MethodReturnTypePtr of C++ like SamplePtr for Event.
There was a problem hiding this comment.
Now we are using method interface to create Field's method (set and get), so this is removed.
| // or by default we will enable for user, | ||
| // -> we can keep it default enable as of now, | ||
| // and later we can add tag based mechanism if required because Interface side we need to check how we can do this | ||
| // 2. We are offering get method after subscrption async but before subscription it is sync, |
There was a problem hiding this comment.
Just turn the one before subscription into async. Or is there anything that prevents you from doing so?
There was a problem hiding this comment.
Now we are using method interface to create Field's method (set and get), so this is removed but
We have another issue consumer partially moved when we call subscribe on field because it take self by value.
| // we will create a module which will have common trait for event and field which will be used by both event and field as a super trait and | ||
| // for this we need to create marker trait for event. | ||
|
|
||
| use crate::*; |
There was a problem hiding this comment.
Please import specific types. Yes, it's tedious, but being explicit here also saves some headaches.
| /// # Returns | ||
| /// Return the result of `Result<()>` which contains the status of the register operation. | ||
| // TODO: Do we need to make callback lifetime 'static or we keep same as field publisher lifetime. | ||
| fn register_set_handler<'a>(&self, callback: impl Fn(&T) + Send + 'a) -> Result<()>; |
There was a problem hiding this comment.
There is no equivalent method for registering the get handler. It may sound weird, but the C++ counterpart has that, since mw::com treats set and get as ordinary methods.
| /// # Returns | ||
| /// Return the result of `Result<()>` which contains the status of the register operation. | ||
| // TODO: Do we need to make callback lifetime 'static or we keep same as field publisher lifetime. | ||
| fn register_set_handler<'a>(&self, callback: impl Fn(&T) + Send + 'a) -> Result<()>; |
There was a problem hiding this comment.
TO me this feels as if we're treating ordinary methods and field methods in a distinct way. Maybe we should think about getting methods ready first and then incorporate them as the foundation of set and get here.
There was a problem hiding this comment.
Method Design is ready - #777
Working on Combined Field and Method changes.
| @@ -329,18 +342,10 @@ where | |||
| /// | |||
| /// # Type Parameters | |||
| /// * `T` - The relocatable event data type | |||
| pub trait SampleMut<T>: DerefMut<Target = T> + Debug | |||
| pub trait EventSampleMut<T>: SampleMut<T> | |||
There was a problem hiding this comment.
Is this just because of update vs send (aka naming)?
There was a problem hiding this comment.
Yes, because of that SampleMut is just marker trait and then we have feature specific trait which provide the API like send and update.
* Created field interface APIs * Updated SampleMut to use in field as well
* Lola Runtime placeholder implementaion for field producer and consumer * Mock Runtime placeholder implementation
* Create proc macro for field init and set handler validation before offer call
* Updated example file with Field APIs usage
8589c65 to
3d76ef9
Compare
* Method and field both code generation added on macros
|
@darkwisebear , I will create a separate PR based on the Method Design PR, and all review feedback will be addressed in that PR. |
|
Hi @darkwisebear , @rpreddyhv , I have created PR #818 for field design based on Method design interface. I already addressed most of the review comment on this PR. Request for review mentioned PR. Closing this PR. |
edit -
Since the Field and Method changes are closely related, and the Field implementation also uses the Method interface while both share the same macro for the interface and type-state pattern, I believe it would be better to review the overall design in a single PR. This will help ensure that any suggestions or feedback can be addressed more effectively and consistently across both implementations.
Therefore, I am closing this PR. Please review the PR - #818
#579