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

use rclcpp logging macros #715

Merged
merged 6 commits into from
Apr 6, 2021
Merged

use rclcpp logging macros #715

merged 6 commits into from
Apr 6, 2021

Conversation

Karsten1987
Copy link
Collaborator

sits on top of #714 but essentially replaces all ROSBAG2_TRANSPORT_LOG_* with their respective RCLCPP_* equivalent. We introduced the custom macros back at the original time of writing as there weren't any _STREAM macros available in rclcpp at that time.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987 Karsten1987 requested a review from a team as a code owner April 6, 2021 01:04
@Karsten1987 Karsten1987 requested review from jaisontj and prajakta-gokhale and removed request for a team April 6, 2021 01:04
@emersonknapp
Copy link
Collaborator

In theory I like the idea of not having custom logging macros.

I'm not sure I'm sold on the pattern of "borrowing" the logger from the transport_node_ in code that runs outside the node - maybe the Player / Recorder should have their own loggers? I'm not 100% sure on that, just thinking out loud.

It's unfortunate that the logging "feels" more verbose this way - total characters mostly fewer, but more lines because of the extra argument.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Collaborator Author

interesting. I would have actually argued pretty much in an opposite way.

I assume that with the Galactic freeze, we'll remove the rosbag2_transport class altogether. Yet, I don't really anticipate a real API change for the Player/Recorder classes, that is both still get a node passed in from an external application context. With this in mind, I'd very much only change and adjust the logger configuration for that node and not for two separate loggers. I am also not sure at that point if one can configure loggers which are not attached to a ROS node.

@emersonknapp
Copy link
Collaborator

I think I just had to sit with it for a few more minutes - I am convinced by your reasoning about Player/Recorder. And you're right, I plan for us to remove Rosbag2Transport class altogether.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@delete-merged-branch delete-merged-branch bot deleted the branch master April 6, 2021 05:29
@Karsten1987 Karsten1987 changed the base branch from use_rclcpp_generic_pubsub to master April 6, 2021 05:29
@Karsten1987
Copy link
Collaborator Author

Karsten1987 commented Apr 6, 2021

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Collaborator Author

Karsten1987 commented Apr 6, 2021

I can reproduce the test failure locally, yet I really don't have a clue how this PR introduces a segfault in the rosbag2_cpp converter impl:

 ➭ ros2 bag record -a -f some_rmw
[INFO] [1617724218.388506984] [rosbag2_cpp]: No plugin found providing serialization format. Falling back to checking RMW implementations.
[INFO] [1617724218.388555046] [rosbag2_cpp]: No plugin found providing serialization format. Falling back to checking RMW implementations.
[ERROR] [1617724218.401780979] [rosbag2_cpp]: Could not initialize RMWImplementedConverter: No RMW implementation found supporting serialization format some_rmw
Segmentation fault: 11

EDIT: Turns out we've used a node for logging even though we don't have a guarantee that the node is available.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987 Karsten1987 merged commit 687f19d into master Apr 6, 2021
@delete-merged-branch delete-merged-branch bot deleted the use_rclcpp_logging branch April 6, 2021 17:50
Kettenhoax pushed a commit to Kettenhoax/rosbag2 that referenced this pull request Apr 9, 2021
* use rclcpp::Node for generic pub/sub

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* address review comments

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* export symbols on windows

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* use rclcpp logging macros

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* do not use node for error logging

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
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

2 participants