iox-23 refine and update publisher and subscriber api #24

elBoberido · 2022-06-21T21:11:17Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR refines and updates the API to match the iceoryx v2 functionality.

The PR should be merged after #22

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes Update and refine publisher and subscriber API #23

examples/publisher_simple.rs

elfenpiff · 2022-06-23T14:15:11Z

examples/subscriber_multithreaded.rs

+    let (subscriber, sample_receive_token) =
+        SubscriberBuilder::<Counter>::new("Radar", "FrontLeft", "Counter")
+            .queue_capacity(5)
+            .create_mt()?;


What does the suffix _mt mean and why does the publisher does not have such a suffix? When it has something todo with multithreaded wouldn't be create_threadsafe somehow more fitting?

Or does the subscriber come with multiple threads?

_mt means that a subscribed Subscriber will be created which can receive samples in a different thread than the one which created the subscriber. Internally the pointer to the C++ subscriber is stored in a smart pointer similar to shared_ptr. In Rust there are two types Rc and Arc. The latter has an atomic ref counting and the former not. This means the latter can be shared across threads and the latter not. Since one only pays for what one uses, there are two differrnt types for a subscriber, st::Subscriber and mt::Subscriber.

elfenpiff · 2022-06-23T14:16:58Z

examples/subscriber_simple.rs

 use std::thread;
 use std::time::Duration;

 #[repr(C)]
-struct CounterTopic {
+struct Counter {


Would it be possible to outsource this into a separate file so that every publisher and subscriber share the same topic definition.
Otherwise it can become easily inconsistent.

Will do this in a follow up PR. I need to refactor the example a bit in order to share files between the examples. While it is dead easy to add simple examples with cargo, by default it expects one file per example. It is possible to add more complex example but it is ... well ... also a bit more complex

src/error.rs

elfenpiff · 2022-06-23T14:22:05Z

src/introspection/memory/ffi.rs

            .queue_capacity(1)
            .history_request(1)
-            .build()
+            .create_without_subscribe()


Just out of curiosity, we have create, create_mt (for multithreaded), create_without_subscriber, create_without_subscribe_mt?

The point I wanna make is, I would put everything into the builder and just have a single create so it is nice and consistent and I remember a special someone nagging me about consistency with m_ so I want to return the favor 😄

So the handling I imagine is something like this:

pub fn new() -> InactiveSubscriber<Self> { SubscriberBuilder::<Self>::new("Introspection", "RouDi_ID", "MemPool") .queue_capacity(1) .history_request(1) .threadsafe(false) .subscribe_on_create(false) .build() .create_without_subscribe()

But then you don't need a builder or you need a second builder for InactiveSubscribersince new does not have any parameter. I explained the rationale further down.

elfenpiff · 2022-06-23T14:28:45Z

src/pb/publisher.rs

-        self.publisher.ffi_pub.stop_offer();
-        self.publisher
+    pub fn stop(self) -> InactivePublisher<T> {
+        self.ffi_pub.stop_offer();


what does ffi mean?

Foreign Function Interface

elfenpiff · 2022-06-23T14:30:16Z

src/pb/publisher.rs

-        self.publisher
+    pub fn stop(self) -> InactivePublisher<T> {
+        self.ffi_pub.stop_offer();
+        InactivePublisher::new_from_publisher(self)


UUhh this is nice, actually we could also implement such a thing for c++.

struct Publisher { static InactivePublisher stop(Publisher&& p); };

This is the type state pattern. Unfortunately not fully possible with C++ since it is not fully possible to control the ownership.

The way this works in Rust is that the compiler will complain if you use the publisher after calling stop_offer

elfenpiff · 2022-06-23T14:31:17Z

src/pb/publisher_options.rs

 use std::marker::PhantomData;

 pub(super) struct PublisherOptions {
    pub history_capacity: u64,
    pub node_name: String,
    pub offer_on_create: bool,
+    pub subscriber_too_slow_policy: ConsumerTooSlowPolicy,
    _phantom: PhantomData<()>,


For what is PhantomData required?

This is required for the generic parameter T. Since no other member is dependent on the, some phantom data is required. This does not affect the size of the struct. It is just a marker. See also https://doc.rust-lang.org/std/marker/struct.PhantomData.html

elfenpiff · 2022-06-23T14:33:08Z

src/sb/subscriber.rs

+        Ok((subscriber, SampleReceiverToken {}))
+    }
+
+    pub fn create_without_subscribe(mut self) -> Result<InactiveSubscriber<T>, IceoryxError> {


I would also go back here and just use one create and make subscribe_on_create an option. If not you forgot create_without_subscriber_mt I think.

But then I cannot use the typestate pattern anymore

create -> returns single threaded subscriber st::Subscriber

create_mt -> returns multi threaded subscriber mt::Subscriber

create_without_subscribe -> returns inactive subscriber InactiveSubscriber

The InactiveSubscriber has

subscribe -> returns single threaded subscriber st::Subscriber

subscribe_mt -> returns multi threaded subscriber mt::Subscriber

The st::Subscriber has unsubscribe which returns a InactiveSubscriber, similar to mt::Subscriber.

This means one cannot call subscribe when already subscribed or try to take samples when unsubscribed.

Okay, what about turning the create into a generic where you state the compile time properties, threadsafe and subscribe on create, and the generic finds the most fitting type and returns it?
Something like:

pub fn new() -> InactiveSubscriber<Self> { SubscriberBuilder::<Self>::new("Introspection", "RouDi_ID", "MemPool") .queue_capacity(1) .history_request(1) .build() .create<threadSafe::Nope, subscription::OnCreate>()

But if this does not make sense I think your approach is the most reasonable one.

This depends on the specialization feature which is not yet stabilized.

src/sb/subscriber.rs

codecov-commenter · 2022-06-25T18:20:35Z

Codecov Report

Merging #24 (90705c1) into master (08b520f) will increase coverage by 0.14%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   35.18%   35.32%   +0.14%     
==========================================
  Files          17       15       -2     
  Lines        4332     4252      -80     
==========================================
- Hits         1524     1502      -22     
+ Misses       2808     2750      -58

Impacted Files	Coverage Δ
src/error.rs	`2.46% <0.00%> (ø)`
src/introspection/memory/ffi.rs	`0.00% <0.00%> (ø)`
src/introspection/port/ffi.rs	`0.00% <0.00%> (ø)`
src/introspection/process/ffi.rs	`0.00% <0.00%> (ø)`
src/lib.rs	`42.44% <ø> (ø)`
src/queue_policy.rs	`0.00% <0.00%> (ø)`
src/sb/subscriber.rs	`25.00% <26.66%> (-15.91%)`	⬇️
src/pb/publisher.rs	`32.50% <31.81%> (-11.50%)`	⬇️
src/pb/ffi.rs	`66.66% <83.33%> (-11.12%)`	⬇️
src/sb/ffi.rs	`67.27% <90.90%> (-4.88%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08b520f...90705c1. Read the comment docs.

…oryx v2

elBoberido · 2022-06-27T19:17:42Z

@elfenpiff I would keep the API as is for the 0.1 release and refine it further in follow up releases. Let's discuss API improvements in a discussion page ... @budrus are you able to enable the discussion feature in the general settings?

elBoberido added the refactoring label Jun 21, 2022

elBoberido requested a review from dkroenke June 21, 2022 21:11

elBoberido self-assigned this Jun 21, 2022

elBoberido marked this pull request as ready for review June 21, 2022 21:11

elBoberido requested a review from elfenpiff June 21, 2022 21:14

elfenpiff reviewed Jun 23, 2022

View reviewed changes

elBoberido changed the base branch from iox-#5-add-roudi-environment to master June 25, 2022 18:07

elBoberido force-pushed the iox-23-refine-and-update-publisher-and-subscriber-api branch 2 times, most recently from ab98813 to 670612c Compare June 25, 2022 18:11

elBoberido changed the base branch from master to iox-5-add-coverage June 25, 2022 18:24

elBoberido force-pushed the iox-5-add-coverage branch from 661d735 to 9134b42 Compare June 27, 2022 10:48

Base automatically changed from iox-5-add-coverage to master June 27, 2022 10:55

elBoberido force-pushed the iox-23-refine-and-update-publisher-and-subscriber-api branch 3 times, most recently from 90705c1 to 670612c Compare June 27, 2022 19:04

elBoberido added 7 commits June 27, 2022 21:04

iox-#23 Refine publisher API

2b7cec0

iox-#23 Refine subscriber API

1542f3c

iox-#23 Fix iceoryx spelling

c9a5e83

iox-#23 Add support for all publisher and subscriber options from ice…

c9a61c1

…oryx v2

iox-#23 Update PR template regarding branch naming

09e9b10

iox-#23 Better naming for error names and descriptions

fbd70c1

iox-#23 Use proper error handling for introspection

94b5f52

elBoberido force-pushed the iox-23-refine-and-update-publisher-and-subscriber-api branch from 670612c to 94b5f52 Compare June 27, 2022 19:05

elBoberido merged commit 6afd441 into master Jun 27, 2022

elBoberido deleted the iox-23-refine-and-update-publisher-and-subscriber-api branch June 27, 2022 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-23 refine and update publisher and subscriber api #24

iox-23 refine and update publisher and subscriber api #24

elBoberido commented Jun 21, 2022 •

edited

Loading

elfenpiff Jun 23, 2022

elBoberido Jun 23, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 27, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 23, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 23, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 23, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 23, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 23, 2022

elfenpiff Jun 23, 2022

elBoberido Jun 27, 2022

codecov-commenter commented Jun 25, 2022 •

edited

Loading

elBoberido commented Jun 27, 2022

iox-23 refine and update publisher and subscriber api #24

iox-23 refine and update publisher and subscriber api #24

Conversation

elBoberido commented Jun 21, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 25, 2022 • edited Loading

Codecov Report

elBoberido commented Jun 27, 2022

elBoberido commented Jun 21, 2022 •

edited

Loading

codecov-commenter commented Jun 25, 2022 •

edited

Loading