Added warning for "publish" when LifecyclePublisher is not activated#562
Added warning for "publish" when LifecyclePublisher is not activated#562Myzhar wants to merge 2 commits intoros2:masterfrom
Conversation
| publish(std::unique_ptr<MessageT, MessageDeleter> & msg) | ||
| { | ||
| if (!enabled_) { | ||
| rclcpp::Logger logger = rclcpp::get_logger("LifecyclePublisher"); |
There was a problem hiding this comment.
Instead of creating a logger every time (with the potential to have typos in the name), I think it would be better to have a private member variable of type rclcpp::Logger, initialize it in the constructor, and then just use that for all of the RCLCPP_WARN calls.
There was a problem hiding this comment.
@clalancette I tried to implement your suggestion but I get this error and I cannot figure how to solve it:
/home/walter/devel/ros2_ws_src/install/rclcpp/include/rclcpp/logging.hpp:264: error: type/value mismatch at argument 1 in template parameter list for ‘template<class, class> struct std::is_same’
::std::is_same<std::remove_reference<decltype(logger)>::type, ::rclcpp::Logger>::value, \
^
it is related to the definition of the macro:
/**
* \def RCLCPP_WARN
* Log a message with severity WARN.
* \param logger The `rclcpp::Logger` to use
* \param ... The format string, followed by the variable arguments for the format string
*/
#define RCLCPP_WARN(logger, ...) \
static_assert( \
::std::is_same<std::remove_reference<decltype(logger)>::type, ::rclcpp::Logger>::value, \
"First argument to logging macros must be an rclcpp::Logger"); \
RCUTILS_LOG_WARN_NAMED( \
logger.get_name(), \
__VA_ARGS__)
it's like if it cannot resolve the type of the member variable
There was a problem hiding this comment.
Ok, it worked using a static member variable
There was a problem hiding this comment.
I don't think we actually want to use a static member variable here; that will get re-initialized any time a new object of this type of class is instantiated. That may cause havoc for already in-use objects (it is probably not thread-safe, in particular).
Looking at this further, probably the most correct thing to do is to change the LifeCyclePublisher constructor to take a type of rclcpp::node_interfaces::NodeLoggerInterface * in, then use that in the logging calls. Unfortunately, this probably also means that we would have to change the Publisher constructor to take that as well (since they use the same factory). If you don't mind giving that a shot, I think that would actually work out a bit better, though it is more code churn.
There was a problem hiding this comment.
Since each publisher is member of a node (am I right?) I think that automatically receiving the logger of the node that owns it is not a bad thing.
There was a problem hiding this comment.
Since each publisher is member of a node (am I right?) I think that automatically receiving the logger of the node that owns it is not a bad thing.
Yep, totally agreed, I think that is the correct thing. My use of the word "unfortunately" above was merely because it is more churn and work for you :).
|
This looks to me like a duplicate of #574. If I'm wrong, please feel free to re-open. Thanks! |
Fixes #552