coordination: replace Roby::Variable with real values

[D-Alex]

I am not sure if this is the right spot and I also do not like to dup the hole object only to not overwrite the Variable for the next call.

This allows to use with_arguments in code blocks given to action_script
or action_state_machine.

describe('...').optional_arg('speed','speed in m/s',0.1)
action_state_machine('move') do
    Compositions::Control::MoveForward.use(odometry_def,control_def).with_arguments(:speed=> speed)
    ...
end

[doudou]

This is wrong at many levels (look at the failed test suite just above this comment)

First of all, even if it would be an OK change, it is also a purely syskit-specific change in Roby. The only constraint on the instanciation objects is that they either answer to #as_plan or #instanciate. Nothing about #arguments.

In the current code, if you want complex argument-based parametrization, create either separate actions or separate definitions for each of your states and then use those in the state machine. It integrates much better in the syskit workflow, as tests can then be automatically run on the definitions, verifying that they are at least (but not only) deployable.

Moreover, it allows you to start each definition separately (in e.g. the syskit IDE), which is great for "human-driven" testing on the real system -- something that unfortunately syskit did not (yet !) remove the need for.

If you truly truly believe that this is a better way (and make a good case for it), the best way forward would be to make sure that Syskit::InstanceRequirement objects are interpreted as actions by the Roby coordination code instead of as #as_plan or #instanciate receivers (calling InstanceRequirement#to_action_model), Actions can have their top-level arguments overriden.

In general, it would obviously be best to ensure that variables can't be used for instance requirements .... but that's another story (and I don't have a solution for it out of the top of my head)

[D-Alex]

What I want are roby shell commands for controlling the robot.

• move!(:speed => 0.1,:distance => 1)
• rotate!(....)
• blink!
• dock!(:speed => 0.2) # start docking

Doing this with roby is simple (see http://rock-robotics.org/stable/api/tools/roby/tutorial/shell.html) but this does not play nice with Compositions.
Doing this with action script is a bit of a pain because you cannot use arguments at the moment.

[D-Alex]

Ok the missing piece was that you can set the parameters from the embedded composition when you call the profile xxx_def. This is not documented and the only example uses with_arguments
http://rock-robotics.org/stable/documentation/system_management_tutorials/1100_action_state_machines.html

Therefore, I agree that you do not need it. Why not simple raising in the with_argument from syskit method if someone tries to passes a Roby::Coordination::Models::Variable and telling him he should use profiles?

https://github.com/rock-core/tools-syskit/blob/master/lib/syskit/instance_requirements.rb#L552

[doudou]

Therefore, I agree that you do not need it. Why not simple raising in the with_argument from syskit method if someone tries to passes a Roby::Coordination::Models::Variable and telling him he should use profiles?

Because that would be an extremely specific check, which I don't like. Arguments can be anything, really, and I don't want to embed in InstanceRequirements something that is specific to coordination.

One thing that could be done right away is to make Variable a delayed argument, that raises when evaluated. One would at least get an error, at least at runtime, in Syskit at deployment time. In the long run, we'll add automated offline testing for action scripts and action state machines. That would be something that would be validated there.

[D-Alex]

You get already an error during runtime because Models::Variable are usually not compatible with the poll blocks or other usage. Therefore, we could also simply put it into a FAQ until the automated offline testings is implemented.

[doudou]

You get already an error during runtime because Models::Variable are usually not compatible with the poll blocks or other usage

At least, with the delayed argument, it would happen before the task gets started or in the case of syskit, while the new network is computed (i.e. before it is applied). Not better in your opinion ?

[D-Alex]

Sounds better of course if the effort is not too big.

[doudou]

I don't have time to do it right now, so created #47 to keep it in mind.