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

fixed ctrl+C on a concurrence container #35

Closed
wants to merge 7 commits into from
Closed

fixed ctrl+C on a concurrence container #35

wants to merge 7 commits into from

Conversation

athackst
Copy link

@athackst athackst commented Mar 18, 2015

This change is Reviewable

@jbohren
Copy link
Member

jbohren commented Mar 18, 2015

As this PR stands, it has one new busy loop that will easily soak up 100% cpu while the concurrence is executing, and another one that will do the same while it's waiting for the others to terminate. I agree that the issue is with this line but there's nothing significant about replacing the Condition with an Event. You could also just replicate something like this loop on that line, but that would also be sub-optimal

The right way to fix this is to have every Concurrence register its _done_cond or _done_event with whatever function is handling the SIGINT. That way there aren't even idle loops. Alternatively, you can make sure each state is keeping track of the global smach.is_shutdown() and break if that gets set.

@athackst
Copy link
Author

So what function is handling the SIGINT for a concurrence?

@jbohren
Copy link
Member

jbohren commented Mar 18, 2015

In rospy, a SIGINT handler gets registered. What smach should do is provide a global function like smach.register_shutdown_handler(), which would add a given callback to a list of handlers. It shold also provide a global function like smach.handle_shutdown() which would call all of the registered handlers. Then, what smach_ros should do on initialization is call rospy.on_shutdown(smach.handle_shutdown).

@jbohren
Copy link
Member

jbohren commented Mar 18, 2015

For non-ros use, you can register your own SIGINT handler and set it to call smach.handle_shutdown().

@jbohren
Copy link
Member

jbohren commented Mar 18, 2015

(: This would be a nice improvement and could get rid of any other loops that exist simply to check smach.is_shutdown()

@athackst
Copy link
Author

I agree that sounds better. However I can't seem to get it to work leaving in the that line. It seems like it's blocking the main thread? Nor does simply checking for smach.is_shutdown() in the state seem to break execution with it in.

@athackst
Copy link
Author

It was blocking the main thread, which ros uses to update signals like sigint and callbacks. I added a starting function which will spin up a state in another thread and handlers for shutdown. To start so it doesn't block you can just do smach_ros.start(sm), where sm is the state you want tot start.

@athackst
Copy link
Author

I also removed the smach.is_shutdown loop in concurrency since it didn't seem necessary any more. All states should now be preempting themselves by default on shutdown and therefore terminate, but I can add it back in if you think it breaks old code.

@athackst
Copy link
Author

We decided not to go with smach for our use case, so I'm closing this.

@athackst athackst closed this May 11, 2016
@jbohren
Copy link
Member

jbohren commented May 11, 2016

I'd still like to merge this in with some testing on my end. So I'm going to leave it open, feel free to unsubscribe if you want, though.

@130s
Copy link
Member

130s commented Jun 8, 2017

Closing in favor of #49

@130s 130s closed this Jun 8, 2017
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.

3 participants