READ CAREFULLY: On breaking the InputPort::read() API

There was a discussion on Orocos Developers where we were looking into these
issues of the data flow ports:
* a port->read(value) that returns OldData fills in value, which means:
-- the port needs to cache each last value, even if the user will never use it
-- this caching breaks data management since another copy 'always' lives
(until a new sample arrives)
* There's no circular buffer
-- we agreed here to add it in addition to the current buffer in the connection
policy

What remains a big issue is if we should break port->read(value) such that it
does not touch 'value' if the current sample was once read before. The
rationale is that if you need this case, you can simply change:

SomeData value;
if ( inport.read(value) != NoData ) {
   // do something with 'value', it's filled in!
   // allowed to change 'value' too, it will be overwritten
   // during the next read !
}

to:

 // SomeData value; --> moved to the class as member variable !
if ( inport.read(value) != NoData ) {
   // do something with 'value', it's filled in by previous NewData read!
   // not allowed to change value directly since we might need it
   // in the next loop !
}

The advantage is mainly in code size and copy-efficiency of the data in the data
flow. By removing this feature, we don't have to keep a copy of the old sample
and we can simplify the implementation

We'd like to get an idea of:
- Does your code use this case ? (a lot ?)
- Would you mind adapting it when upgrading ?
- Would you agree if we put a warning system in this to flag when new vs old
would give a different result ?
- What's your opinion overall on this ?

No matter what the decision is, this won't affect 2.5...

Peter

PS: please reply to the orocos-users list only.

READ CAREFULLY: On breaking the InputPort::read() API

2011/7/15 Peter Soetens <peter [..] ...>

> There was a discussion on Orocos Developers where we were looking into
> these
> issues of the data flow ports:
> * a port->read(value) that returns OldData fills in value, which means:
> -- the port needs to cache each last value, even if the user will never use
> it
> -- this caching breaks data management since another copy 'always' lives
> (until a new sample arrives)
> * There's no circular buffer
> -- we agreed here to add it in addition to the current buffer in the
> connection
> policy
>
> What remains a big issue is if we should break port->read(value) such that
> it
> does not touch 'value' if the current sample was once read before. The
> rationale is that if you need this case, you can simply change:
>
>

> SomeData value;
> if ( inport.read(value) != NoData ) {
>   // do something with 'value', it's filled in!
>   // allowed to change 'value' too, it will be overwritten
>   // during the next read !
> }
> 

>
> to:
>
>
>  // SomeData value; --> moved to the class as member variable !
> if ( inport.read(value) != NoData ) {
>   // do something with 'value', it's filled in by previous NewData read!
>   // not allowed to change value directly since we might need it
>   // in the next loop !
> }
> 

>

I am a bit disturbed by this feature to not fill 'value' with the actual
value of the port.
While the modification seems acceptable for C++ code of components, and that
I understand the benefits of changing this, I wonder about how people
reading ports from scripts would manage it.

For instance, I have some components that read ports in their 'updateHook',
and where I have the kind of code you mentioned (if read == NewData and
sometimes if read != NoData).
But the point is that I also have scripts, that are loaded and executed on
my components, where I have a inputPort.read(value).

And my problem now is that when read returns OldData, it does not mean that
'this' script has already read it, but that another script (or the component
hooks) have already read it... and the 'value' data of my script will not be
filled.
While the modification of adding a class attribute for 'value' works and is
an acceptable practice for C++ code, I will know need also to have this
attribute become a scripting 'attribute', so that my script will be able to
access it... and then I will have to impose such a behavior for all my
'users', that write components and/or scripts:
1. In components, create one 'attribute' for each input port
2. In scripting, use this attribute when you read (and not just
inputPort.read()).

I think this will be likely to brake the quite good compatibility that I
have, where almost all scripts can be executed on almost all components
(provided the port names are consistent when the scripts use ports).

Charles.

> The advantage is mainly in code size and copy-efficiency of the data in the
> data
> flow. By removing this feature, we don't have to keep a copy of the old
> sample
> and we can simplify the implementation
>
> We'd like to get an idea of:
> - Does your code use this case ? (a lot ?)
> - Would you mind adapting it when upgrading ?
> - Would you agree if we put a warning system in this to flag when new vs
> old
> would give a different result ?
> - What's your opinion overall on this ?
>
> No matter what the decision is, this won't affect 2.5...
>
> Peter
>
> PS: please reply to the orocos-users list only.
> --
> Orocos-Dev mailing list
> Orocos-Dev [..] ...
> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>

READ CAREFULLY: On breaking the InputPort::read() API

On Tuesday 19 July 2011 14:24:51 Charles Lesire-Cabaniols wrote:
> 2011/7/15 Peter Soetens <peter [..] ...>
>
> > There was a discussion on Orocos Developers where we were looking into
> > these
> > issues of the data flow ports:
> > * a port->read(value) that returns OldData fills in value, which means:
> > -- the port needs to cache each last value, even if the user will never
> > use it
> > -- this caching breaks data management since another copy 'always' lives
> > (until a new sample arrives)
> > * There's no circular buffer
> > -- we agreed here to add it in addition to the current buffer in the
> > connection
> > policy
> >
> > What remains a big issue is if we should break port->read(value) such
> > that it
> > does not touch 'value' if the current sample was once read before. The
> > rationale is that if you need this case, you can simply change:
> >
> >

> > SomeData value;
> > if ( inport.read(value) != NoData ) {
> > 
> >   // do something with 'value', it's filled in!
> >   // allowed to change 'value' too, it will be overwritten
> >   // during the next read !
> > 
> > }
> > 

> >
> > to:
> >
> >
> > 
> >  // SomeData value; --> moved to the class as member variable !
> > 
> > if ( inport.read(value) != NoData ) {
> > 
> >   // do something with 'value', it's filled in by previous NewData read!
> >   // not allowed to change value directly since we might need it
> >   // in the next loop !
> > 
> > }
> > 

>
> I am a bit disturbed by this feature to not fill 'value' with the actual
> value of the port.
> While the modification seems acceptable for C++ code of components, and
> that I understand the benefits of changing this, I wonder about how people
> reading ports from scripts would manage it.
>
> For instance, I have some components that read ports in their 'updateHook',
> and where I have the kind of code you mentioned (if read == NewData and
> sometimes if read != NoData).
> But the point is that I also have scripts, that are loaded and executed on
> my components, where I have a inputPort.read(value).
>
> And my problem now is that when read returns OldData, it does not mean that
> 'this' script has already read it, but that another script (or the
> component hooks) have already read it... and the 'value' data of my script
> will not be filled.
> While the modification of adding a class attribute for 'value' works and is
> an acceptable practice for C++ code, I will know need also to have this
> attribute become a scripting 'attribute', so that my script will be able to
> access it... and then I will have to impose such a behavior for all my
> 'users', that write components and/or scripts:
> 1. In components, create one 'attribute' for each input port
> 2. In scripting, use this attribute when you read (and not just
> inputPort.read()).
>
> I think this will be likely to brake the quite good compatibility that I
> have, where almost all scripts can be executed on almost all components
> (provided the port names are consistent when the scripts use ports).
>

Thanks for this analysis, we overlooked this use case until now. The
workaround you propose is also the only thing I can think of... since the port
does not store read values, we can't even add a new 'readFoo' function that
returns it.

A reasonable solution to all this is to add a new InputPort type which
implements the new 'nostore' behavior. It would reuse the maximum amount of
code of the existing InputPort in order to reduce the effects on code size etc.
The existing InputPort would then keep its API and current behavior... I
think this is the only reasonable thing to do and it allows users to
transition on a component-by-component basis.

Suggestions for names ?

Peter

READ CAREFULLY: On breaking the InputPort::read() API

On Fri, 22 Jul 2011, Peter Soetens wrote:

> On Tuesday 19 July 2011 14:24:51 Charles Lesire-Cabaniols wrote:
>> 2011/7/15 Peter Soetens <peter [..] ...>
>>
>>> There was a discussion on Orocos Developers where we were looking into
>>> these
>>> issues of the data flow ports:
>>> * a port->read(value) that returns OldData fills in value, which means:
>>> -- the port needs to cache each last value, even if the user will never
>>> use it
>>> -- this caching breaks data management since another copy 'always' lives
>>> (until a new sample arrives)
>>> * There's no circular buffer
>>> -- we agreed here to add it in addition to the current buffer in the
>>> connection
>>> policy
>>>
>>> What remains a big issue is if we should break port->read(value) such
>>> that it
>>> does not touch 'value' if the current sample was once read before. The
>>> rationale is that if you need this case, you can simply change:
>>>
>>>

>>> SomeData value;
>>> if ( inport.read(value) != NoData ) {
>>>
>>>   // do something with 'value', it's filled in!
>>>   // allowed to change 'value' too, it will be overwritten
>>>   // during the next read !
>>>
>>> }
>>> 

>>>
>>> to:
>>>
>>>
>>>
>>>  // SomeData value; --> moved to the class as member variable !
>>>
>>> if ( inport.read(value) != NoData ) {
>>>
>>>   // do something with 'value', it's filled in by previous NewData read!
>>>   // not allowed to change value directly since we might need it
>>>   // in the next loop !
>>>
>>> }
>>> 

>>
>> I am a bit disturbed by this feature to not fill 'value' with the actual
>> value of the port.
>> While the modification seems acceptable for C++ code of components, and
>> that I understand the benefits of changing this, I wonder about how people
>> reading ports from scripts would manage it.
>>
>> For instance, I have some components that read ports in their 'updateHook',
>> and where I have the kind of code you mentioned (if read == NewData and
>> sometimes if read != NoData).
>> But the point is that I also have scripts, that are loaded and executed on
>> my components, where I have a inputPort.read(value).
>>
>> And my problem now is that when read returns OldData, it does not mean that
>> 'this' script has already read it, but that another script (or the
>> component hooks) have already read it... and the 'value' data of my script
>> will not be filled.
>> While the modification of adding a class attribute for 'value' works and is
>> an acceptable practice for C++ code, I will know need also to have this
>> attribute become a scripting 'attribute', so that my script will be able to
>> access it... and then I will have to impose such a behavior for all my
>> 'users', that write components and/or scripts:
>> 1. In components, create one 'attribute' for each input port
>> 2. In scripting, use this attribute when you read (and not just
>> inputPort.read()).
>>
>> I think this will be likely to brake the quite good compatibility that I
>> have, where almost all scripts can be executed on almost all components
>> (provided the port names are consistent when the scripts use ports).
>>
>
> Thanks for this analysis, we overlooked this use case until now. The
> workaround you propose is also the only thing I can think of... since the port
> does not store read values, we can't even add a new 'readFoo' function that
> returns it.
>
> A reasonable solution to all this is to add a new InputPort type which
> implements the new 'nostore' behavior. It would reuse the maximum amount of
> code of the existing InputPort in order to reduce the effects on code size etc.
> The existing InputPort would then keep its API and current behavior... I
> think this is the only reasonable thing to do and it allows users to
> transition on a component-by-component basis.
>
> Suggestions for names ?

I think it is decent design: the "Ports" are (in our BRICS Component Model
design...) the "interconnections" between the "outside" and the "inside" of
a component, and in this context it can make sense to give them their own
"behaviour"; in the current case, that behaviour is about what they do with
data that is travelling from the "outside" to the "inside". In a similar
context, Ports might be the right place to add "inside/out" behaviour,
such as adding time stamps, UIDs, semantic tags,...

I have to think some more about it.

About the naming: I am in favour of using names that have the best semantic
meaning, but that can only be defined in the right semantic context. My
paragraph above _is_ such a context, where 'nostore' is to be interpreted
as the behaviour of a Port as the 'actor' between the inside and the
outside of a component. Maybe that's enough as semantic context, maybe it
is not.

I have to think some more about it.

Herman

READ CAREFULLY: On breaking the InputPort::read() API

2011/7/15 Peter Soetens <peter [..] ...>:
> There was a discussion on Orocos Developers where we were looking into these
> issues of the data flow ports:
> * a port->read(value) that returns OldData fills in value, which means:
> -- the port needs to cache each last value, even if the user will never use it
> -- this caching breaks data management since another copy 'always' lives
> (until a new sample arrives)
> * There's no circular buffer
> -- we agreed here to add it in addition to the current buffer in the connection
> policy
>
> What remains a big issue is if we should break port->read(value) such that it
> does not touch 'value' if the current sample was once read before. The
> rationale is that if you need this case, you can simply change:
>
>

> SomeData value;
> if ( inport.read(value) != NoData ) {
>   // do something with 'value', it's filled in!
>   // allowed to change 'value' too, it will be overwritten
>   // during the next read !
> }
> 

>
> to:
>
>
>  // SomeData value; --> moved to the class as member variable !
> if ( inport.read(value) != NoData ) {
>   // do something with 'value', it's filled in by previous NewData read!
>   // not allowed to change value directly since we might need it
>   // in the next loop !
> }
> 

>
> The advantage is mainly in code size and copy-efficiency of the data in the data
> flow. By removing this feature, we don't have to keep a copy of the old sample
> and we can simplify the implementation
>
> We'd like to get an idea of:
> - Does your code use this case ? (a lot ?)

Yes

> - Would you mind adapting it when upgrading ?
> - Would you agree if we put a warning system in this to flag when new vs old
> would give a different result ?
> - What's your opinion overall on this ?

It might be better performance-wise, I think it's a lot _worse_
considering code readability and intuitive understanding of it.
When I see port.read(var) I go on expecting var to have the value
that's on the port - this will no longer always be the case...

Steven

>
> No matter what the decision is, this won't affect 2.5...
>
> Peter
>
> PS: please reply to the orocos-users list only.
> --
> Orocos-Dev mailing list
> Orocos-Dev [..] ...
> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>

READ CAREFULLY: On breaking the InputPort::read() API

On Jul 15, 2011, at 05:02 , Steven Bellens wrote:

> 2011/7/15 Peter Soetens <peter [..] ...>:
>> There was a discussion on Orocos Developers where we were looking into these
>> issues of the data flow ports:
>> * a port->read(value) that returns OldData fills in value, which means:
>> -- the port needs to cache each last value, even if the user will never use it
>> -- this caching breaks data management since another copy 'always' lives
>> (until a new sample arrives)
>> * There's no circular buffer
>> -- we agreed here to add it in addition to the current buffer in the connection
>> policy
>>
>> What remains a big issue is if we should break port->read(value) such that it
>> does not touch 'value' if the current sample was once read before. The
>> rationale is that if you need this case, you can simply change:
>>
>>

>> SomeData value;
>> if ( inport.read(value) != NoData ) {
>>   // do something with 'value', it's filled in!
>>   // allowed to change 'value' too, it will be overwritten
>>   // during the next read !
>> }
>> 

>>
>> to:
>>
>>
>>  // SomeData value; --> moved to the class as member variable !
>> if ( inport.read(value) != NoData ) {
>>   // do something with 'value', it's filled in by previous NewData read!
>>   // not allowed to change value directly since we might need it
>>   // in the next loop !
>> }
>> 

>>
>> The advantage is mainly in code size and copy-efficiency of the data in the data
>> flow. By removing this feature, we don't have to keep a copy of the old sample
>> and we can simplify the implementation
>>
>> We'd like to get an idea of:
>> - Does your code use this case ? (a lot ?)
>
> Yes
>
>> - Would you mind adapting it when upgrading ?
>> - Would you agree if we put a warning system in this to flag when new vs old
>> would give a different result ?
>> - What's your opinion overall on this ?
>
> It might be better performance-wise, I think it's a lot _worse_
> considering code readability and intuitive understanding of it.
> When I see port.read(var) I go on expecting var to have the value
> that's on the port - this will no longer always be the case...

Interesting, I think the opposite. If read(value) is not "returning" a value, then it shouldn't change value for me. It should just tell me that it had nothing new (ie nothing new to return). Different folks ... different strokes.
S

READ CAREFULLY: On breaking the InputPort::read() API

On 07/15/2011 01:56 PM, S Roderick wrote:
>> It might be better performance-wise, I think it's a lot _worse_
>> considering code readability and intuitive understanding of it.
>> When I see port.read(var) I go on expecting var to have the value
>> that's on the port - this will no longer always be the case...
>
> Interesting, I think the opposite. If read(value) is not "returning" a value, then it shouldn't change value for me. It should just tell me that it had nothing new (ie nothing new to return). Different folks ... different strokes.
Yep, I realized recently that some people at DFKI intuitively thought
that OldData was not changing the parameter.

Sylvain

READ CAREFULLY: On breaking the InputPort::read() API

On 07/15/2011 11:23 AM, Peter Soetens wrote:
> We'd like to get an idea of:
> - Does your code use this case ? (a lot ?)
> - Would you mind adapting it when upgrading ?
> - Would you agree if we put a warning system in this to flag when new vs old
> would give a different result ?
> - What's your opinion overall on this ?
I +1 this change. We do use this pattern, but I don't mind pushing the
needed change. Especially since the "new" usage pattern can be used on
the current code, so we could start migrating / reviewing even before
the change is rolled out.

READ CAREFULLY: On breaking the InputPort::read() API

On Friday 15 July 2011 11:34:49 Sylvain Joyeux wrote:
> On 07/15/2011 11:23 AM, Peter Soetens wrote:
> > We'd like to get an idea of:
> > - Does your code use this case ? (a lot ?)
> > - Would you mind adapting it when upgrading ?
> > - Would you agree if we put a warning system in this to flag when new vs
> > old would give a different result ?
> > - What's your opinion overall on this ?
>
> I +1 this change. We do use this pattern, but I don't mind pushing the
> needed change. Especially since the "new" usage pattern can be used on
> the current code, so we could start migrating / reviewing even before
> the change is rolled out.

That is indeed something we could inject into 2.5, such that it would warn for
deprecated use...

Peter

Ruben Smits's picture

READ CAREFULLY: On breaking the InputPort::read() API

On Friday 15 July 2011 11:23:30 Peter Soetens wrote:
> There was a discussion on Orocos Developers where we were looking into
> these issues of the data flow ports:
> * a port->read(value) that returns OldData fills in value, which means:
> -- the port needs to cache each last value, even if the user will never use
> it -- this caching breaks data management since another copy 'always'
> lives (until a new sample arrives)
> * There's no circular buffer
> -- we agreed here to add it in addition to the current buffer in the
> connection policy
>
> What remains a big issue is if we should break port->read(value) such that
> it does not touch 'value' if the current sample was once read before. The
> rationale is that if you need this case, you can simply change:
>
>

> SomeData value;
> if ( inport.read(value) != NoData ) {
>    // do something with 'value', it's filled in!
>    // allowed to change 'value' too, it will be overwritten
>    // during the next read !
> }
> 

>
> to:
>
>
>  // SomeData value; --> moved to the class as member variable !
> if ( inport.read(value) != NoData ) {
>    // do something with 'value', it's filled in by previous NewData read!
>    // not allowed to change value directly since we might need it
>    // in the next loop !
> }
> 

>
> The advantage is mainly in code size and copy-efficiency of the data in the
> data flow. By removing this feature, we don't have to keep a copy of the
> old sample and we can simplify the implementation
>
> We'd like to get an idea of:
> - Does your code use this case ? (a lot ?)

Yes, a lot (but only with fixed-size data)

> - Would you mind adapting it when upgrading ?

No, but I would like to be warned somehow if I need to take a look at it
because a use a mixture of both cases.

> - Would you agree if we put a warning system in this to flag when new vs
> old would give a different result ?

A runtime warning or a compile time warning?

> - What's your opinion overall on this ?
>
> No matter what the decision is, this won't affect 2.5...
>
> Peter
>
> PS: please reply to the orocos-users list only.

-- Ruben

READ CAREFULLY: On breaking the InputPort::read() API

2011/7/15 Ruben Smits <ruben [dot] smits [..] ...>

> On Friday 15 July 2011 11:23:30 Peter Soetens wrote:
> > There was a discussion on Orocos Developers where we were looking into
> > these issues of the data flow ports:
> > * a port->read(value) that returns OldData fills in value, which means:
> > -- the port needs to cache each last value, even if the user will never
> use
> > it -- this caching breaks data management since another copy 'always'
> > lives (until a new sample arrives)
> > * There's no circular buffer
> > -- we agreed here to add it in addition to the current buffer in the
> > connection policy
> >
> > What remains a big issue is if we should break port->read(value) such
> that
> > it does not touch 'value' if the current sample was once read before. The
> > rationale is that if you need this case, you can simply change:
> >
> >

> > SomeData value;
> > if ( inport.read(value) != NoData ) {
> >    // do something with 'value', it's filled in!
> >    // allowed to change 'value' too, it will be overwritten
> >    // during the next read !
> > }
> > 

> >
> > to:
> >
> >
> >  // SomeData value; --> moved to the class as member variable !
> > if ( inport.read(value) != NoData ) {
> >    // do something with 'value', it's filled in by previous NewData read!
> >    // not allowed to change value directly since we might need it
> >    // in the next loop !
> > }
> > 

> >
> > The advantage is mainly in code size and copy-efficiency of the data in
> the
> > data flow. By removing this feature, we don't have to keep a copy of the
> > old sample and we can simplify the implementation
> >
> > We'd like to get an idea of:
> > - Does your code use this case ? (a lot ?)
>
> Yes, a lot (but only with fixed-size data)
>
> > - Would you mind adapting it when upgrading ?
>
> No, but I would like to be warned somehow if I need to take a look at it
> because a use a mixture of both cases.
>

Same for me.

>
> > - Would you agree if we put a warning system in this to flag when new vs
> > old would give a different result ?
>
> A runtime warning or a compile time warning?
>
> > - What's your opinion overall on this ?
> >
> > No matter what the decision is, this won't affect 2.5...
> >
> > Peter
> >
> > PS: please reply to the orocos-users list only.
>
> -- Ruben
> --
> Orocos-Users mailing list
> Orocos-Users [..] ...
> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-users
>

READ CAREFULLY: On breaking the InputPort::read() API

On Friday 15 July 2011 11:33:33 Ruben Smits wrote:
> On Friday 15 July 2011 11:23:30 Peter Soetens wrote:
> > There was a discussion on Orocos Developers where we were looking into
> > these issues of the data flow ports:
> > * a port->read(value) that returns OldData fills in value, which means:
> > -- the port needs to cache each last value, even if the user will never
> > use it -- this caching breaks data management since another copy
> > 'always' lives (until a new sample arrives)
> > * There's no circular buffer
> > -- we agreed here to add it in addition to the current buffer in the
> > connection policy
> >
> > What remains a big issue is if we should break port->read(value) such
> > that it does not touch 'value' if the current sample was once read
> > before. The rationale is that if you need this case, you can simply
> > change:
> >
> >

> > SomeData value;
> > if ( inport.read(value) != NoData ) {
> > 
> >    // do something with 'value', it's filled in!
> >    // allowed to change 'value' too, it will be overwritten
> >    // during the next read !
> > 
> > }
> > 

> >
> > to:
> >
> >
> > 
> >  // SomeData value; --> moved to the class as member variable !
> > 
> > if ( inport.read(value) != NoData ) {
> > 
> >    // do something with 'value', it's filled in by previous NewData read!
> >    // not allowed to change value directly since we might need it
> >    // in the next loop !
> > 
> > }
> > 

> >
> > The advantage is mainly in code size and copy-efficiency of the data in
> > the data flow. By removing this feature, we don't have to keep a copy of
> > the old sample and we can simplify the implementation
> >
> > We'd like to get an idea of:
> > - Does your code use this case ? (a lot ?)
>
> Yes, a lot (but only with fixed-size data)
>
> > - Would you mind adapting it when upgrading ?
>
> No, but I would like to be warned somehow if I need to take a look at it
> because a use a mixture of both cases.
>
> > - Would you agree if we put a warning system in this to flag when new vs
> > old would give a different result ?
>
> A runtime warning or a compile time warning?

Run-time...the only way to trigger a compile-time issue is by renaming the
read() function or the InputPort class...but I fear this will just lead people
to using 'sed' scripts on their code and as such that it's a not giving the
safety you'd expect in practice.

Peter