[ now really moving this to Orocos-dev]

On Monday 17 March 2008 18:02:43 Sylvain Joyeux wrote:
> OK. Here it is. Now, both data and buffer links work. My guts tell me
> that read-write connections still do not work though. At least I would
> not gamble a lot of money/the life of some student near to the robot on
> this ...

Other remarks about the patches:
* please use spaces as indentation, with a width of 4.
* The following function still has issues:

template
ConnectionInterface::shared_ptr
ReadDataPort::createConnection(PortInterface* other,
ConnectionTypes::ConnectionType con_type)
{
// If the other side is remote, we must create the connection
// ourselves. In that case, the initial value and buffer size are not
// used by the connection factory.
//
// This is a hack -- it needs a proper solution.
if (other->serverProtocol())
{
ConnectionFactory cf;
return ConnectionInterface::shared_ptr ( cf.createDataObject(other,
this, T(), con_type) );
}
else
other->createConnection(this, con_type);
}

Apart from the missing return statement, it will cause endless recursion when
two read ports are connected. I understand now that otherwise, it works:
other is a remote write port and will create the buffer correctly, while we
get a proxy in return. ok.

Nevertheless, PortInterface::connectTo() has this logic as well. It will first
ask the read port to create a connection.That will fail (in my
implementation) and then it will ask the write port to create a connection,
which will do like you want, resulting in the same behaviour. Maybe we can
adapt your DataFlowI.cpp code such that it uses PortInterface::connectTo(),
and then the code snippet above can go away.

BTW, do you mind if I commit your patches one by one, but set the indentation
right, or do you prefer to resubmit ?

Peter

On Monday 17 March 2008 18:02:43 Sylvain Joyeux wrote:
> OK. Here it is. Now, both data and buffer links work. My guts tell me
> that read-write connections still do not work though. At least I would
> not gamble a lot of money/the life of some student near to the robot on
> this ...

These are my remarks:

1. The corba layer has some wrong usages of the logger:
log()<< "..."< It should be
log(Error)<< "..." < The log level must be set always at the beginning. You copy/pasted this
misuse sometimes.

2. Due to the fix in PortInterface::connectTo, it's possible that your createConnection() for readers patch may be dropped. On the other hand,
it could be that the current code always creates a proxy, because the
connection factory is always involved.

3. I like the 'port mode', it allows more intelligent connection creation.

4. My unit test fails because the connection is created in the wrong direction: ts->connectPorts(ts2) creates the connection data at the side of
ts2, while the C++ code assumes ts creates the connection data object.
An inversion I overlooked in my design. If I loosen the test and don't check
initialisation direction, data ports work, buffer ports as well !
For some reason, the size() returns the wrong number though.

5. when building RTT with your patches, I get
/home/sspr/src/Orocos/trunk/rtt/src/DataPort.hpp: In member function ‘boost::intrusive_ptr RTT::ReadDataPort::createConnection(RTT::PortInterface*, RTT::ConnectionTypes::ConnectionType) [with T = double]’:
/home/sspr/src/Orocos/trunk/rtt/src/DataPort.hpp:487: let op: control reaches end of non-void function
/home/sspr/src/Orocos/trunk/rtt/src/BufferPort.hpp: In member function ‘boost::intrusive_ptr RTT::ReadBufferPort::createConnection(RTT::PortInterface*, RTT::ConnectionTypes::ConnectionType) [with T = double]’:
/home/sspr/src/Orocos/trunk/rtt/src/BufferPort.hpp:672: let op: control reaches end of non-void function

Probably you forgot a patch ?

All in all, a big improvement. Only the reader's createConnection() I would drop, as it has no initial value,
buffer size etc. set. The PortInterface::connectTo() code contains the code that detects reader-reader
connections and issues the warnings. The problem remains that the connectPorts takes the wrong
direction of data/buffer connection. This needs to be addressed as well, then the reader's createConnection
can be dropped.

Peter

On Monday 17 March 2008 18:02:43 Sylvain Joyeux wrote:
> OK. Here it is. Now, both data and buffer links work. My guts tell me
> that read-write connections still do not work though. At least I would
> not gamble a lot of money/the life of some student near to the robot on
> this ...

You may be right, the intended scenario is this:
when a CorbaPort is involved in the initial creation of a connection (using
the logic of the ConnectionFactory), the local connection is initialised with
a data or buffer CORBA proxy object. This object, when used to read/write the
data, communicates the values over the network. On the remote side, no
ConnectionFactory was used and the new server was created using the
Corba::DataFlowInterface, which serves a data/buffer. On both sides, a
ConnectionInterface object has been created and attached to the port. One
connection holds a proxy, the other (indirectly) a server. Both connections
only have 1 port attached to them. Hence, when I locally break up the
connection, the remote connection is still alive. This is inconsitent with
the C++ API, which removes all ports when a connection is disconnect()'ed.
For this to work equally, we would need connection management as well in the
CORBA layer. Not a high priority...

>
> I defined a connectPorts on DataFlowInterface so that the connecting
> process do not need to get any data flow object on its side.

ok.

>
> One thing though: I don't know one can "adapt" buffered links to data
> links and vice-versa (create a data link between a buffer port and
> a data port for instance). If it is the case, the patch 0007 should be
> dropped, and the 'connectPorts' call should be split as two different
> overloads of connectDataPort and connectBufferPort.

This is not possible. Buffered can not be mixed with data. There is a reason
for this: a data object always has a value, while a buffer can be empty or
full. Also, a data object can always be read or written, a buffer can fail to
return or accept a value. This means that the algorithms using one of those
need to know with which kind they communicate. There is one symbiotic state
(which is not present in Orocos): when a buffer is blocking, it behaves like
a data object because a push and pop will always succeed (return true) and
accept or return a value. We could encapsulate that in a DataObjectInterface,
hiding information about the 'buffer state' (full, empty, size, capacity
etc). But this is again not a high priority.

>
> The only thing I'm not sure about is that poping a value from a buffer
> link actually trigger something on the producing side (i.e. this is not
> a push-pull model). I see that a lot of stuff built around corba event
> services is present in the source code, so I assume that it should not
> be the case (or maybe it is experimental code, or maybe I don't really
> know what corba events are ... :P).

I first tried to implement this protocol using the CORBA event service
implemtation in TAO, but it failed to work according to specifications. Not
to say, it failed to work completely at that time. It was easier to define
BufferChannel and use AssignableExpression for this part. It was our
intention to be completely compatible with the ES though.

I've attached the unit test I was working on. I didn't finish it yet, but it
triggered the crash I wrote about earlier.

Peter

me tracking the svn repository with git ;-)

Sylvain