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

Master introduce v1cmd syntax #84

Open
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@psoetens
Copy link
Member

commented Jan 21, 2015

This adds the RTT v1 command semantics to operations used in RTT scripts, by introducing a .cmd() method, analog to .send() and .call(). An operation invoked with .cmd() will wait in a non-blocking way for the operation to complete. So this can mainly be used in periodically executed components, which call operations which take a long time to execute in non-periodic components.

To be discussed, but not to be merged into toolchain-2.8

@meyerj meyerj added this to the 2.9 milestone Oct 27, 2015

@psoetens psoetens force-pushed the psoetens:master-introduce-v1cmd-syntax branch from 1e04fb4 to a6ced57 Nov 19, 2015

psoetens and others added some commits Oct 24, 2014

scripting: add support for .cmd() semantics in scripting
This script introduces two changes to scripting:

1. It rearranges the ActionInterface::readArguments/execute/reset semantics
when mapped to DataSource::evaluate/rvalue/reset functions. This had some
influence on the flow of executing (remote) operations, but all cases have been
rectified.

2. It adds to the parser a look for '.cmd' suffix on operation calls,
 which is similar to '.send', but yields the script execution until
the send returns something else than SendNotReady. The result of a .cmd
call is a SendStatus which can be read/used by the script itself.

This has been tested on C++ operations and exported script functions.
'cmd' called operations will not update their arguments when SendSuccess is
returned and the return value of the operation is ignored.

Signed-off-by: Peter Soetens <peter@thesourceworks.com>
operations: add const qualifier in collect family of functions
There was no need to have this non-const. non-const prevented us
to use it on a const& to a SendHandle, which we do need if the
SendHandle is presented by a DataSource.

Signed-off-by: Peter Soetens <peter@thesourceworks.com>
Fixed the .cmd() syntax to also work in state machine scripts.
Since a condition edge now waits on the result of a collect
datasource, that datasource needs to implement copy/clone
semantics properly, such that when copying the state machine,
we keep referencing the correct datasource.

This patch also adds support for var SendStatus ss = foo.cmd()
in which case, the initialisation of the var is delayed until
the cmd result is known (identical behavior to:
var SendStatus ss;
ss = foo.cmd();
)

Signed-off-by: Peter Soetens <peter@thesourceworks.com>
Introduce .cmd() and .send() syntax for exported script functions
This patch adds .cmd() and .send()+collectIfDone() syntax for an exported script function
just like for a C++ operation. The patch consists of 3 parts:

1. Cleanup in CmdFunction and CallFunction : removed unused variables
2. Adding logic into the ExpressionParser to detect the case of script functions.
   Since they don't have true SendHandle objects and use SendStatus datasources,
   the parser needs to re-arrange some data sources when handling a 'SendHandleAlias'.
   The SendHandle of a script function is effectively the alias to the .send() function,
   + that a .reset() is not forwarded. Evaluating a SendHandle of a script function is
   the same as writing sh.collectIfDone().
3. Added unit tests in state_test to test all these cases.

I also removed a deprecated warning in the ScriptngService

Signed-off-by: Peter Soetens <peter@thesourceworks.com>
scripting: fixed segfault in CmdFunction::reset() if the engine is al…
…ready gone

This is related to 00b2679 in CallFunction.hpp.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
scripting: fix .send() bug when using a SendHandle for exported scrip…
…t functions

We noticed that a send() on a script function did not work when
the return value was stored in a SendHandle. The reason was that the
SendHandleAlias was stored as an Attribute, but can't be instantiated
(since its an alias to a FusedFunctorDataSource), hence, it copied
all arguments, prior to giving the arguments a chance to instantiate.
so this send() then used copies that weren't used later on...
We now remove the SendHandleAlias from the StateMachineService such
that they are removed before instantiation happens. We only need them
during parsing as attributes, not during execution.

Signed-off-by: Peter Soetens <peter@thesourceworks.com>
scripting: fixed the operation.call() syntax
Without this patch calling operation task.foo with .call() ended up with a
```
Service or Task "task" has no Peer or Service foo (or task was not found at all).
```
error message.

I also eliminated some compiler warnings due to signed/unsigned comparisons.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
scripting: fix .cmd() in while loops or state programs
A wrapper did forcefully not forward the reset(). It seems this is no longer
required, since removing that code makes .cmd() invocations work correctly
again.

Signed-off-by: Peter Soetens <peter@intermodalics.eu>

@psoetens psoetens force-pushed the psoetens:master-introduce-v1cmd-syntax branch from a6ced57 to edff4c6 Nov 19, 2015

@meyerj

This comment has been minimized.

Copy link
Member

commented Dec 21, 2015

merged into toolchain-2.9: 2af3745

@meyerj

This comment has been minimized.

Copy link
Member

commented Dec 21, 2015

Commit faf49ed breaks the state_test:

19.181 [ Info   ][Thread] Creating Thread for scheduler=ORO_SCHED_OTHER, priority=1, CPU affinity=0, with name='root'
19.181 [ Info   ][root] Thread created with scheduler type 'ORO_SCHED_OTHER', priority 0, cpu affinity 255 and period 0 (PID= 11112 ).
19.181 [ Info   ][Thread] Creating Thread for scheduler=ORO_SCHED_OTHER, priority=1, CPU affinity=0, with name='caller'
19.181 [ Info   ][caller] Thread created with scheduler type 'ORO_SCHED_OTHER', priority 0, cpu affinity 255 and period 0 (PID= 11113 ).
19.182 [ Info   ][ProgramLoader::loadProgram] Parsing file func.ops
19.188 [ Info   ][ProgramLoader::loadProgram] Exported Function 'foo' added to task 'root'
19.188 [ Info   ][ProgramLoader::loadProgram] func.ops : Successfully parsed.
19.188 [ Info   ][ScriptingService::loadStateMachine] Parsing file state_test.cpp
state_test: /opt/orocos/indigo/src/orocos_toolchain/rtt/rtt/scripting/SendHandleAlias.cpp:75: virtual RTT::scripting::SendHandleAlias* RTT::scripting::SendHandleAlias::copy(std::map<const RTT::base::DataSo
urceBase*, RTT::base::DataSourceBase*>&, bool): Assertion `(inst == false) && "SendHandleAlias may not be instantiated !"' failed.
unknown location(0): fatal error in "testStateSendFunction": signal: SIGABRT (application abort requested)
/opt/orocos/indigo/src/orocos_toolchain/rtt/tests/state_test.cpp(1949): last checkpoint
scripting: fixed SendHandleAlias copying bug during state machine ins…
…tantiation

The loop that removed SendHandleAlias variables from the StateMachineService could have skipped the
second one for the case the first two variables are SendHandles.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
@meyerj

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

Commit faf49ed breaks the state_test:

Fixed in psoetens@d444550.

@meyerj

This comment has been minimized.

Copy link
Member

commented Feb 10, 2016

The state_test is still broken:

/opt/orocos/indigo/src/orocos_toolchain/rtt/tests/state_test.cpp(949): error in "testStateSendFunction": check sa->getStateMachine( "x" )->inState("FINI") failed

The failing test case testStateSendFunction runs the following state machine:

export function foo(int arg) {
  do test.assert( tvar_i == arg ) 
  do test.assert( tvar_i != tconst_i ) 
  set tvar_i = tvar_i+2
  do test.assert( tvar_i == arg + 2 ) 
}

StateMachine X {
 initial state INIT {
  run {
   tvar_i = 0

   var SendHandle sh, sh2
   sh = foo.send(tvar_i) 

   sh // tests accidental sh evaluation

   test.assert( sh.collectIfDone() == SendNotReady )
   test.assertEqual( tvar_i, 0 )

   while ( sh.collectIfDone() == SendNotReady) 
      yield
   test.assert( sh.collectIfDone() == SendSuccess )
   test.assertEqual( tvar_i, 2 )

   sh2 = foo.send(tvar_i) 
   test.assert( sh2.collectIfDone() == SendNotReady )
   test.assertEqual( tvar_i, 2 )
   while ( sh2.collectIfDone() == SendNotReady) 
      yield
   test.assert( sh2.collectIfDone() == SendSuccess )
   test.assertEqual( tvar_i , 4 )
  }
  transitions {
     select FINI
  }
 }
 final state FINI { // Failure state.
  entry { do test.assert(true); }
 }
}

The problem is that the condition of the first while loop in the run program never evaluates to false and that every evaluation of sh.collectIfDone() resends the foo operation, which is executed in every yield cycle. The run program never finishes and therefore the final state test fails.

meyerj added some commits Feb 10, 2016

scripting: Fixed collecting from a SendHandle returned from sending a…
…n exported function

With patch edff4c6, resetting the collect data source returned by
FunctionFactory::produceCollect() requeues the function and keeps sending it for every evaluation
of sh.collectIfDone() in a loop. Commit f82fa9a reverts this faulty
solution.

Instead, to make sure that an evaluation of .cmd() also requeues the function, the data source returned
by the ExpressionParser is an ActionAliasDataSource which correctly combines the send and collect cycle.

Some additional minor patches were required to fix the correct behavior of this solution:

* The FusedMSendDataSource stored as the first argument inside the FusedMCollectDataSource must be an
  AssignableDataSource, as otherwise every evaluation of the collect data source also re-evaluates the
  send data source from RTT::internal::GetArgument<Seq, RTT::SendHandle<Signature> >::operator().
* When copying the data sources, every instance of FusedMSendDataSource must only be cloned once so that
  the FusedMCollectDataSource that is evaluated by the ActionAliasDataSource collects from the same
  SendHandle that was used to send the command.
* FusedMCollectDataSource::reset() must not forward to args[0]->reset(), the FusedMSendDataSource.
* Evaluating a CmdFunction instance should not reset the cached SendStatus to SendNotReady if the
  function was already queued.
* The default SendStatus returned by CmdFunction::rvalue() was changed from SendNotReady to
  SendFailure, in analogy to FusedMCollectDataSource.
* Use create_sequence::assignable() in OperationInterfacePartFused::produceCollect(), as all
  arguments in a collect call must be assignable, including the SendHandle data source (see first
  bullet item).

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
@meyerj

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

The state_test is still broken:

The last two commits (f82fa9a and 361fe29) seem to fix the issue.

@meyerj

This comment has been minimized.

Copy link
Member

commented Sep 6, 2016

I am not sure if this is the expected behavior for the send() call type of a function:

Deployer [S]> scripting.eval("void test() {}")
 = true                
Deployer [S]> var SendHandle sh = test.send()
Semantic error: Attempt to initialize a var SendHandle with a SendStatus.
Deployer [S]> sh
 = (SendHandle)        
Deployer [S]> var SendHandle sh2
 = (SendHandle)        

Deployer [S]> sh2 = test.send()
 = SendNotReady        

Deployer [S]> 

Why can the result of test.send() not be assigned to a new SendHandle variable sh? Why does the second assignment to sh2 does not give the same error?

Should FunctionFactory::produceSend() not always return a SendHandle data source here? But FunctionFactory::produceHelper() returns a CmdFunction which is a SendStatus data source in FunctionFactory.cpp:230. I think the correct solution would be to add a new method produceCommand() to the OperationInterfacePart base class and implement it for functions. produceSend() could be left unimplemented as before or requires another solution.

Did the last commit 361fe29 introduce this bug?

meyerj added some commits Oct 20, 2017

tests: add a test case to program_test for send and collect of script…
…ing functions

Test cases testProgramCallFoo and testProgramDoFoo were exactly the same and only
covered calling functions from programs. This commit replaces the latter by a new
test case testProgramSendFoo, which would also reveal the bug that caused a
function to be resent on each evaluation of sh.collectIfDone(). Before, only a
similar test case in state_test was triggering that bug.

The resending collect problem has been fixed in
361fe29.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
tests: fixed execution thread spec in fixture for the state_test
The operation "vo0" added for the testStateYieldbyCmd check in state_test.cpp was not
an OwnThread operation and hence executed by the GlobalEngine instead of the task that
is also running the state machine. This patch does not change the result of the test:
.cmd() correctly yields and allows the engine to execute the operation before checking
the result in the next cycle.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

meyerj added some commits Jun 7, 2016

scripting: fixed memory leak during destruction of CallFunction and C…
…mdFunction instances

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
Add license header to rtt/scripting/CmdFunction.hpp
... as generated by tools/scripts/verify_headers.sh.
@meyerj

This comment has been minimized.

Copy link
Member

commented May 8, 2019

I am tempted to at least partially revert this patch in the toolchain-2.10 branch for the upcoming release. It still has unresolved issues (#84 (comment)) and introduces new complexity.

The .cmd() syntax only restores a small part of RTT v1 Commands and applies it to RTT v2 operations, namely that they can be called from Orocos scripting in a way that is equivalent to send + yield in a loop:

// instead of:
operation.cmd(1.0, "string argument");

// ... you can write:
var SendHandle sh = operation.send(1.0, "string argument");
while(sh.collectIfDone() == SendNotReady) {
  yield;
}

(which additionally allows to return the results as reference arguments to collectIfDone()). I am not sure from the documentation whether invoking a command from scripting was actually blocking in v1 or not. At least it was not when invoking a command from the OCL TaskBrowser.

The implementation side of RTT v1 commands was not restored in this PR. RTT v1 commands are stateful task primitives that allow the user to pass a non-blocking start callback and a completion condition to the RTT::Command constructor. The actual command execution was then completely up to the user and could be done in either the command function itself (blocking), other primitives based on the internal state, e.g. in the updateHook() or inside an Orocos state machine. In RTT v2 operations implemented as C++ functions always execute at once and there is no way to start an operation, yield and continue later. Once the C++ function returns, the operation is done and the results are returned to the caller.

Did I miss something? Any more thoughts? @psoetens? Is someone already using this new feature?

@snrkiwi

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I don't believe that the original intent was to fully implement the v1 Command syntax and implementation, but to give a useful approximation of it. But given the complexity I don't doubt that there are outstanding issues ...

Yes, we use this extensively to, for example, have state machines "block" on a long call while still checking (and potentially executing) transitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.