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

Filtering out unexecutable tasks from the routine. #193

Merged
merged 3 commits into from
Apr 27, 2015

Conversation

hawesie
Copy link
Member

@hawesie hawesie commented Apr 22, 2015

This has become necessary since the abilty to add daily tasks allows the addition of arbitrary tasks which are no longer bounded sensibly in time by the routine windows.

Also this now deals with runaway and non-terminating child processes.

HOWEVER this needs a different fork of SMACH

Either

  1. an update to SMACH if/when this PR is in: Deal with prior preemption and runaway child states. ros/executive_smach#37

Or

  1. https://github.com/hawesie/executive_smach

So 2) as of April 23, 2015.

hawesie added 2 commits April 22, 2015 21:21
This has become necessary since the abilty to add daily tasks allows the addition of arbitrary tasks which are no longer bounded sensibly in time by the routine windows.
…to-terminate execution processes (either tasks or navigation).
@bfalacerda
Copy link
Contributor

this seeks to work nicely. what do we do with it, since it depends on unreleased software?

@hawesie
Copy link
Member Author

hawesie commented Apr 26, 2015

This PR is definitely necessary for both deployments (so cc @marc-hanheide). This means we will need a checkout of this repo in our ws, or otherwise self-release smach (bad idea I guess).

@marc-hanheide
Copy link
Member

Can you explain this? I don't know how this effects us as we are not using any routine stuff in aaf, AFAIK.

@hawesie
Copy link
Member Author

hawesie commented Apr 26, 2015

It's actually the hawesie@475d48a commit you need, but it got added to an existing PR.

Before this the executor didn't actually have a way of moving past tasks that have genuinely got locked up. A timeout would be triggered, but that timeout wouldn't do anything under a number of conditions as it somewhere it would be waiting for a thread join that will never happen. Now that join times out too.

In addition to that, if a demanded task tried to kill a previous task, there is a timing issue that could mean the preemption is not handled cleanly. This resulted in the executor getting into an ugly mess. This latter issue is also fixed by that commit.

This commit requires changes to SMACH which are in the PR linked above.

@bfalacerda
Copy link
Contributor

The SMACH changes are also important for monitored_navigation, which, in rare situations, could get stuck in a state that would require it to be killed

@marc-hanheide
Copy link
Member

So, that means we should release our own SMACH, right? So, @hawesie do you want to transfer ownership of your repo to strands-project so I can set up Debian builds for this and we release it? Waiting for ROS to do it seems unfeasible.
We'd have to bump the version number to make sure our packages use our version rather than the official one...

@marc-hanheide
Copy link
Member

@hawesie I can of course also fork your repo into strands-project to release from there.

@marc-hanheide
Copy link
Member

I now set up the strands-project repo for Jenkins builds based on @hawesie s code. We can then release from there.

@marc-hanheide
Copy link
Member

As our own smach is now released...

retest this please

@marc-hanheide
Copy link
Member

retest this please

@marc-hanheide
Copy link
Member

Now fine to merge.

marc-hanheide added a commit that referenced this pull request Apr 27, 2015
Filtering out unexecutable tasks from the routine.
@marc-hanheide marc-hanheide merged commit 0a06fa7 into strands-project:hydro-release Apr 27, 2015
@hawesie
Copy link
Member Author

hawesie commented Apr 27, 2015

Thanks @marc-hanheide. I will let you know if and when things get merged upstream.

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