Re: Updated dataflow semantics for RTT
Johannes Meyer <[email protected]> Tue, 22 Sep 2015 18:15:52 +0200
| Newsgroups | gmane.science.robotics.orocos.devel |
|---|---|
| Message-ID | <CABowFJHEVAXoSGRw2uJH6yMYoDfb9_j8MWq_Tyr4ccJKUsF-Qw@mail.gmail.com> |
--===============0506500026== Content-Type: multipart/alternative; boundary=001a11425fb40f55df05205851f9 --001a11425fb40f55df05205851f9 Content-Type: text/plain; charset=UTF-8 Hi, On Tue, Sep 22, 2015 at 3:32 PM, Janosch Machowinski < [email protected]> wrote: > Hi, > here are my remarks: > First of all, I like the idea of the input buffer (push case). But I also > see some > problems with it. The restriction on the same buffer size and type in the > connection policy will be annoying (In my mind I can see the runtime > errors pop up). > It will be hard to track this in bigger systems. Do I get it correctly, > that you only > want to preserve arrival order and runtime here ? > No, we think the current implementation with one buffer per connection could really be considered as a bug, at least for certain use cases. Consider two output ports W1 and W2 which are both connected to the same input port R with a data connection. What I would expect is: W1.write("foo") W2.write("bar") assert(R.read(x) == NewData && x == "bar") assert(R.read(x) == OldData && x == "bar") so each writer overwrites the data sample from previous writes (think of a robot command). But what happens in RTT v2 is: W1.write("foo") W2.write("bar") assert(R.read(x) == NewData && x == "foo") assert(R.read(x) == NewData && x == "bar") and I cannot even be sure if I first read "foo" or "bar" because it depends on what connection was read last. The first result would be the new default case (push), while the second can still be achieved by setting pull to true, which had no effect until now for local connections. Certainly there is a significant chance that the new restrictions break existing deployments and that's why we would only release it with a new minor, if not even major version number. But to fix this serious drawback of the current dataflow implementation it is certainly worth the effort. On Tue, Sep 22, 2015 at 4:17 PM, Janosch Machowinski < [email protected]> wrote: > Am 22.09.2015 um 16:09 schrieb Sylvain Joyeux: > >> If you are already changing the Connection implementation, I would > >> recommend, to put a new thread in every connection in remote case. We > experienced the > >> issue, that bad Wifi connections, slowed our systems down, as it would > hang on the write > >> call. > > This should IMO be done by the middleware ... in the CORBA case we > > should have a way to spawn separate corba dispatchers (e.g. based on > > the policie's name field) to isolate the domains. Moreover, there's > > already been work done to use CORBA's "oneway" call to avoid this > > problem completely (but I don't know what's the status of that) > > > > Sylvain > This is basically a design decision, thread all remote connection, to be > sure > that the do not block you task, or rely on the connection to be properly > implemented. I agree with Sylvain here: It is the transport's responsibility to not block on write or read operations and if the underlying implementation cannot guarantee that, to use single worker thread, thread pool or one thread per connection. If this would be enforced by the RTT connection factory already, it would be impossible to use real-time capable transports like Xenomai message queues. We indeed also have an updated CORBA transport ready which introduces oneway calls wherever possible and might solve some of the problems you observed. It could be considered as stable and I could prepare another pull request, but of course there would be plenty of conflicts with the updated-dataflow-semantics branch, so I would prefer to postpone this until this one is merged. > > For the Feedback on the write I would recommend to create a second enum, > and not > to use FlowStatus. Sometimes I use switch/case to react to the read > FlowStatus. If you > Introduce new members, this will result in compile warnings, because of > unhandled > entrie. > You are right and I first added a new enum-like class WriteStatus, but it would also require a separate TypeInfo, to be added to the default typekit, a corresponding struct in the CORBA IDL and so on, so at the end it seemed easier to just add some constants to FlowStatus. The reason to have a class instead of a simple enum is that the implicit conversion to bool could be implemented more meaningful, more specifically that WriteSuccess evaluates to true and all other constants to false. > > For the creation of shared connections, I would recommend an new API. The > create call > should contain all involved members. This spares us the headache of > tracking buffer sizes > and types around in the code. Also this will simplify the code logic, as > we don't need the lookup. > e.g. > out.connectShared(b.in, c.in, d.in, policy) ; > A new API instead of adding the shared flag to the ConnPolicy? Or only as a thin wrapper which implies that the shared flag is set? It should be noted that it is not the new concept of shared connections that adds constraints, but especially the per-input port buffers implied by the default pull = false setting, even for private connections. What you probably mean is to introduce a separate API for all new dataflow models and make sure that the existing API calls behave the same as before. A separate API would at least require additional updates in the deployer in order to make it accessible from deployment scripts or XML configurations. If people really feel a need for that, a possible intermediate solution would be a preprocessor macro and/or cmake variable that enables a compatibility mode which restores the current v2.x connection model and basically enforces the pull connection type for local connections and installs a per-connection buffer at the reader's side for remote push connections. In this mode, RTT could at least print some deprecation warnings to the Orocos log if a connection attempt would have failed without the macro to be defined. Then at least you can postpone necessary changes in your application until there is more time to do it and still use other new features of an upcoming RTT release. > Greetings > Janosch > Best regards Johannes --001a11425fb40f55df05205851f9 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div class=3D"gmail_extra">Hi,</div><div class=3D"gmail_ex= tra"><br><div class=3D"gmail_quote">On Tue, Sep 22, 2015 at 3:32 PM, Janosc= h Machowinski <span dir=3D"ltr"><<a href=3D"mailto:Janosch.Machowinski@d= fki.de" target=3D"_blank">[email protected]</a>></span> wrote:= <br><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bor= der-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:sol= id;padding-left:1ex"> =20 =20 =20 <div text=3D"#000000" bgcolor=3D"#FFFFFF"> <div>Hi,<br> here are my remarks:<br> First of all, I like the idea of the input buffer (push case). But I also see some<br> problems with it. The restriction on the same buffer size and type in the <br> connection policy will be annoying (In my mind I can see the runtime errors pop up). <br> It will be hard to track this in bigger systems. Do I get it correctly, that you only<br> want to preserve arrival order and runtime here ?<br></div></div></bl= ockquote><div><br></div><div>No, we think the current implementation with o= ne buffer per connection could really be considered as a bug, at least for = certain use cases. Consider two output ports W1 and W2 which are both conne= cted to the same input port R with a data connection. What I would expect i= s:</div><div><br></div><div>W1.write("foo")</div><div>W2.write(&q= uot;bar")</div><div>assert(R.read(x) =3D=3D NewData && x =3D= =3D "bar")</div><div>assert(R.read(x) =3D=3D OldData=C2=A0&&a= mp; x =3D=3D "bar")</div><div><br></div><div>so each writer overw= rites the data sample from previous writes (think of a robot command). But = what happens in RTT v2 is:</div><div><br></div><div><div>W1.write("foo= ")</div><div>W2.write("bar")</div><div>assert(R.read(x) =3D= =3D NewData && x =3D=3D "foo")</div><div>assert(R.read(x)= =3D=3D NewData && x =3D=3D "bar")</div><div><br></div><d= iv>and I cannot even be sure if I first read "foo" or "bar&q= uot; because it depends on what connection was read last. The first result = would be the new default case (push), while the second can still be achieve= d by setting pull to true, which had no effect until now for local connecti= ons.</div></div><div><br></div><div>Certainly there is a significant chance= that the new restrictions break existing deployments and that's why we= would only release it with a new minor, if not even major version number. = But to fix this serious drawback of the current dataflow implementation it = is certainly worth the effort.</div><div><br></div><div><br></div>On Tue, S= ep 22, 2015 at 4:17 PM, Janosch Machowinski=C2=A0<span dir=3D"ltr"><<a h= ref=3D"mailto:[email protected]" target=3D"_blank">Janosch.Machow= [email protected]</a>></span>=C2=A0wrote:<br><blockquote class=3D"gmail_quot= e" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-colo= r:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=3D= "">Am 22.09.2015 um 16:09 schrieb Sylvain Joyeux:<br>>> If you are al= ready changing the Connection implementation, I would<br>>> recommend= , to put a new thread in every connection in remote case. We experienced th= e<br>>> issue, that bad Wifi connections, slowed our systems down, as= it would hang on the write<br>>> call.<br>> This should IMO be do= ne by the middleware ... in the CORBA case we<br>> should have a way to = spawn separate corba dispatchers (e.g. based on<br>> the policie's n= ame field) to isolate the domains. Moreover, there's<br>> already be= en work done to use CORBA's "oneway" call to avoid this<br>&g= t; problem completely (but I don't know what's the status of that)<= br>><br>> Sylvain<br></span>This is basically a design decision, thre= ad all remote connection, to be<br>sure<br>that the do not block you task, = or rely on the connection to be properly<br>implemented.</blockquote><div><= br></div><div>I agree with Sylvain here: It is the transport's responsi= bility to not block on write or read operations and if the underlying imple= mentation cannot guarantee that, to use single worker thread, thread pool o= r one thread per connection. If this would be enforced by the RTT connectio= n factory already, it would be impossible to use real-time capable transpor= ts like Xenomai message queues.</div><div><br></div><div>We indeed also hav= e an updated CORBA transport ready which introduces oneway calls wherever p= ossible and might solve some of the problems you observed. It could be cons= idered as stable and I could prepare another pull request, but of course th= ere would be plenty of conflicts with the updated-dataflow-semantics branch= , so I would prefer to postpone this until this one is merged.</div><div><b= r></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:= 0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);= border-left-style:solid;padding-left:1ex"><div text=3D"#000000" bgcolor=3D"= #FFFFFF"><div> <br> For the Feedback on the write I would recommend to create a second enum, and not<br> to use FlowStatus. Sometimes I use switch/case to react to the read FlowStatus. If you<br> Introduce new members, this will result in compile warnings, because of unhandled<br> entrie.<br></div></div></blockquote><div><br></div><div>You are right= and I first added a new enum-like class WriteStatus, but it would also req= uire a separate TypeInfo, to be added to the default typekit, a correspondi= ng struct in the CORBA IDL and so on, so at the end it seemed easier to jus= t add some constants to FlowStatus. The reason to have a class instead of a= simple enum is that the implicit conversion to bool could be implemented m= ore meaningful, more specifically that WriteSuccess evaluates to true and a= ll other constants to false.</div><div><br></div><div>=C2=A0</div><blockquo= te class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-widt= h:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-le= ft:1ex"><div text=3D"#000000" bgcolor=3D"#FFFFFF"><div> <br> For the creation of shared connections, I would recommend an new API. The create call<br> should contain all involved members. This spares us the headache of tracking buffer sizes<br> and types around in the code. Also this will simplify the code logic, as we don't need the lookup.<br> e.g. <br> out.connectShared(<a href=3D"http://b.in" target=3D"_blank">b.in</a>,= <a href=3D"http://c.in" target=3D"_blank">c.in</a>, <a href=3D"http://d.in= " target=3D"_blank">d.in</a>, policy) ;<br></div></div></blockquote><div><b= r></div><div>A new API instead of adding the shared flag to the ConnPolicy?= Or only as a thin wrapper which implies that the shared flag is set?</div>= <div>It should be noted that it is not the new concept of shared connection= s that adds constraints, but especially the per-input port buffers implied = by the default pull =3D false setting, even for private connections. What y= ou probably mean is to introduce a separate API for all new dataflow models= and make sure that the existing API calls behave the same as before.</div>= <div><br></div><div>A separate API would at least require additional update= s in the deployer in order to make it accessible from deployment scripts or= XML configurations.</div><div><br></div><div>If people really feel a need = for that, a possible intermediate solution would be a preprocessor macro an= d/or cmake variable that enables a compatibility mode which restores the cu= rrent v2.x connection model and basically enforces the pull connection type= for local connections and installs a per-connection buffer at the reader&#= 39;s side for remote push connections. In this mode, RTT could at least pri= nt some deprecation warnings to the Orocos log if a connection attempt woul= d have failed without the macro to be defined. Then at least you can postpo= ne necessary changes in your application until there is more time to do it = and still use other new features of an upcoming RTT release.</div><div><br>= </div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0p= x 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);bo= rder-left-style:solid;padding-left:1ex"><div text=3D"#000000" bgcolor=3D"#F= FFFFF"><div> Greetings<br> =C2=A0=C2=A0=C2=A0 Janosch</div></div></blockquote><div><br></div><di= v><br></div><div>Best regards</div><div>Johannes</div><div><br></div></div>= <div></div> </div></div> --001a11425fb40f55df05205851f9-- --===============0506500026== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline -- Orocos-Dev mailing list [email protected] http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev --===============0506500026==--