Skip to content

Add events queue abstract class and implem #40

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.

Sign up for GitHub

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

Jump to bottom
Merged
irobot-ros merged 9 commits into from
Feb 8, 2021
Merged

Add events queue abstract class and implem #40

irobot-ros merged 9 commits into from
Feb 8, 2021

Conversation

@mauropasse
Copy link
Collaborator

@mauropasse mauropasse commented Feb 4, 2021 •
edited
Loading

Here I create a pure virtual EventsQueue class and an implementation SimpleEventsQueue

Usage:

#include "rclcpp/experimental/buffers/simple_events_queue.hpp"

auto events_queue = std::make_unique<rclcpp::experimental::buffers::SimpleEventsQueue>();

auto executor = std::make_shared<rclcpp::executors::EventsExecutor>(std::move(events_queue));

Todo:
Implement in a different PR other policies

alsora
alsora reviewed Feb 5, 2021
View reviewed changes
alsora
alsora reviewed Feb 5, 2021
View reviewed changes
alsora
alsora reviewed Feb 5, 2021
View reviewed changes
alsora
alsora reviewed Feb 5, 2021
View reviewed changes
@mauropasse
Copy link
Collaborator Author

mauropasse commented Feb 5, 2021

With my latest changes this PR could be merged.
This other PR to my own branch is useful to discuss a BoundedEventsQueue implementation:
mauropasse#6

@mauropasse mauropasse mentioned this pull request Feb 5, 2021
Closed
alsora
alsora reviewed Feb 5, 2021
View reviewed changes
rclcpp/include/rclcpp/experimental/buffers/bounded_events_queue.hpp Outdated
*/
RCLCPP_PUBLIC
void
set_queue_size_limit(size_t queue_size_limit)
Copy link
Collaborator

@alsora alsora Feb 5, 2021

Choose a reason for hiding this comment

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

why not pass this to constructor?

Copy link
Collaborator Author

@mauropasse mauropasse Feb 5, 2021

Choose a reason for hiding this comment

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

Did it on 5780ccf

alsora
alsora reviewed Feb 8, 2021
View reviewed changes
rclcpp/test/rclcpp/executors/test_events_queue.cpp Outdated
bounded_queue->set_queue_size_limit(10);

// Create an events executor using the bounded queue
EventsExecutor executor_sub(std::move(bounded_queue));
Copy link
Collaborator

@alsora alsora Feb 8, 2021

Choose a reason for hiding this comment

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

The unit-tests for the queues should be independent from the executor.
You should just create a queue and start manually pushing/popping things

Copy link
Collaborator Author

@mauropasse mauropasse Feb 8, 2021

Choose a reason for hiding this comment

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

Ok I pushed this commit mauropasse@6fb2463
but this PR is not being updated, strange. I'll wait some time to see if gets updated

@mauropasse mauropasse closed this Feb 8, 2021
@mauropasse mauropasse reopened this Feb 8, 2021
@irobot-ros irobot-ros merged commit 4d78d42 into irobot-ros:irobot/add-events-executor Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alsora alsora alsora left review comments

+1 more reviewer

@irobot-ros irobot-ros irobot-ros approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mauropasse @alsora @irobot-ros