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

Add context builder #312

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

routiful
Copy link

@routiful routiful commented May 17, 2023

Hello :)

I've studied Rust since last year and I've been developing some applications with Rust.
ROS client library in Rust is the best approach to write solid nodes and packages.
As one of ROS users, I should contribute features so that many ROS users can use this.

To make context builder is assigned to @DS3a, #206. However, it hasn't developed since last year.
Thus, I commited that to support various features such as domain_id.

@huchijwk

Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
@DS3a
Copy link
Contributor

DS3a commented May 17, 2023

Hi, I am extrememely sorry for the delay, I got busy with school, internships, clearing backlogs, etc. Thank you so much for picking up the slack I caused.

I have used ros2rust for a lot of my personal projects and definitely want to contribute to it. But unfortunately I won't be able to for atleast a few months, until my summer break starts.

Thanks again, I will find some other issue to resolve with this repository when I get the time.

Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
@routiful
Copy link
Author

I have fixed CI in foxy and humble. However, warnings and errors from CI in rolling are not related to this PR. Would you help me to solve this?

@esteve @nnmm

@esteve
Copy link
Collaborator

esteve commented May 22, 2023

@routiful thank you so much for working on this! I'll look into the CI issues with rolling

@routiful
Copy link
Author

I searched the CI issues with rolling. The error codes shown below are related rosidl_dynamic_typesupport_* structs.

error: `extern` block uses type `u128`, which is not FFI-safe
    --> /__w/ros2_rust/ros2_rust/ros_ws/src/m2eakkraxx/ros2_rust/rclrs/target/debug/build/rclrs-77127645f5241a56/out/rcl_bindings_generated.rs:7310:26
     |
7310 |         dynamic_message: *mut rosidl_dynamic_typesupport_dynamic_data_t,
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
     |
     = note: 128-bit integers don't currently have a known stable ABI

This issue is related rust-bidgen and it seems that they haven't founded solution yet (rust-lang/rust-bindgen#1549). Someone used a blacklist_function method in build.rs to avoid this (zmwangx/rust-ffmpeg-sys@d54e559)

The Unsafe functions might make unpredictable behavior when its are executed. Thus, I understands its make occur compile errors. I'll keep find a solution how to handle this.

Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
@routiful
Copy link
Author

The CI issues with rolling is caused by a rosidl_dynamic_typesupport package. A dynamic data is able to get long double(float128) type but bindgen can't handle this type now.

So I suggest that make block rosidl_dynamic_typesupport (4481f99) for now in order to contribute code to other ros distro and then create new issue to handle this.

@esteve @nnmm

Copy link
Contributor

@nnmm nnmm left a comment

Choose a reason for hiding this comment

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

Hey @routiful, sorry it has taken so long for someone to pick up your PR! Here's a first review.

The blocklist approach is fine with me since we don't use any of these functions for now.

/// [1]: https://docs.ros.org/en/rolling/Concepts/About-Domain-ID.html
pub fn domain_id(mut self, domain_id: usize) -> Self {
#[cfg(ros_distro = "foxy")]
std::env::set_var("ROS_DOMAIN_ID", domain_id.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

In Foxy, could you just disable the entire domain_id functionality in the context builder, and add it to the node builder instead?

Copy link
Author

Choose a reason for hiding this comment

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

New commits haven't passed doc-test due to domain_id func doesn't exist in Foxy. I have searched doc_cfg but it is not merged yet rust-lang/rust#43781.
Is there any solutions to test each ros distro?

Copy link
Author

Choose a reason for hiding this comment

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

I have modified doc-test :)

/// Creates a rcl_init_options_t struct from this builder.
///
/// domain id validation is performed in this method.
fn create_rcl_init_options(&self) -> Result<rcl_init_options_t, RclrsError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would make the builder have a field of rcl_init_options_t type, instead of only building it in build(). But I don't have a good argument for it, so I'll leave it up to you whether you want to do that :)

Copy link
Author

Choose a reason for hiding this comment

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

I accept your suggestion. That field give more flexible features to assign allocators and a domain_id.

Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
Signed-off-by: Taehun Lim <taehun.lim@42dot.ai>
@routiful
Copy link
Author

@nnmm Any updates?

@Guelakais
Copy link
Contributor

Is this pull request still relevant and if so, is it still in a state so that it can be merged with the main branch?

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

Successfully merging this pull request may close these issues.

None yet

5 participants