Crystal and more #3

lelongg · 2019-02-09T17:23:44Z

This is a pretty big update and unfortunately it might be a bit too hard to review.

It adds :

ROS2 Crystal support
support for nested messages
support for arrays in message
generated RCL binding

esteve · 2019-02-11T09:41:16Z

@lelongg whoa, this is so great, thank you so much! Do you think you could split this into smaller and more focused pull requests? Given that your changes started in December, I think it's doable. It'll make it much easier for me to review, I'll make a few comments in this PR, though.

esteve

Overall it's in the right direction, but I haven't had time to properly review it all, please split up this pull request into smaller ones. I left a few comments, please use this feedback in future contributions.

rclrs/package.xml

README.md

common_interfaces/.github/ISSUE_TEMPLATE.md

NOTICE

rcl_sys/CMakeLists.txt

lelongg · 2019-02-22T15:23:16Z

Most concerns should have been addressed.
Splitting in separate meaningful PR might be difficult since this repository cannot be built in its current state (due to changes in ROS2 dependencies) and there is not much change I can easily remove from this PR and still making it compile.

A compilation order issue remains.
First compilation fails because messages are built before rosidl_generator_rs.
I don't know how to fix this yet.

Solution was to add rosidl_default to the workspace.

esteve · 2019-02-27T18:59:29Z

@lelongg thank you so much for taking the time to address my feedback. Don't worry about splitting up the PR, it'd make it easier sure, but the work you've done is excellent. Let me have a couple of days to review it all and if I don't get back to you, feel free to ping me again. Thanks!

register the 'rosidl_generator_rs'

tprk77 · 2019-03-03T00:01:41Z

Hi @lelongg, I've been trying out your branch. It's very cool and the examples are working great.

But I've run into a pretty serious issue. It seems that iterative builds are using stale source files. I am having to delete the build directory every time I want to compile.

Steps to reproduce:

$ colcon build --packages-up-to rclrs_examples
Starting >>> ament_cmake_export_crates
Finished <<< ament_cmake_export_crates [0.19s]
...
Finished <<< rclrs_examples [39.0s]
colcon build [9/9 done] [0 ongoing]
Summary: 9 packages finished [43.9s]

$ echo "RANDOM JUNK (((((((((((((((((((((" >> src/ros2_rust/rclrs_examples/src/rclrs_publisher.rs

$ colcon build --packages-up-to rclrs_examples
Starting >>> ament_cmake_export_crates
Finished <<< ament_cmake_export_crates [0.20s]
...
Finished <<< rclrs_examples [0.32s]
colcon build [9/9 done] [0 ongoing]
Summary: 9 packages finished [5.07s]

# NOTE This should NOT have succeeded!

$ tail src/ros2_rust/rclrs_examples/src/rclrs_publisher.rs
        message.data = format!("Hello, world! {}", publish_count);
        println!("Publishing: [{}]", message.data);
        publisher.publish(&message)?;
        publish_count += 1;
    }

    Ok(())
}

RANDOM JUNK (((((((((((((((((((((

lelongg · 2019-03-03T01:48:47Z

@tprk77 Yes I noticed that too and it's very inconvenient but I haven't solved it yet.

@esteve I've found issues in array generation when working on this project today and I expect to find some more. Will it interfere with your review if I keep adding fixes to this PR? Alternatively, I can try to remove changes to the generator from this PR and fix it separately. It might lighten this PR a bit.

esteve · 2019-03-03T11:55:45Z

@lelongg I think it'd make this PR easier to review if the generator stuff were in a separate PR, but it's your call, whatever works best for you.

tprk77 · 2019-03-03T23:55:12Z

@lelongg Sounds good. I am all for getting this in and fixing the issues later.

I've been trying this out some more, and I have one more comment. It seems there are a few types like Node which contain Rc making them unable to be Send or Sync. I think it might be useful in some situations to move types like Node, Subscription, and Publisher to other threads. It seems like the underlying rcl should(?) be capable of supporting that, but I don't really know. You would know better than me about the feasibility.

Again, maybe something to consider later on. 😁

.repos file is pruned other PR, so skipping keys shouldn't be needed https://github.com/esteve/ros2_rust/pull/3

esteve

Thanks for removing the generator changes, it's made reviewing this PR easier. This is the feedback I have so far.

rclrs/src/node/mod.rs

esteve · 2019-03-05T08:45:36Z

rclrs_examples/src/rclrs_publisher.rs


    let mut publish_count: u32 = 1;

-    while rclrs::ok() {
+    while context.ok() {


I'd keep rclrs:::ok for symmetry with the other client libraries.

From https://index.ros.org//doc/ros2/Releases/Release-Crystal-Clemmys/.

However, you may just continue to offer a single, global init and shutdown in your client library, and just store a single global context object.

As it is optional I was thinking about adding the global context object later.

It's fine if the context is not global for the time being, we can address that in the future. What I meant is that all the client libraries have an 'init` function that optionally take a context (or use the global one implicitly) and all functions are attached to the root namespace (e.g. rclcpp::init, rclpy.init, etc.)

Moreover, the concept of the context as defined in ROS2 differs slightly from what's presented in this pull request. It's not the context that calls rcl_init, but the opposite:

https://github.com/ros2/rcl/blob/master/rcl/src/rcl/init.c#L59

so in order to have mimic the other client libraries, we need to have a rclrs::init function that accepts a context and all functions are done at the rclrs level (e.g. spin, ok, etc.)

Moreover, the concept of the context as defined in ROS2 differs slightly from what's presented in this pull request. It's not the context that calls rcl_init, but the opposite

It looks like rclcpp context calls rcl_init.

https://github.com/ros2/rclcpp/blob/a54a32915313960497bda2d601cdf98e9f2e9bf0/rclcpp/src/rclcpp/context.cpp#L89

in order to have mimic the other client libraries, we need to have a rclrs::init function that accepts a context and all functions are done at the rclrs level

rclpy.init does take an optional context but rclcpp::init does not.

https://github.com/ros2/rclcpp/blob/e32d17980dcf761712cb854b3703336c380a570e/rclcpp/src/rclcpp/utilities.cpp#L28

How much of a requirement do you think it is to mimic another client interface ? I think it is an important question to settle since it can considerably restrain the design space in the future.

How much of a requirement do you think it is to mimic another client interface ? I think it is an important question to settle since it can considerably restrain the design space in the future.

I'd say it's pretty important that rclrs follows the other client libraries, if only to help users switch to Rust if they know other ROS2 libraries. There's only a handful of methods common to all the libraries (i.e. init, createX, destroyX), so apart from that, we have enough freedom to implement the API in any way we want.

@lelongg did you have time to look into making the rclrs API similar to the other clients'? Thanks.

I didn't make the change yet. What API do you suggest ? We can mimic the rclcpp API by making rclrs::ok take a context object but rclcpp::init does not and so we cannot add rclrs::init until we get the global context.

rclrs_examples/src/rclrs_publisher.rs

esteve · 2019-03-05T08:47:03Z

rclrs_examples/src/rclrs_subscriber.rs

-fn main() {
-    rclrs::init().unwrap();
+fn main() -> rclrs::RclResult {
+    let context = rclrs::Context::default();


rclrs::init() like the other examples.

rosidl_generator_rs/msg/Bool.msg

rclrs/package.xml

esteve · 2019-03-05T09:01:19Z

@tprk77 I'd rather get the pull request fixed or the changes moved to another pull request. This repository works fine right now (albeit only with Bouncy), I'd prefer not to introduce any changes that contain bugs.

lelongg · 2019-03-05T12:48:31Z

@tprk77 the iterative compilation issue should be addressed by #7.

esteve · 2019-05-05T11:16:41Z

Let's better merge this now and address anything else later. Thanks @lelongg !

Fix change state

* Added timer_period_ns * Adds Timer::is_ready(). Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com> * Added comments to avoid warnings * Added the getter for rcl_clock_t --------- Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com> Co-authored-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

lelongg and others added 15 commits December 22, 2018 20:10

nested messages working

a16336e

fix array support

52329a3

add rcl_sys

0afd8b3

add author & fix compilation order

711e557

readme

7664286

format

ce221b2

fix clippy warnings

a890b6b

delete patch

811b718

remove leftover build.rs

4212c5d

fix authors

9d28b9a

add qos support

ee61486

add spin & change handle handling

17fb7d0

clippy

6805c7e

edit readme

a4a35a2

Update README.md

d9af4d3

lelongg changed the title ~~Crystal & and more~~ Crystal and more Feb 10, 2019

esteve requested changes Feb 11, 2019

View reviewed changes

lelongg added 8 commits February 20, 2019 18:46

fix message generation issue

262924e

remove messages

9b2b172

fix fixed size nested array issue

270d344

delete unused files

7fe7ce8

reset authors

8e88968

remove rcl_sys

4e0da9f

remove remaining authors & revert readme

3ac2be1

fix quickstart

b4338cf

lelongg added 3 commits February 22, 2019 17:49

fix fixed size array warning

0d49823

add rosidl_defaults to repos

a13bc91

fix warnings with array generation

30c2043

This was referenced Feb 27, 2019

Crystal support #4

Closed

message and crate generation #5

Closed

christianrauch and others added 2 commits February 28, 2019 01:17

register the 'rosidl_generator_rs'

fb3efd8

Merge pull request #1 from christianrauch/register_gen

0b2ee43

register the 'rosidl_generator_rs'

tprk77 mentioned this pull request Mar 2, 2019

Status? #2

Closed

lelongg mentioned this pull request Mar 4, 2019

Add Dockerfile for CI lelongg/ros2_rust#2

Open

revert message generation to its initial state

4a6683e

ruffsl referenced this pull request in ruffsl/ros2_rust Mar 5, 2019

Remove unnecessary skipped keys

fdf1784

.repos file is pruned other PR, so skipping keys shouldn't be needed https://github.com/esteve/ros2_rust/pull/3

esteve requested changes Mar 5, 2019

View reviewed changes

lelongg added 3 commits March 5, 2019 10:50

add rcl build dependency to rclrs

3f3a205

move spin and spin_once from Node to rclrs

f657ce1

move publisher sleep at the end of the loop

c55dd16

lelongg mentioned this pull request Mar 5, 2019

copy crate at build time #7

Merged

esteve mentioned this pull request Mar 5, 2019

Move repository to github.com/ros2-rust and add members #8

Closed

lelongg added 2 commits March 7, 2019 14:22

re-add msg to rosidl_generator_rs

df7befc

add TODO for publisher and subscription lifetime

5589047

esteve mentioned this pull request Mar 18, 2019

Add Dockerfile #6

Merged

esteve approved these changes May 5, 2019

View reviewed changes

esteve merged commit 98d431d into ros2-rust:master May 5, 2019

esteve mentioned this pull request Jul 9, 2019

WIP: integration of unity_ros2 improvements and port to crystal ros2-dotnet/ros2_dotnet#33

Closed

fmrico pushed a commit to fmrico/ros2_rust that referenced this pull request Jan 10, 2024

Merge pull request ros2-rust#3 from fmrico/fix_change_state

eceda80

Fix change state

Crystal and more #3

Crystal and more #3

Uh oh!

Conversation

lelongg commented Feb 9, 2019

Uh oh!

esteve commented Feb 11, 2019

Uh oh!

esteve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lelongg commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esteve commented Feb 27, 2019

Uh oh!

tprk77 commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lelongg commented Mar 3, 2019

Uh oh!

esteve commented Mar 3, 2019

Uh oh!

tprk77 commented Mar 3, 2019

Uh oh!

esteve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

esteve commented Mar 5, 2019

Uh oh!

lelongg commented Mar 5, 2019

Uh oh!

esteve commented May 5, 2019

Uh oh!

Uh oh!

lelongg commented Feb 22, 2019 •

edited

Loading

tprk77 commented Mar 3, 2019 •

edited

Loading