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

Add tutorial on Node Interfaces Template Class #4992

Open
wants to merge 12 commits into
base: rolling
Choose a base branch
from

Conversation

NickTziaros
Copy link

Relevant to isuue #4924.

Created the tutorial Using the Node Interfaces Template Class (C++) in the intermediate section.
Let me know if this is what you had in mind.

  • Created Using-Node-Interfaces-Template-Class
  • Modified Intermediate.rst

NickTziaros and others added 6 commits February 7, 2025 16:15
…lass.rst

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
@alsora
Copy link
Contributor

alsora commented Feb 7, 2025

I noticed people having a lot of resistance towards the templated NodeInterface class.

I would suggest to at least mention the possibility to also do something like this if you need a simple solution

    void node_info(
        rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_interface,
        rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface)
    {
      RCLCPP_INFO(node_logging_interface->get_logger(), "Node name: %s", node_base_interface->get_name());
    }

@NickTziaros
Copy link
Author

@alsora I considered mentioning this approach as well, especialy since it was also brought up in the lightning talk .

However, I thought it might be out of scope since this tutorial focuses on showcasing the Template class. I might be wrong though . I would love the input from @kscottz @Yadunund since they raised the issue.

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

This is great! Thanks a lot for the contributions! Left a couple suggestions to consider.

Comment on lines 102 to 114
int main(int argc, char * argv[])
{
rclcpp::init(argc, argv);
rclcpp::executors::SingleThreadedExecutor exe;
std::shared_ptr<SimpleLifeCycleNode> lc_node =
std::make_shared<SimpleLifeCycleNode>("Simple_LifeCycle_Node");
exe.add_node(lc_node->get_node_base_interface());
node_info(*lc_node);
exe.spin();
rclcpp::shutdown();

return 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the example will be more powerful if we also instantiate an instance of SimpleNode and call node_info() with this object. (we can skip spinning the nodes to keep focus on API usage)

@Yadunund
Copy link
Member

Yadunund commented Feb 7, 2025

I noticed people having a lot of resistance towards the templated NodeInterface class.

I would suggest to at least mention the possibility to also do something like this if you need a simple solution

    void node_info(
        rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_interface,
        rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface)
    {
      RCLCPP_INFO(node_logging_interface->get_logger(), "Node name: %s", node_base_interface->get_name());
    }

I think we can include this approach before highlighting the NodeInterfaces way and emphasize that the latter has some advantages including compactness, and potentially API stability if relying on ALL_RCLCPP_NODE_INTERFACES. @alsora would you agree?

Copy link
Collaborator

@kscottz kscottz left a comment

Choose a reason for hiding this comment

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

@NickTziaros Thanks for stepping up here! We really appreciate it.

I think the other reviewers have covered most of the required changes. I sent you two suggestions. The first one should help clarify the motivation for the tutorial. The second one is a minor change.

I'll approve on the condition that these get folded in and everyone else's concerns are mostly addressed.

NickTziaros and others added 4 commits February 10, 2025 12:08
…lass.rst

Co-authored-by: Katherine Scott <katherineAScott@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: Katherine Scott <katherineAScott@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
…lass.rst

Co-authored-by: yadunund <yadunund@gmail.com>
Signed-off-by: Nikos Tziaros <33639811+NickTziaros@users.noreply.github.com>
@NickTziaros
Copy link
Author

I think the example will be more powerful if we also instantiate an instance of SimpleNode and call node_info() with this object. (we can skip spinning the nodes to keep focus on API usage)

Added both a LifecycleNode annd a simple Node in the examples.

I think we can include this approach before highlighting the NodeInterfaces way and emphasize that the latter has some advantages including compactness, and potentially API stability if relying on ALL_RCLCPP_NODE_INTERFACES. @alsora would you agree?

I also creates an example on passing the node interfaces explicitely.

Thank you all for you input. I tried to incorporate the changes you asked in this last commit. Let me know what you think and how I can make this better.

@kscottz
Copy link
Collaborator

kscottz commented Feb 10, 2025

@Yadunund @ahcorde This LGTM. If we're all happy let's get this merged. I would like to get it out to the community ASAP.

@ahcorde ahcorde requested a review from Yadunund February 11, 2025 12:16
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.

5 participants