Skip to content
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 Example for Depalletizing Using MoveIt Task Constructor #184

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

PetoAdam
Copy link
Member

This Pull Request introduces a demonstration package for depalletizing tasks. The package leverages the MoveIt Task Constructor framework and is designed for use with KUKA robots.

@PetoAdam
Copy link
Member Author

Once the PR is merged, I will add the necessary documentation to the wiki.

Copy link

sonarcloud bot commented Sep 12, 2024

auto mtc_depalletizing_task_node = std::make_shared<MTCDepalletizingTaskNode>(options);
rclcpp::executors::MultiThreadedExecutor executor;

auto spin_thread = std::make_unique<std::thread>(
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason for making this thread a unique ptr instead of simple var?

Copy link
Member Author

@PetoAdam PetoAdam Sep 17, 2024

Choose a reason for hiding this comment

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

I sometimes use pointers for threading (mostly when they might throw an exception).
Advantages:
Automatic Cleanup: std::unique_ptr ensures the thread is cleaned up when it goes out of scope, preventing resource leaks.
Exception Safety: Ensures the thread’s destructor is called even if an exception is thrown, joining the thread if necessary.
Clear Ownership: Indicates single ownership, making the code easier to understand and maintain.

Disadvantages:
Overhead: May introduce unnecessary overhead in terms of performance and complexity.
Move Semantics: std::thread is already move-only, and wrapping it in a smart pointer can complicate the design.
Readability: Can reduce code readability and clarity by adding another layer of abstraction

Yes, I know it's a simple demo application and std::thread is pretty robust itself, I can change it to a simple var if you wish. :)

Copy link
Member

Choose a reason for hiding this comment

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

Automatic Cleanup: std::unique_ptr ensures the thread is cleaned up when it goes out of scope, preventing resource leaks.
Exception Safety: Ensures the thread’s destructor is called even if an exception is thrown, joining the thread if necessary.

are these not only true, if you implement a custom deleter for std::thread?

Copy link
Member Author

Choose a reason for hiding this comment

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

the custom deleter is a better approach, as it can gracefully stop a thread, while in the case of a smart pointer, if the pointer runs out of scope, the thread's destructor is called, which terminates the thread abruptly. That's why I manually joined the thread in the example. Having a custom deleter would be more ideal, I just did not think this simple example would need it.

Copy link
Member

Choose a reason for hiding this comment

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

But compared to the simple std::thread, what is the difference without the custom deleter? the simple var would also run out of scope and the destructor would be called. So I do not see why the unique ptr is better in this case, as without the deleter it will terminate the same way (also in case of an exception, where join() would not be called automatically)

(I do not necessarily want to change to simple var, just want to understand the effects of both and without a custom deleter I think your first two advantages are not valid, as the behaviours of both cases are identical)

Copy link
Member Author

Choose a reason for hiding this comment

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

I read a bit about how exactly it works. Yes, you are right, the only reason one might want to use a smart pointer for storing a thread is for ownership purposes. It does not really apply for our case, so changing it back to a simple var makes a lot of sense.

@Svastits
Copy link
Member

I do not think we can merge this to master currently, as mtc is not released and would therefore block our release, as the release process does not allow upstream repos. Maybe merge to rolling, but we should discuss this :)

@PetoAdam
Copy link
Member Author

Yeah, I hope MTC will be available through apt soon(tm). We can discuss what to do tomorrow. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants