Skip to content

Commit

Permalink
Revert "interface.register_action: use Roby::Task as default return t…
Browse files Browse the repository at this point in the history 
…ype"

This breaks actions defined via action_state_machine which no longer start when called
from the ruby shell. Probably because Roby::Task is just a MetaClass.

This reverts commit 7ec9de2.
  • Loading branch information
Alexander Duda authored and Alexander Duda committed Dec 3, 2015
1 parent 7ec9de2 commit 495fb8b
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions lib/roby/actions/models/interface.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ def register_action(name, action_model)
end
const_set task_model_name, task_model
task_model.permanent_model = self.permanent_model?
action_model.returns(task_model)
end
end

Expand Down

7 comments on commit 495fb8b

@D-Alex
Copy link
Member

@D-Alex D-Alex commented on 495fb8b Dec 3, 2015

Choose a reason for hiding this comment

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

This is problematic because at the moment http://rock-robotics.org/stable/api/tools/roby/tutorial/shell.html is no longer working. The reason is a wrong default type see 7ec9de2.

@doudou
Copy link
Member

@doudou doudou commented on 495fb8b Dec 7, 2015

Choose a reason for hiding this comment

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

I'm lost. Is the problem caused by the original commit or its revertion

@doudou
Copy link
Member

@doudou doudou commented on 495fb8b Dec 7, 2015

Choose a reason for hiding this comment

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

A bit of background ... then we can start looking for the best solution.

The reason why actions by default generate their own return type is so that instanciated actions in the plan actually give information on the intent (i.e. you get a Move task instead of a plain Roby::Task). It's actually not needed by Roby itself. I'm getting into the opinion that it's probably making more harm than good and would support just ripping it off.

Thoughts ?

@D-Alex
Copy link
Member

@D-Alex D-Alex commented on 495fb8b Dec 7, 2015

Choose a reason for hiding this comment

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

The problem was solved by the 7ec9de2 but this was introducing a new bug causing action_state_machines no longer to start when called from the ruby shell. Therefore, I reverted 7ec9de2. Ideally, any thing that is a Roby::Task should work if no return type is specified. And I really do like the information you are encoding with the new class.

One solution might be to accept any Roby::Task as ancestor for this kind of classes because they are not adding any functionality.

 task_model.has_ancestor?(MyTask) ==> true # if MyTask is a Roby::Task and task_model was auto generated

@doudou
Copy link
Member

@doudou doudou commented on 495fb8b Dec 7, 2015

Choose a reason for hiding this comment

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

this was introducing a new bug causing action_state_machines no longer to start when called from the ruby shell.

Any idea why not ?

And I really do like the information you are encoding with the new class. One solution might be to accept any Roby::Task as ancestor for this kind of classes because they are not adding any functionality.

The information is lost if you return anything else than the new class. And one can always set the return type to a specific Roby task explicitely.

@D-Alex
Copy link
Member

@D-Alex D-Alex commented on 495fb8b Dec 7, 2015

Choose a reason for hiding this comment

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

My explanation was that Roby::Task is missing something that is added by Roby::Task.new_submodel. I can have a look if you have no idea why this is happening.

@doudou
Copy link
Member

@doudou doudou commented on 495fb8b Dec 9, 2015

Choose a reason for hiding this comment

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

Ah ... action state machines ... right.

Roby::Task instances cannot run. It's an abstract task model. For "normal" actions, it is moot as one never returns a Roby::Task. For action state machines, the toplevel task is generated by the framework.

We could in principle validate that the toplevel task is not abstract .... but that's "generically speaking" wrong (one could creat an action that instanciates a state machine with an abstract root task and then replace it with a non-abstract one)

Please sign in to comment.