-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: rolling
Are you sure you want to change the base?
Conversation
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
…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>
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
|
@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. |
There was a problem hiding this 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.
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
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; | ||
} |
There was a problem hiding this comment.
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)
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
I think we can include this approach before highlighting the |
There was a problem hiding this 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.
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
source/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.rst
Outdated
Show resolved
Hide resolved
…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>
Added both a LifecycleNode annd a simple Node in the examples.
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. |
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.