search

ros2 / urdf

Repository for URDF parsing code
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

Replace fprintf with ROS 2 logging #16

Open sloretz opened 4 years ago

sloretz commented 4 years ago

The Model class communicates error messages by calling fprintf()

https://github.com/ros2/urdf/blob/533de85e9a4611fc42855b1d65d9f32cb42b0d6b/urdf/src/model.cpp#L70

This should instead use ROS 2's logging functionality. This might mean passing in a logger instance to the class.

See also:

https://github.com/ros2/urdf/pull/13#pullrequestreview-461949731 https://github.com/ros2/urdf/pull/13#discussion_r473320615

guru-florida commented 3 years ago

Should the parse do any writing to log files? As a suggestion, perhaps it could throw a parsing exception? The caller can then decide if the error should be logged or silent.

FYI I am resolving a similar issue over in ros2_controls URDF Transmissions parser (PR 182) and I figured I'd come over here to see how it was done and get a feel for precedence. Perhaps find an existing parsing exception class. @Karsten1987 suggested I remove the rclcpp dependency since I am only using it for logging in favor of rclutils logging directly. For now, I am going to switch the Transmission parsing class to throw runtime_error exception and there is also a simple convenience function that parses urdf Transmissions using that class that will catch the exception and send to rclutils logging (which rclcpp logging is based on). Eventually I'd like to integrate Transmissions parsing as part of this urdf module. Thoughts?

If you would like I can replace the fprintf calls with rclutils calls since I am doing the same thing over there. Unless you use other parts of rclcpp it reduces the dependency footprint.

clalancette commented 3 years ago

If you would like I can replace the fprintf calls with rclutils calls since I am doing the same thing over there. Unless you use other parts of rclcpp it reduces the dependency footprint.

As it stands, this package doesn't depend on either rclcpp or rcutils. I'd be in favor of using the RCUTILS macros, as adding a dependency on rclcpp just for logging seems too heavyweight. But I'd also like to hear @sloretz 's opinion on it.

sloretz commented 3 years ago

As it stands, this package doesn't depend on either rclcpp or rcutils. I'd be in favor of using the RCUTILS macros, as adding a dependency on rclcpp just for logging seems too heavyweight.

Agreed - I would use the rcutils API for this package. As far as I can tell that only requires including the macros and using them.

#include <rcutils/logging_macros.h>
// ...
        RCUTILS_LOG_WARN_NAMED(
          "my_package", "Blah Bla Blah [%s] failed", some_str.c_str());
// ...

FYI I am resolving a similar issue over in ros2_controls URDF Transmissions parser (PR 182) and I figured I'd come over here to see how it was done and get a feel for precedence. Perhaps find an existing parsing exception class.

@guru-florida You might find inspiration in SDFormat's Errors class. The APIs sdf::read[String|File|Xml]() Return a bool, but also offer Errors which has info in case it does fail.

ahcorde commented 3 weeks ago

Related PR https://github.com/ros2/urdf/pull/37