Implement CORBA disconnect(port).

[eberghoefer]

Disconnect from certain port.

Implementing the previous missing function to disconnect RTT::corba::RemoteOutputPort and RTT::corba::RemoteInputPorts from a certain given remote port. (instead of disconnect all).

[eberghoefer]

I extended the pull request with an additional unit-test in the corba-test application. For my use-case only the call of the disconnect function on RemotePorts giving also another RemotePort as parameter is of interest. The according unit-tests are successful.

Since @doudou requested also having a possibility of giving a local port as parameter, I added two tests for that too. Unfortunately I couldn't figure out how to make that case work (see my next comment with code reference), so suggestions on that one would be very welcome.

[doudou]

Any suggestions?

Well, since it seems that it would not be possible to get local port suport easily, I think it's fair to reduce the functionality at disconnection of remote ports. But then you should test for that case and emit an exception (or return false ? I don't know what would be closer to RTT's general behaviour there)

[eberghoefer]

Yes I agree, actually I think that the use case to disconnect local from remote ports or vice versa will still be a rare corner case. Since at least in the RTT::corba stuff it seams to be that all methods that have a return bool signalling errors by returning false and writing an error message to the log, I followed that here too and write a message to the log and return false, if the "other" port given to one of the RemotePort Disconnect(Port) functions is not a RemotePort too. Also to make clear that this is the case and make sure that indeed false is returned, I leave the unitTest in that checks local to remote variants, but expecting a false as return now.

[eberghoefer]

So if no further objections, could someone please merge the request.

[doudou]

@jbohren @meyerj if you could have a look ...

[meyerj]

I did a quick pass and added some nit comments to the PR.

The new test testRemotePortDisconnect was actually not creating true remote connections because DataFlowI::createConnection() bypasses the creation of a remote connection and creates a local connection instead if the reader port is local (from the perspective of the output port). This could have an effect on the disconnection, too. I added a commit in my fork that explicitly sets the transport_id in the connection policy in order to enforce the CORBA transport, same as other tests: https://github.com/meyerj/rtt/commit/ed9348e77a3c01216c19c1fbee4ee470fc7710f4

The case where one of the two ports is local could simply be covered by forwarding to the local port's disconnect(port) implementation, which keep track of the remote interfaces in the RemoteConnId stored inside the connection manager. I have not verified this.

[eberghoefer]

@meyerj Thank you for your review, I have changed your requests and picked the corrected unitTest from your fork (thanks for that one too ;) ).

    The case where one of the two ports is local could simply be covered by forwarding to the local 
    port's disconnect(port) implementation, ...

This in principle was what I tried out first (can be found in the history of this discussion). Anyway, I tried it again with your adapted unit test, and it indeed changed something. Now the port.disconnect(this) for the RemoteInputPort works also for the case where it is connected to a local output port. I have changed the unit-test again to expect this now and removed the log message for that reason! But still, for the case RemoteOutputPort tries to disconnect from a local input port by trying to leave that up to the local port side port.disconnect(this)) still does not work. If you replace Line 251 (from the current version) of the file RemotePorts.cpp with return port->disconnect(this); instead of just return false; you'll see, that it still returns "false" as the test expects. And adding a test here, that checks for the port being disconnected would fail.

So if you have an idea how to make this also work, let me know.

[eberghoefer]

So since at least to have the functionality of disconnecting remote ports not requiring to stay on the branch, could anybody be so kind to merge the request? @meyerj @doudou ? If not somebody has further requests to be changed.

[jmachowinski]

Could you smash all commits into one ?

[eberghoefer]

sure, here it goes...