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

Added a simple ros2 rust logger example #459

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

Conversation

Guelakais
Copy link
Contributor

@Guelakais Guelakais commented Feb 21, 2025

Mentioning #431 I've made a simple ros2 rust node wich includes a simple publisher and logging. It's based on the minimal publisher.
added simple example for ros2 rust node with logging

added the manifest file for the example

added the ros2 manifest file

modechange

Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

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

@Guelakais thanks for your contribution. Could you revert the file mode changes? All the files are marked as executable in your PR and it makes it more difficult to review the actual changes when there are more than 100 files changed

@Guelakais
Copy link
Contributor Author

@Guelakais thanks for your contribution. Could you revert the file mode changes? All the files are marked as executable in your PR and it makes it more difficult to review the actual changes when there are more than 100 files changed

I think it's better now. I have split the whole thing into more commits and left out the modechanges.

@Guelakais
Copy link
Contributor Author

Then I'm a bit confused about the errors. For me, the example was built without any problems.

@esteve
Copy link
Collaborator

esteve commented Feb 21, 2025

@Guelakais what errors? The CI passes:

https://github.com/ros2-rust/ros2_rust/pull/459/checks

@Guelakais
Copy link
Contributor Author

Seems to be gone. Maybe it was just a coincidence.

@romainreignier
Copy link
Contributor

If I may add a comment, I think that this new example is a lot of duplication of the minimal_publisher example.
I think it would make more sense to add the simple log line to the minimal examples and instead, either add more examples in the rustdoc or add a specific example to show the advanced features of logging as done by @avanmalleghem in #460.

@Guelakais
Copy link
Contributor Author

If I may add a comment, I think that this new example is a lot of duplication of the minimal_publisher example. I think it would make more sense to add the simple log line to the minimal examples and instead, either add more examples in the rustdoc or add a specific example to show the advanced features of logging as done by @avanmalleghem in #460.

I understand your perspective. However, I disagree with the implication that we must choose between the two approaches. While it's technically feasible to augment all examples with logging, the core issue is not time, but runtime efficiency. Specifically, ROS 2 logging relies on publishing messages to the \rosout topic via DDS. Each logger instance effectively becomes a publisher, introducing overhead and potentially impacting real-time performance due to the added DDS communication. This is the primary constraint we need to address.

@esteve
Copy link
Collaborator

esteve commented Feb 27, 2025

While it's technically feasible to augment all examples with logging, the core issue is not time, but runtime efficiency.

This point does not matter, these packages are examples on how to use the logging API, not fully fledged production-ready applications.

In going to pick one of the two PRs, whichever seems more complete. #460 seems to exercise more parts of the logging API, which is the end goal of these examples

In any case, thanks @Guelakais for your contribution

@avanmalleghem
Copy link

Well... I didn't create my PR to replace the one from @Guelakais . It is more "Ok we have a small and very simple example to start with. Now let's show some other features". The issue #431 talks about creating logging "examples". Now we have 2 examples 👍

@romainreignier
Copy link
Contributor

In my point of view, examples in an available the documentation is less maintenance that a full example for a simple use case.

@Guelakais
Copy link
Contributor Author

In my point of view, examples in an available the documentation is less maintenance that a full example for a simple use case.

On the other hand, there are simply programmers who like to run the working examples on their computer first to get a feel for what the tool can do. In addition, with the rust compiler toolchain, the examples are always compiled at the same time and tested during compilation, which makes maintenance very easy. The Rust compiler usually provides very helpful explanations as to why something is not working and possible solutions.

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.

4 participants