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

Switch to rclcpp logging and improve messages #167

Merged
merged 4 commits into from
Mar 5, 2019

Conversation

paulbovbel
Copy link
Contributor

@paulbovbel paulbovbel commented Feb 16, 2019

Switch to rclcpp logging and improve messages (Close #166)

@tfoote tfoote added the in review Waiting for review (Kanban column) label Feb 16, 2019
@paulbovbel paulbovbel force-pushed the improve-logging branch 2 times, most recently from fadf3ae to ad5d401 Compare February 16, 2019 19:32
@dirk-thomas
Copy link
Member

Please consider creating two separate pull requests for these two unrelated changes. That will allow both patches to be reviewer easier and move forward independent of each other.

@cottsay cottsay added requires-changes in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Feb 21, 2019
@paulbovbel paulbovbel changed the title Add wait to ROS1 server call and improve logging Switch to rclcpp logging and improve messages Feb 28, 2019
@paulbovbel
Copy link
Contributor Author

Done

include/ros1_bridge/factory.hpp Outdated Show resolved Hide resolved
include/ros1_bridge/factory.hpp Outdated Show resolved Hide resolved
include/ros1_bridge/factory.hpp Outdated Show resolved Hide resolved
@paulbovbel
Copy link
Contributor Author

paulbovbel commented Mar 4, 2019

Fixed up PR with requested changes, thanks for the diligence

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) requires-changes labels Mar 4, 2019
@dirk-thomas
Copy link
Member

Build Status

@dirk-thomas
Copy link
Member

@paulbovbel Please rebase your branch on top of the latest state of the target branch.

@paulbovbel
Copy link
Contributor Author

paulbovbel commented Mar 5, 2019

Done! I accidentally wiped your changes, but I think I've replicated them in 2678029

@dirk-thomas
Copy link
Member

Build Status

@paulbovbel
Copy link
Contributor Author

Fixed.

@dirk-thomas
Copy link
Member

Build Status

@dirk-thomas
Copy link
Member

Fixed.

Please run the packages tests locally to avoid this extra iteration in the future.

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit f80bfdd into ros2:master Mar 5, 2019
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 5, 2019
@dirk-thomas dirk-thomas mentioned this pull request Apr 3, 2019
}

const boost::shared_ptr<ros::M_string> & connection_header =
ros1_msg_event.getConnectionHeaderPtr();
if (!connection_header) {
printf(" dropping message without connection header\n");
RCLCPP_WARN(logger, "Dropping ROS 1 message %s without connection header", ros1_type_name);
Copy link
Member

Choose a reason for hiding this comment

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

The non-format argument must not be a std::string. See #182 for the fix.

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