Skip to content
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

iox-#10 Create sys package #43

Merged
merged 5 commits into from
Jun 29, 2022
Merged

iox-#10 Create sys package #43

merged 5 commits into from
Jun 29, 2022

Conversation

elBoberido
Copy link
Member

@elBoberido elBoberido commented Jun 29, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the Rust coding style and is formatted with rustfmt
  2. Branch follows the naming format (iox-123-this-is-a-branch)
  3. Commits messages are according to this guideline
  4. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  5. Relevant issues are linked
  6. Add sensible notes for the reviewer
  7. All checks have passed (except task-list-completed)
  8. Assign PR to reviewer

Notes for Reviewer

This PR refactors the low level bindings to a separate iceoryx-sys crate to. This should help to fix the docs.rs build.

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

  1. All open points are addressed and tracked via issues

References

@elBoberido elBoberido force-pushed the iox-10-create-sys-package branch 2 times, most recently from 3d1c6f7 to e737017 Compare June 29, 2022 21:28
@elBoberido elBoberido self-assigned this Jun 29, 2022
@elBoberido elBoberido marked this pull request as ready for review June 29, 2022 22:25
@elBoberido elBoberido changed the title [WIP] iox-#10 Create sys package iox-#10 Create sys package Jun 29, 2022
@elBoberido elBoberido merged commit 15316ed into master Jun 29, 2022
@elBoberido elBoberido deleted the iox-10-create-sys-package branch June 29, 2022 23:03
@elBoberido elBoberido linked an issue Jun 29, 2022 that may be closed by this pull request
@@ -9,7 +9,7 @@
<img src="https://user-images.githubusercontent.com/8661268/114321508-64a6b000-9b1b-11eb-95ef-b84c91387cff.png" width="50%">
</p>

Experimental rust wrapper for the [iceoryx](https://github.com/eclipse-iceoryx/iceoryx) IPC middleware.
Experimental Rust bindings for the [iceoryx](https://github.com/eclipse-iceoryx/iceoryx) IPC middleware.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Experimental sounds always like something is most likely to explode. What do you think of a term like "Rust binding in an early stage of development" or "Rust binding where the API may still change"

I know you already merged this but when you create another branch you may adjust this as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be part of #4 which will come later today

pub use subscriber::ConditionVariable;
pub use subscriber::SubscribeState;
pub use subscriber::Subscriber;
pub use subscriber::SubscriberArc;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Arc mean in this context?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means Atomically Reference Counted and indicates that the lifetime of the subscriber is managed by a thread-safe reference-counting pointer

pub use subscriber::SubscribeState;
pub use subscriber::Subscriber;
pub use subscriber::SubscriberArc;
pub use subscriber::SubscriberRc;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Rc mean in this context?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means Reference Counted and indicates that the lifetime of the subscriber is managed by a single-threaded reference-counting pointer

pub use subscriber::SubscriberArc;
pub use subscriber::SubscriberRc;
pub use subscriber::SubscriberStrongRef;
pub use subscriber::SubscriberWeakRef;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have a strong and a weak ref subscriber?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite long since I wrote this code and only God and I understood what it was for. Now only God knows.
Seriously, the Sample holds an Rc or Arc owning reference to the subscriber. The idea was to downgrade this reference to a non-owning Weak for Rc or Weak for Arc reference. But I'm not sure anymore why.

@@ -6,7 +6,7 @@ use crate::ConsumerTooSlowPolicy;

use std::marker::PhantomData;

pub(super) struct PublisherOptions {
pub struct PublisherOptions {
pub history_capacity: u64,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what happens when we make the history capacity an uint16_t in cpp and forget to adjust it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a layer of indirection

let raw = cpp!([service as "const char *",
                instance as "const char *",
                event as "const char *",
                history_capacity as "uint64_t", // <- this comes from the Rust struct
                node_name as "const char *",
                offer_on_create as "bool",
                subscriber_too_slow_policy as "uint8_t"]
                -> *mut Publisher as "PublisherPortUser*"
{
    PublisherOptions options; // <- this is the C++ struct
    options.historyCapacity = history_capacity;
    options.nodeName = IdString_t(TruncateToCapacity, node_name);
    options.offerOnCreate = offer_on_create;
    options.subscriberTooSlowPolicy = static_cast<ConsumerTooSlowPolicy>(subscriber_too_slow_policy);
    auto portData = PoshRuntime::getInstance().getMiddlewarePublisher(
        {
            IdString_t(TruncateToCapacity, service),
            IdString_t(TruncateToCapacity, instance),
            IdString_t(TruncateToCapacity, event)
        },
        options
    );
    return new PublisherPortUser(portData);
});

Depending on the build flags, this will be a warning or an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release on crates.io
2 participants