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

Dashing support #19

Closed
OTL opened this issue Aug 15, 2019 · 24 comments
Closed

Dashing support #19

OTL opened this issue Aug 15, 2019 · 24 comments

Comments

@OTL
Copy link

OTL commented Aug 15, 2019

Do you have a plan to support Dashing?
Dashing is the first LTS ROS2 distro. I think it is great if ros2_rust supports it!

@lelongg
Copy link
Contributor

lelongg commented Aug 26, 2019

I agree it definitely should be done but I don't think anybody is working on this right now.

@AndrewJSchoen
Copy link

Yes, this would be great. I tried, on a whim, to get it to compile on my ROS2 eloquent install, and this failed. If you are interested in the error I got, I can send it along.

@AndrewJSchoen
Copy link

Finally got some time to migrate back to this issue. It seems like the first set of issues is to adapt to using the IDL interface, as opposed to individual msg, srv, and action files, and is therefore related to #11. For example, I get the following:

Package 'rosidl_generator_rs' registered an extension for the obsolete
  extension point 'rosidl_generate_interfaces'.  It is being skipped and
  needs to be updated to the new extension point
  'rosidl_generate_idl_interfaces'.Please refer to the migration steps on the
  Dashing release page for more details.

Unfortunately, I haven't done much in this realm of ROS (or ROS2), so I wasn't sure how to proceed. Are there some examples you @lelongg may have, or suggestions?

@AndrewJSchoen
Copy link

Quick update on stuff, I have a fork that I have been working on a bit. So far, ignoring the rosidl generator error, I have been able to update the bindings for QoS and address some dyn warnings. At the point I am currently, it seems like the publisher code doesn't compile because it is missing the std_msgs package, which I assume is because of the generator error?

error[E0432]: unresolved import `std_msgs`
 --> src/rclrs_subscriber.rs:2:5
  |
2 | use std_msgs;
  |     ^^^^^^^^ no `std_msgs` external crate

error[E0432]: unresolved import `std_msgs`
 --> src/rclrs_publisher.rs:2:5
  |
2 | use std_msgs;
  |     ^^^^^^^^ no `std_msgs` external crate

@esteve and/or @lelongg, Do you have any intuition for what needs to be adjusted here for idl support? I haven't been able to find a good set of documentation about what we need. Maybe I'm just looking in the wrong place.

@lelongg
Copy link
Contributor

lelongg commented May 8, 2020

It's been a while since I dived into this. One thing I remember is that if you use a message in some package in your workspace, the package containing this message's definition must be in this workspace as well. Otherwise the rust code generation is not triggered. Do you have quick instructions on how your issue can be reproduced ?

@AndrewJSchoen
Copy link

Hi @lelongg, so far I haven't included any other packages in the workspace, just the ros2_rust package (my fork). Therefore, all I am doing is running colcon build of my workspace in ROS2 Eloquent.

It seems like rosidl_generate_interfaces was replaced by rosidl_generate_idl_interfaces. If I register to that extension point instead, I get an error due to the fact that ${_idl_file_without_actions} in rosidl_generator_rs/cmake/rosidl_generator_rs_generate_interfaces.cmake is empty. Presumably, that is because ${rosidl_generate_interfaces_IDL_FILES} is also empty. Not sure if there is a new variable that has replaced that one in Eloquent.

@ruffsl
Copy link
Contributor

ruffsl commented May 8, 2020

Nice to see some activity here. I took a stab at updating rclrs to Eloquent a few months ago, but a got bit overwhelmed with the intricacies in the changes to rosidl in Eloquent. If we'd like to invest some time updating everything, perhaps we could just leapfrog to the next LTS with Foxy releasing soon?

@AndrewJSchoen
Copy link

@ruffsl Yeah, I think those intricacies are the same ones that I am seeing here, haha. I think that may make sense.

@lelongg
Copy link
Contributor

lelongg commented May 8, 2020

Do you know if rosidl can be considered stable yet ? Tracking changes to message generation internal details proved to be painful. I considered bypassing cmake based generation to use rust macro (as rosrust does) or build.rs based generation. How would you feel about this ?

@AndrewJSchoen
Copy link

@lelongg I'm not sure, unfortunately. Maybe @dirk-thomas or some people from OSRF might be able to offer some suggestions in that area?

@ruffsl
Copy link
Contributor

ruffsl commented May 8, 2020

Do you know if rosidl can be considered stable yet ?

In general for at least for Foxy? The Foxy API freeze deadline has passed, so I'd think the change log written in index page for the upcoming LTS is accurate. Newer point releases seem just to pertain to tests, documentation, or bug fixes.

https://index.ros.org/doc/ros2/Releases/Release-Foxy-Fitzroy/#rosidl-generator-c-cpp-namespace-api-changes

ros2/rosidl#446

I considered bypassing cmake based generation to use rust macro (as rosrust does) or build.rs based generation. How would you feel about this ?

I'd also like to move away from cmake, as it makes using a native cargo workflow more tricky. Although it seems ROS2 is heavily invested in CMake for packaging and IDL macros. Perhaps we could revisit colcon_cargo to see how we could wrap such functionality into something more friendly to the rust ecosystem. E.g. registering rust made ros packages and assets into he ament index, etc.

@AndrewJSchoen
Copy link

@ruffsl Would there be any downsides of going all-in on colcon_cargo, as opposed to keeping things wrapped in CMake? Would any integration be lost along the way? If all the functionality could be kept, and be stable, making things more in-line with native cargo workflows seems like it would make sense.

@dirk-thomas
Copy link

Do you have any intuition for what needs to be adjusted here for idl support? I haven't been able to find a good set of documentation about what we need. Maybe I'm just looking in the wrong place.

The changes have been introduced for Dashing and are described in the migration section of that release: https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#rosidl
The best examples are the existing message generators. The changes to them are referenced from the migration description.

Nothing significant has changed in Eloquent or Foxy (the latter has a few headers moved which should be a simple search-n-replace). I can't predict the future but the rosidl stuff is fairly stable imo.

Would there be any downsides of going all-in on colcon_cargo, as opposed to keeping things wrapped in CMake?

Existing interface packages are using CMake to trigger code generation. So for Rust there still must be a CMake part which is the extension point being triggered by each of these interface packages passing the interface files which should be processed.

Everything in the process after this triggering can be written in whatever language you like. The existing code generators use CMake and Python to perform the work (Python logic e.g. converts .msg files into .idl and also provides a parser for these .idl files). While you could do that all yourself I don't think it makes any sense to duplicate the complex logic necessary in that part.

@mosteo
Copy link

mosteo commented May 9, 2020

Another alternative that I'm going to use in the Ada client library is linking to the C typesupports and use the introspection to generate the static Ada types. I already use the introspection functions for data access in the Ada library, so it should be relatively straightforward. Honestly, when I looked at the C++/Python templates for their struct generation I found impossible to understand/reuse that.

@AndrewJSchoen
Copy link

Thanks @mosteo for that as a reference. I’ll take a look.

@lelongg
Copy link
Contributor

lelongg commented May 9, 2020

Existing interface packages are using CMake to trigger code generation. So for Rust there still must be a CMake part which is the extension point being triggered by each of these interface packages passing the interface files which should be processed.

If I remember correctly and if this has not changed, using the existing rosidl generator process with rust would really pay off if rust generation is included in rosidl_defaults, for the trigger to take place even when message from outside the current workspace are used. Is it an open alternative ? If so what would be the requirements for a new generator to be included in rosidl_defaults ?

The other alternative I was referring to would completely bypass the cmake process and the generated code would be a build artifact of the end user package (like rosrust does).

Everything in the process after this triggering can be written in whatever language you like. The existing code generators use CMake and Python to perform the work (Python logic e.g. converts .msg files into .idl and also provides a parser for these .idl files). While you could do that all yourself I don't think it makes any sense to duplicate the complex logic necessary in that part.

I was not familiar with that step but I agree that duplicating the conversion of .msg to .idl seems like a waste of time. Maybe this can be prevented by having the .idl stored in the ament store, so we would be able to parse the it directly.

@dirk-thomas
Copy link

Maybe this can be prevented by having the .idl stored in the ament store, so we would be able to parse the it directly.

You might want to take a look at the grammar / parser being used in rosidl_parser (e.g. https://github.com/ros2/rosidl/blob/master/rosidl_parser/rosidl_parser/grammar.lark). I don't think you want to come up with your own.

Honestly, when I looked at the C++/Python templates for their struct generation I found impossible to understand/reuse that.

The logic in the em might look complex but once you get the syntax it is fairly straight forward what the templates are doing. I would suggest looking at e.g. Python - comparing the template for a message with the generated Python code for a simple message like geometry_msgs/Vector3 should help to see what it is supposed to generate and then find the equivalent logic in the template.

@AndrewJSchoen
Copy link

I have some experience with templating (although not empy specifically). I can take a look at that as well.

@lelongg
Copy link
Contributor

lelongg commented May 9, 2020

You might want to take a look at the grammar / parser being used in rosidl_parser (e.g. https://github.com/ros2/rosidl/blob/master/rosidl_parser/rosidl_parser/grammar.lark). I don't think you want to come up with your own.

I see what you mean, however that doesn't seem too involved and should represent a reasonable amount of work with the right parser generator. If rust generator cannot become part of rosidl_default my preference would go to writing something like this as I find working with cmake very difficult (I don't know what tool to use to figure what's going on during a cmake build, so I have a hard time debugging) but it's very personal and probably not a valid technical argument.

@mosteo
Copy link

mosteo commented May 10, 2020

The logic in the em might look complex but once you get the syntax it is fairly straight forward what the templates are doing. I would suggest looking at e.g. Python - comparing the template for a message with the generated Python code for a simple message like geometry_msgs/Vector3 should help to see what it is supposed to generate and then find the equivalent logic in the template.

Thanks for these pointers, I will keep that in mind if I have to go back to templates.

@AndrewJSchoen
Copy link

AndrewJSchoen commented Aug 5, 2020

Just as an fyi, I have done most, if not all of the QOS work in the master branch of the fork here. Combined with the IDL stuff that @nnarain did for IDL stuff (#21), what should be our next steps?

@nnarain
Copy link
Contributor

nnarain commented Aug 6, 2020

I can make necessary changes to the pipeline to get the build passing on my PR: #24. Which will pull in changes from: #20.

The pipeline changes are probably the main thing to resolve here since it's blocking PRs. Afterwards @AndrewJSchoen your QOS changes can be added.

@nnarain
Copy link
Contributor

nnarain commented Aug 15, 2020

Looks like switching the build over to foxy was enough to get it passing.

@nnmm
Copy link
Contributor

nnmm commented Mar 18, 2022

I think this can be closed now, since we're already on Foxy+. If there is still desire to add Dashing to CI, please reopen.

@nnmm nnmm closed this as completed Mar 18, 2022
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

No branches or pull requests

8 participants