-
Notifications
You must be signed in to change notification settings - Fork 9
Runtime independent design for Rust APIs #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main_api_rs
Are you sure you want to change the base?
Runtime independent design for Rust APIs #17
Conversation
bharatGoswami8
commented
Nov 17, 2025
- Primary APIs moved to Runtime traits
- Add generic compatibilty for Vehicle Consumer and Producer
- Added Provider and Consumer InstanceInfo
* Primary APIs moved to Runtime traits * Add generic compatibilty for Vehicle Consumer and Producer * Added Provider and Consumer InstanceInfo
|
|
||
| fn main() { | ||
| let runtime_builder = RuntimeBuilderImpl::new(); | ||
| let runtime = Builder::<MockRuntimeImpl>::build(runtime_builder).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected the non mocked runtime is used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes , both can be used.
Added in next commit.
| /// Reading from the received pointer before initialization is undefined behavior. | ||
| fn as_mut_ptr(&mut self) -> *mut T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be unsafe used if undefined behavior can happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't, as the method itself will not introduce UB - using the pointer could, though. However, any operation that needs dereferencing is anyway unsafe on the pointer itself, so that's already how it should be.
com-api/com-api-concept/src/lib.rs
Outdated
| { | ||
| } | ||
|
|
||
| pub trait Subscriber<T: Reloc + Send, R: Runtime + ?Sized,> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is T the payload of the event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised that generated code for both the mock and the lola binding exist. At the moment it looks even identical.
I was expecting that if we use generics the same generated code can be used for both bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. @bharatGoswami8 can we remove both lola and mock generated crates and create top level crate com-api-gen-types ? If you need smoene doing this, let me know and we can add commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now we have only one gen file with generic R
pawelrutkaq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are heading to implement iceoryx2 and see if we have any issues with other backends.
| type Publisher<T: Reloc + Send>: Publisher<T>; | ||
| type ProviderInfo: Send + Clone; | ||
| type ConsumerInfo: Send + Clone; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General note. Can we suffix here the types with Type suffix or any other name that is different than the bounded Trait So we don't have type Publisher: Publisher. This is really confusing later in the code and easy to miss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now we don't have any guideline for naming in s-core coding guideline .... https://eclipse-score.github.io/score/main/contribute/development/rust/coding_guidelines.html
we may need to get the naming guideline from s-core rust community.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we at least don't name different things with exactly same name?
As Paweł pointed we have type Publisher with assigned struct Publisher that implements trait Publisher.
I don't think we need explicit rule for that.
| &self, | ||
| _instance_specifier: InstanceSpecifier, | ||
| ) -> Self::ServiceDiscovery<I>; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess async api we can add in next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in latest commit.
com-api/com-api-concept/src/lib.rs
Outdated
| fn check_str(_service_name: &str) -> bool { | ||
| // For now, accept any non-empty string as a valid service name | ||
| // In a real implementation, this might validate the format | ||
| true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that we will have to specify the str layout in S-CORE as it has to be runtime agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we can call the backend compatible APIs or we can have validation in com-api.
I think we should use backend API so that if communication medium or channel change we no need to modify validation logic in com-api side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a point ofc. But this would mean that if you change a backend (runtime) then You must change InstanceSpecifier across code base. This would contradict the whole idea of current work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstanceSpecifier is definitely meant to be backend agnostic. All backend specific stuff has to be buried inside ProducerInfo or ConsumerInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I guess here we will just do like allowed letters , numbers and / ie and thats it.
com-api/com-api-concept/src/lib.rs
Outdated
| /// The string shall describe where to find a certain instance of a service. Each level shall look | ||
| /// like this | ||
| /// <InterfaceName>:my/path/to/service_name | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing Debug, PartialEq, PartialOrd, Ord,Eq
|
|
||
| unsafe impl Reloc for () {} | ||
| unsafe impl Reloc for u32 {} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shall add reloc for all u*, all i*, float*, bool,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing:
usize
u128
i128
isize
f64
all atomics ?
[T] where T: Reloc
[T; N] where T: Reloc
core::mem::MaybeUninit where T: Reloc
tuples from T1, to T5 ie (so variants from 1 elem to 5 elems) where TX: Reloc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move reloc trait with this to separate file reloc.rs since we will also need to add the Reloc derive proc macro and reexport it from there.
| sample.send().unwrap(); | ||
| } | ||
|
|
||
| fn main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shall have the main running the same code for MockRuntime and LolaRuntime. So we can see all that works together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in latest commit.
|
|
||
| use super::*; | ||
| #[test] | ||
| fn create_producer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add an example with composition.
pub struct SomeClass {
// Keeps VehicleConsumer, or its part
}
So we can write UT that shows the SomeClass works with SomeRuntime and MockRuntime and is testable due to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added VehicleMonitor , just added for Consumer.
Will extend this with producer also.
| use std::fmt::Debug; | ||
| use std::future::Future; | ||
| use std::ops::{Deref, DerefMut}; | ||
| use std::path::Path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change whats possible to core instead std.
[workspace.lints.clippy]
std_instead_of_core = "warn"
alloc_instead_of_core = "warn"
and enable warnings as errors or use err here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in latest commit.
| type Subscriber<T: Reloc + Send>: Subscriber<T, Self>; | ||
| type ProducerBuilder<I: Interface, P: Producer<Self, Interface = I>>: ProducerBuilder<I, P, Self>; | ||
| type Publisher<T: Reloc + Send>: Publisher<T>; | ||
| type ProviderInfo: Send + Clone; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes ProviderInfo added as instance info of Producer , I think this will be use with offer service method but need to check feasibility.
This is added just to notify we have two different type for Producer and Consumer.
com-api/com-api-concept/src/lib.rs
Outdated
| /// | ||
| /// TODO: Shall we also require DerefMut<Target=MaybeUninit<T>> from implementing types? How to deal | ||
| /// TODO: with the ambiguous assume_init() then? | ||
| pub trait SampleMaybeUninit<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make all samples implement Debug by requiring it for T ? later is's hard to debug in code without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in latest commit.
|
Working on the review comments... |
* Added Async API for get_instance * added Debug trait for All the Sample trait * Added Core instead of std all the possible palce * Added one Gen file and remove runtime specific gen file * Added both Mock and Lola time in same main file * Added VehicleMonitor as Consumer on example * Removed all the feature flag * added impl for Reloc * Removed root toml file
piotrchodorowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented Iceoryx integration on top of those changes and added comments with all my findings.
I didn't add any comments in runtime part as those changes should results from proposed changes in concept part.
| #[derive(Clone, Debug)] | ||
| pub struct InstanceSpecifier { | ||
| pub specifier: String, | ||
| specifier: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removed pub? I think we need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? If you want to access the string, you can just as well use AsRef<str>. We can also add Into<Option<String>> if that's what would fit your purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But without pub I cant access this field within runtime e.g:
|
280 | let specifier = instance_info.instance_specifier.specifier.clone().unwrap();
| ^^^^^^^^^ private field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can access using instanceSpecifier instance rather then making specifier field as public.
We have new() which is returning InstanceSpecifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, it is clear to me now how to use it
| type Interface: Interface; | ||
| type OfferedProducer: OfferedProducer<Interface = Self::Interface>; | ||
| type OfferedProducer: OfferedProducer<R, Interface = Self::Interface>; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need new here
| fn new(instance_info: R::ProviderInfo) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chain here should be Runtime::producer_builder().build(). Would that fit as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was needed to be able to crate Producer with instance_info from ProducerBuilder.
| type SampleMaybeUninit<'a>: SampleMaybeUninit<T> + 'a | ||
| where | ||
| Self: 'a; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need new here
| fn new(identifier: &str, instance_info: R::ProviderInfo) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chain here should be Runtime::producer_builder().build().offer().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is needed to be able to crate Publisher with identifier and instance info passed.
In current implementation there is no way to provide service name for Publisher.
|
|
||
| pub struct VehicleProducer<R: Runtime + ?Sized> | ||
| { | ||
| _runtime: core::marker::PhantomData<R>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _runtime: core::marker::PhantomData<R>, | |
| _runtime: core::marker::PhantomData<R>, | |
| instance_info: R::ProviderInfo, |
| impl<R: Runtime + ?Sized> Producer<R> for VehicleProducer<R> { | ||
| type Interface = VehicleInterface; | ||
| type OfferedProducer = VehicleOfferedProducer<R>; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fn new(instance_info: R::ProviderInfo) -> Self { | |
| VehicleProducer { | |
| _runtime: std::marker::PhantomData, | |
| instance_info, | |
| } | |
| } |
| For mock test: | ||
| ``` | ||
| inc_mw_com/com-api$ cargo run --example basic-consumer-producer --features "lola" | ||
| inc_mw_com$ cargo test --test basic-consumer-producer-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called within com-api folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update on latest commit.
| ``` | ||
|
|
||
| For LoLa build: | ||
| For mock test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for all interfaces now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update on latest commit.
| inc_mw_com$ cargo test --test basic-consumer-producer-test | ||
| ``` | ||
|
|
||
| For mock Build and Run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for all interfaces now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update on latest commit.
| pub fn monitor_tire_data(self) -> Result<String> { | ||
| let subscribed = self.consumer.left_tire.subscribe(3)?; | ||
| let mut sample_buf = SampleContainer::new(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removed logic with sending and receiving n samples?
| sample.send().unwrap(); | ||
| } | ||
|
|
||
| fn run_with_runtime<R: Runtime>(name: &str, runtime: &R) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend to change this example logic to something like:
let offered_producer = create_offered_producer(&runtime);
let consumer = create_consumer(&runtime);
let monitor = VehicleMonitor::new(offered_producer, consumer);
for _ in 0..SAMPLES_NUM {
monitor.produce_tire_data().unwrap();
monitor.consume_tire_data().unwrap();
}
|
@darkwisebear I made a check on trait issues we discussed and doing |
|
@bharatGoswami8 One small note: after some additional investigation I see that it would be good (or even it could be necessary) to make all new() calls returning Result instead of just Self. Including of course the two new()'s proposed by myself before. |
As discussed in this comment thread #17 (comment) , we will update new method with returning Result. |
|
Working on pending review comments fix, I will share latest commit next week start... |