Skip to content

Added warning for "publish" when LifecyclePublisher is not activated#562

Closed
Myzhar wants to merge 2 commits intoros2:masterfrom
Myzhar:master
Closed

Added warning for "publish" when LifecyclePublisher is not activated#562
Myzhar wants to merge 2 commits intoros2:masterfrom
Myzhar:master

Conversation

@Myzhar
Copy link
Copy Markdown

@Myzhar Myzhar commented Sep 28, 2018

Fixes #552

@tfoote tfoote added the in review Waiting for review (Kanban column) label Sep 28, 2018
@Myzhar Myzhar changed the title Added warning if for "publish" when LifecyclePublisher is not activated Added warning for "publish" when LifecyclePublisher is not activated Sep 28, 2018
publish(std::unique_ptr<MessageT, MessageDeleter> & msg)
{
if (!enabled_) {
rclcpp::Logger logger = rclcpp::get_logger("LifecyclePublisher");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, it worked using a static member variable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 :).

@clalancette clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Jan 3, 2019
@cottsay
Copy link
Copy Markdown
Member

cottsay commented Feb 21, 2019

This looks to me like a duplicate of #574. If I'm wrong, please feel free to re-open. Thanks!

@cottsay cottsay closed this Feb 21, 2019
@cottsay cottsay added duplicate This issue or pull request already exists and removed in progress Actively being worked on (Kanban column) requires-changes labels Feb 21, 2019
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
Narrow scope of subscription to prevent leak and separate nominal test from init/fini test.

Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants