Conversation
3997ed3 to
e9fc96a
Compare
|
Ready for review. The point of this PR is to get rid of free functions so that the demo nodes are set up for using logging macros how we expect them to be used (accessing the node's logger). There are still some nodes that don't inherit from Node in this repo but not in the demo_node_cpp package, with the exception of parameter nodes that are blocked by ros2/rclcpp#409. I left the sync client without node inheritance because there isn't really a distinction between sync and async when using a response callback function (which is required if spin has been called on the node by the main). The
|
|
@ros2/team waiting for review. If you notice inconsistency in the usage of |
| timer_->cancel(); // We only want to make a single request. | ||
| this->make_request(); | ||
| }; | ||
| timer_ = create_wall_timer(0s, on_timer); // This will trigger once spin is called on the node. |
There was a problem hiding this comment.
@Karsten1987 and I spoke about this and it should be fine to just call make_request straight away here (should rename the method to start_async_request or something) since both approaches just queue something for once the node gets spun on.. I'll give it a shot
There was a problem hiding this comment.
simplified in a010a4d @mikaelarguedas fyi (you also mentioned this was unnecessarily complicated FWIU)
| request->b = 3; | ||
| private: | ||
| rclcpp::client::Client<example_interfaces::srv::AddTwoInts>::SharedPtr client_; | ||
| rclcpp::timer::TimerBase::SharedPtr timer_; |
There was a problem hiding this comment.
we don't need that timer_ anymore, do we?
This reverts commit cc5cf74.
b9e5794 to
35f99d5
Compare
rebased since those PRs have been merged and removed any extra namespaces so you shouldn't see e.g. |
|
CI (planning to merge when comes back green):
|
I want to swap to using logging macros in #191.
Having free functions was complicating the process, but these demos should be using Node inheritance anyway.
haven't done the service demos yet