-
Notifications
You must be signed in to change notification settings - Fork 37
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 throttle #20
Add throttle #20
Conversation
Signed-off-by: wep21 <[email protected]>
src/throttle_node.cpp
Outdated
|
||
std::optional<std::pair<std::string, rclcpp::QoS>> ThrottleNode::try_discover_source() | ||
{ | ||
auto publishers_info = this->get_publishers_info_by_topic(input_topic_); |
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.
it this copy-paste from the relay utility? If so, it seems to me that we might want to factor out a common-utilities lib to deduplicate any of the qos handling logic. Also going to call this non-blocking as I don't mind repeating a small amount of code once - but as soon as it's written a third time it should definitely be made reusable
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.
Fixed at cf45f19
09f8105
to
cf45f19
Compare
include/topic_tools/utilities.hpp
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
#ifndef TOPIC_TOOLS__UTILITIES_HPP_ |
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.
does this need to be a public API? I would be inclined to hide it in src
rather than exposing it publicly to consuming packages. That way we don't have to worry about breaking API/ABI if we change things.
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.
@emersonknapp I moved them into base node class.
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
cf45f19
to
e8f71ff
Compare
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.
LGTM, let's move forward with it
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.
APPROVED, let's move forward.
Closes #6