-
Notifications
You must be signed in to change notification settings - Fork 128
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 simple talk/listener tutorial, matching the official ROS 2 documentation #378 #390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic! Thank you so much!
Sorry for all the little nitpicks, most of them are pretty minor, but I think they'll help promote clarity. Despite all of them, though, this is really excellent work! I can see this helping a lot of newcomers to ROS 2 Rust!
Unfortunately, I found the tutorial itself unfinished so far and wanted to incorporate the justified criticism properly. One big change is that there is now a separate `Build and Run` section, which generally explains the correct building and starting of nodes. There are also a few sections that deal with explanations and a few that most people skip over. I'm largely satisfied so far and of course open to suggestions for improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Guelakais thanks for taking the time to write this tutorial. I've done a brief pass at it, I'll do a more thorough review later. In the mean time, some highlights:
- Please be consistent with the spelling of Rust, ROS 2 and ros2-rust
- The examples are great, I prefer if they were included in the PR instead of linked to Gitlab
- Some of the text seems to be taken directly from the official ROS 2 tutorials. Either rewrite it or acknolewdge the source material
I'm going to rewrite the description of the PR to better describe the intent and goals
@Guelakais please follow the spelling I described in #390 (review), it's Rust (first letter in upper case, rest in lower case), ROS 2 (with a space between ROS and 2), and ros2-rust. Thanks. |
As long as I don't understand how to run colcon with cargo-args correctly, it just doesn't do any good when I start adding code. I need proper instructions on how to run your buildchain locally. Otherwise it just doesn't work.
As long as I don't understand how your building process works properly and can reproduce it, I won't add any code for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to replicate your build process locally and on Github it takes 10 minutes every time. That's why I've decided not to include any sample code for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thank you so much for the expanded tutorial! I really appreciate the effort here and its nice to see someone giving rclrs
a spin.
Overall, some small formatting changes. But, its worth mentioning that ROS 2's official documentation for Writing a simple publisher and subscriber builds this page from a rst
(reStructredText) file.
Now we have a lot of documentation in markdown already. I don't think we should swap everything to rst
, but I think we should stick to basic markdown syntax (as described here) to keep things simple and consistent with our own docs.
Added language declaration to the first upcoming `Cargo.toml` code snippet in lines 49 - 60 to allow markdown colouring of this snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Co-authored-by: Sam Privett <[email protected]>
Co-authored-by: Sam Privett <[email protected]>
Co-authored-by: Sam Privett <[email protected]>
Co-authored-by: Sam Privett <[email protected]>
yeah, ok Co-authored-by: Sam Privett <[email protected]>
@Guelakais thank you so much for all the impressive work and the patience going through our feedback. We're going to merge this and if we need to tweak anything we'll do in a follow-up PR, the work you did is great and very much appreciated! 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Guelakais thanks!
@jhdcs unless there are blockers, can you approve this PR so I can merge it? Thanks! |
I've decided to write my own Tutorial for your Repository. Suggestions are welcome!
fixes #378