Skip to content

Add Publish Action #20

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
merged 4 commits into from
Mar 22, 2023
Merged

Add Publish Action #20

merged 4 commits into from
Mar 22, 2023

Conversation

@tonynajjar
Copy link

@tonynajjar tonynajjar commented Mar 22, 2023 •
edited
Loading

@tonynajjar tonynajjar requested a review from saraalarab1 March 22, 2023 11:47
@tonynajjar
Copy link
Author

tonynajjar commented Mar 22, 2023

POC works, just need to do some more checks

Tony Najjar added 3 commits March 22, 2023 13:25
@tonynajjar tonynajjar removed the draft label Mar 22, 2023
@tonynajjar
Copy link
Author

tonynajjar commented Mar 22, 2023

Ready for review. We'll need to think of a cleaner solution long-term

jplapp
jplapp approved these changes Mar 22, 2023
View reviewed changes
Copy link

@jplapp jplapp left a comment

Choose a reason for hiding this comment

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

This looks great and I think of all our discussed approaches, this is the nicest one

Regarding the nav2 generalization - I think one would also need to think about how to abstract the "trigger" so whether using a topic , some fixed interval, or e.g. an update from the input topics

@tonynajjar tonynajjar merged commit 8996e15 into humble Mar 22, 2023
jplapp
jplapp reviewed Mar 23, 2023
View reviewed changes
nav2_collision_monitor/src/collision_monitor_node.cpp
for (std::shared_ptr<Polygon> polygon : polygons_) {
if (polygon->getActionType() == PUBLISH) {
timer_ = this->create_wall_timer(
100ms,
Copy link

@jplapp jplapp Mar 23, 2023

Choose a reason for hiding this comment

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

on second thought, I think it would be nice to be able to configure this interval, depending on the application also a reduced frequency could be used, e.g. 5Hz or 2Hz to further reduce CPU impact

Copy link
Author

@tonynajjar tonynajjar Mar 23, 2023

Choose a reason for hiding this comment

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

at 10 Hz the CPU usage was not more than 10%. But I can surely still add it if you see it necessary

tonynajjar pushed a commit that referenced this pull request Jun 27, 2023
Revert "Merge pull request #20 from logivations/collision-monitor-add…
5420aa4 
…-publish-action"

This reverts commit 8996e15, reversing
changes made to 617f60a.
tonynajjar pushed a commit that referenced this pull request Jul 20, 2023
Revert "Merge pull request #20 from logivations/collision-monitor-add…
0c9a5bf 
…-publish-action"

This reverts commit 8996e15, reversing
changes made to 617f60a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jplapp jplapp jplapp approved these changes

@andriimaistruk andriimaistruk Awaiting requested review from andriimaistruk

@saraalarab1 saraalarab1 Awaiting requested review from saraalarab1

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

@tonynajjar @jplapp