http://bugs.orocos.org/show_bug.cgi?id=802

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #9 from Peter Soetens <peter [..] ...> 2011-02-24 23:21:36 CET ---
(In reply to comment #8)
> (In reply to comment #6)
> > I would accept all but the first one with minor 'hesitation'. When looking at
> > the API documentation (
> >
> http://people.mech.kuleuven.be/~orocos/pub/stable/documentation/rtt/v2.2...
> > ) your additions seem like filling in the logical missing functions. When in
> > doubt, I would always consider usability more important than 'the perfect
> > design'. On the other hand, If everyone had been taught to use
> > task->provides(), we wouldn't have had this problem in the first place.
>
> I give you a new set of patches where the hasOperation is in a separated commit.
> So you will be free to not include this one.
>
> >
> > Another note: I think the __attribute__((obsolete)) is gcc specific, so you'd
> > better define it as a macro in the rtt-config.h.in file. (RTT_API_OBSOLETE).
>
> In this new set of patches, i introduce the RTT_API_DEPRECATED.
>
> It have reorganised the commits so it should be clean.

Thanks for this nice work. Why didn't you deprecate the original functions in
your 'local' operations patch ? These functions live in the RTT namespace, so
we must have a commitment to not break the public API unless we absolutely must
(because it's broken).

A thing I'm still unhappy about is the getNames() function in the
OperationInterface.hpp. It should be deprecated and replaced by
'getOperationNames()', analogous to getAttributeNames() and getPortNames().
The misfortune here is that you just renamed an existing getOperationNames() to
getLocalOperationNames(), which would cause confusion, if anyone was using that
function (now it will just break their build ).

I think we need to go for the deprecation and can wait for
getNames()->getOperationNames() until 2.4, but it's the kind of API change I
wish we never should make.

Peter

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #8 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-22 18:21:09 CET ---
(In reply to comment #6)
> I would accept all but the first one with minor 'hesitation'. When looking at
> the API documentation (
> http://people.mech.kuleuven.be/~orocos/pub/stable/documentation/rtt/v2.2...
> ) your additions seem like filling in the logical missing functions. When in
> doubt, I would always consider usability more important than 'the perfect
> design'. On the other hand, If everyone had been taught to use
> task->provides(), we wouldn't have had this problem in the first place.

I give you a new set of patches where the hasOperation is in a separated
commit. So you will be free to not include this one.

>
> Another note: I think the __attribute__((obsolete)) is gcc specific, so you'd
> better define it as a macro in the rtt-config.h.in file. (RTT_API_OBSOLETE).

In this new set of patches, i introduce the RTT_API_DEPRECATED.

It have reorganised the commits so it should be clean.

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #7 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-22 18:19:45 CET ---
Created attachment 637
--> http://bugs.orocos.org/attachment.cgi?id=637
A new set of patches for this bug.

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #6 from Peter Soetens <peter [..] ...> 2011-02-22 14:36:06 CET ---
(In reply to comment #2)
> Thanks to your previous message, I think i understand the local and remote
> operation distinction.
>
> But i still have a question about that :
>
> > peter wrote:
> >
> > To first set a misunderstanding straight, If you want to check for a peer's property/operations etc, you need to call:
> >
> > peer->provides()->hasProperty("foo")
> > peer->provides()->hasOperation("bar")
> >
> > provides() returns the Service Object you want to query. Since a component can have multiple services, a global 'TaskContext::hasProperty' function is not meaningful, since it would only be able to query the 'top level'/'this' service of the component, and not any subservice. While with provides, you would write:
> >
> > peer->provides("scripting")->hasOperation("runScript")
> >
> > So I'm not eager to add these functions to TaskContext.hpp.
>
> I decided to not use sub-services for my components. So I always add an
> operation "directly" to the component with this->addOperation( "reset",
> &MyTask::reset, this, OwnThread).doc("Reset the system."); for example. So when
> i want to use this operation in an other component i do
> OperationCaller<void(void)> my_reset_meth = a_task_ptr->getOperation("reset");.
> So i would like to have a hasOperation() in Taskcontext or remove the
> getOperation() and always use provides() for querying services.
>
> In your previous mail i understand the modification you wish for
> OperationInterface and Service. But i don't understand what do you want for
> TaskContext ?
>
> Here are some patches. Could you tell me if it is on the good way please ?

I would accept all but the first one with minor 'hesitation'. When looking at
the API documentation (
http://people.mech.kuleuven.be/~orocos/pub/stable/documentation/rtt/v2.2...
) your additions seem like filling in the logical missing functions. When in
doubt, I would always consider usability more important than 'the perfect
design'. On the other hand, If everyone had been taught to use
task->provides(), we wouldn't have had this problem in the first place.

Another note: I think the __attribute__((obsolete)) is gcc specific, so you'd
better define it as a macro in the rtt-config.h.in file. (RTT_API_OBSOLETE).

Peter

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #5 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-21 09:31:15 CET ---
Created attachment 634
--> http://bugs.orocos.org/attachment.cgi?id=634
Make the distinction between local and remote services

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #4 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-21 09:30:45 CET ---
Created attachment 633
--> http://bugs.orocos.org/attachment.cgi?id=633
Rename hasMember to hasPart.

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #3 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-21 09:30:14 CET ---
Created attachment 632
--> http://bugs.orocos.org/attachment.cgi?id=632
Add the has* interface to the TaskContext

http://bugs.orocos.org/show_bug.cgi?id=802

--- Comment #2 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-21 09:29:00 CET ---
Thanks to your previous message, I think i understand the local and remote
operation distinction.

But i still have a question about that :

> peter wrote:
>
> To first set a misunderstanding straight, If you want to check for a peer's property/operations etc, you need to call:
>
> peer->provides()->hasProperty("foo")
> peer->provides()->hasOperation("bar")
>
> provides() returns the Service Object you want to query. Since a component can have multiple services, a global 'TaskContext::hasProperty' function is not meaningful, since it would only be able to query the 'top level'/'this' service of the component, and not any subservice. While with provides, you would write:
>
> peer->provides("scripting")->hasOperation("runScript")
>
> So I'm not eager to add these functions to TaskContext.hpp.

I decided to not use sub-services for my components. So I always add an
operation "directly" to the component with this->addOperation( "reset",
&MyTask::reset, this, OwnThread).doc("Reset the system."); for example. So when
i want to use this operation in an other component i do
OperationCaller<void(void)> my_reset_meth = a_task_ptr->getOperation("reset");.
So i would like to have a hasOperation() in Taskcontext or remove the
getOperation() and always use provides() for querying services.

In your previous mail i understand the modification you wish for
OperationInterface and Service. But i don't understand what do you want for
TaskContext ?

Here are some patches. Could you tell me if it is on the good way please ?

http://bugs.orocos.org/show_bug.cgi?id=802

Chavent Paul <paul [dot] chavent [..] ...> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |paul [dot] chavent [..] ...

--- Comment #1 from Chavent Paul <paul [dot] chavent [..] ...> 2011-02-21 09:24:42 CET ---
On Wednesday 26 January 2011 10:46:56 paulchavent@ wrote:
> Hi RTT-devs.
>
> I'am trying to make a patch for the bug '802 : Test if a peer has an
> operation/property'.

To first set a misunderstanding straight, If you want to check for a peer's
property/operations etc, you need to call:

peer->provides()->hasProperty("foo")
peer->provides()->hasOperation("bar")

provides() returns the Service Object you want to query. Since a component can
have multiple services, a global 'TaskContext::hasProperty' function is not
meaningful, since it would only be able to query the 'top level'/'this'
service of the component, and not any subservice. While with provides, you
would write:

peer->provides("scripting")->hasOperation("runScript")

So I'm not eager to add these functions to TaskContext.hpp.

>
> I submit you a first attempt. For ports, attributes and properties, it
> should be ok. But for operations it's not definitive.
>
> I need your help because i don't understand the Service + Operation
> philosophy.
>
> I add an hasOperation method to the TaskContext class. But i can't
> implement it with (Service::getOperation != 0) because the behavior should
> be different (mustn't display the message). Moreover i can't implement a
> Service::hasOperation because it already exists.
>
> The problem is that the Service seems to have two kind of operations. The
> Service's methods hasOperation and getOperationNames for example work on
> its simpleoperation member, and, getOperation for example work on its
> OperationInterface base class.

Correct. We keep track of both C++ operations and of 'remote' operations with
the same object (Service). When a local C++ operation is added, it appears in
both lists, if a remote operation is added (or a script function for example)
it only apears in the 'remote' list.

>
> In hasOperation and getOperationNames, the calls to OperationInterface
> (hasMembers and getNames) are commented out. So i would like to have a bit
> of history on what happened here : - should the getOperation method take
> care of simpleoperation ?
> - should the hasOperation method take care of OperationInterface ?
> - isn't it possible to have only one type of operation ?
> I found the current implementation very complicated.

It's because you're touching the first layers of the 'middleware' aspect in
RTT. At some point, we distinguish between local operations and remote ones.
hasOperation() only returns true if the operation is local in this component
or in this process. Meaning if you call it on a peer component, it might
return false always (because the peer is in another process for example).
hasMember() checks for the operation across processes.

Now I think myself this makes no sense to Joe user. So what I propose is this:

In OperationInterface.hpp:
-> leave hasMember for what it is, but deprecate it in the doxygen docs in
favour of hasPart():
-> add hasPart to OperationInterface.hpp which does exactly the same as
hasMember()
In Service.hpp:
-> rename hasOperation() to hasLocalOperation(). You'll need to rename them
too in ServiceRequester.cpp, Service.cpp and StateGraphParser.cpp
-> add a new function hasOperation() that returns hasPart(). Rationale: if
hasOperation() returns true, every OperationCaller object will be able to call
it (it will be a local or remote operation).

The idea is then that every mortal user uses hasOperation() and only framework
developers use hasLocalOperation() or hasPart() when they need to know if an
operation is local or remote.

Peter