You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@plc4x.apache.org by Julian Feinauer <j....@pragmaticminds.de> on 2019/03/07 08:21:24 UTC

[DISCUSS] Major refactoring of the "Fields" API

Hi all,

I think PLC4X is pretty stable and major right now and we currently have two main points of improvement. First, is the generative driver topics which is “driven” by chris mostly.
The other direction is to add new drivers.

But there is one thing in the API which I do not like (and which is not very good from usability standpoint) and this is the “Fields” API.
I think we made a big step forward with the last refactoring we did (from Java Types to “Custom Types”).

Currently PlcField is a Merker interface which bytes us on some cross cutting concerns.
E.g. Tims implementation of the Triggered Scraper (PLC4X-88) is currently S7 specific, because he needs to get some information about the (parsed) Field.

Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.



So I suggest to do a (major) internal refactoring of both these (related) sides.

More concrete I propose:



PlcField:

After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).

Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).




BaseDefaultFieldItem:
I propose to Model the Array, List or Map behavior differently than with all these getters.
I would propose to have several subclasses with Methods like
isArray()
isList()
isMap()
and suitable getters (untyped)
get()
get(int)
getMap(key)
and typed
getBoolean()
…

This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).

Perhaps, in the same refactoring we could even introduce a “PreparedField” or something, which would mean that there was already a round trip to the PLC and the field is “valid”, i.e. will not lead to an exception like “unknown address” or something.
This is something we currently handle pretty bad in the scraper (as we do not differentiate between parsing exceptions “wrong format”, temporary exceptions “connection lost for one or two scrape cycles” or some addresses being “unknown or unreadable”).

What do you others think of that?
If others see this similar (I know chris told me multiple times to keep it simle but often times now this simplicity hurts us in applications) I would start to perapre a design doc in confluence.

Best
Julian

[1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java

Re: [DISCUSS] Major refactoring of the "Fields" API

Posted by Julian Feinauer <j....@pragmaticminds.de>.

Hey Chris,

I just made a quick (but I think not unreasonable) fix for this Issue and opened a PR https://github.com/apache/incubator-plc4x/pull/47.
Could you check over it if this seems suitable for you?

The reason why I use the Java Type there is that all drivers may have different (internal) Types (S7 Driver uses for example the TransportSize enum as DataType).
Thus the Java Type sounds like the smallest common divisor (can one say that in english??) so that clients get information about the result of a Field query.

Julian

Am 07.03.19, 10:01 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:

Hi Chris,

the new Scraper IS currently S7 specific, yes.
This is why I want such a refactoring to make it unspecific (because I also don’t whant it that way).
The reason for this is the following code.

```
this.s7Field = S7Field.of(triggerVariable);
```

Together with the more crucial

```
switch (this.s7Field.getDataType())
```

(both Snippets from TriggerConfiguration.java).

So now you may see why I propose to add some kind of "getDataType()" to the PlcField interface, as this would fix everything and keep things generic.

Of course we need to do it with a bit of a feeling, as all these PLCs may have different types or type-systems, but I'm thinking of it : )

Julian

Am 07.03.19, 09:53 schrieb "Christofer Dutz" <ch...@c-ware.de>:

Hi Julian,

the new Scraper is S7 specific? Was the old one too? Just asking, cause I don't really like the idea of having protocol-specific tools as it's the whole point of PLC4X to be unspecific.

Regarding the other topics ... I sort of couldn't immediately wrap my head around that ... could you maybe do a branch where you have your proposed changes (doesn't have to work) ... so we can see the difference?
I guess we can understand much easier what you have in mind that way.

But in general I think it's a good idea to support structures (Are you thinking of structures the way they are handled in C ... where there's simply an array of bytes and the "struct" lays a pattern/template/stencil on that and allows to to access individual fields.
I think this would be a great feature ... especially as we could use this for automatic optimizations of request (automatically generate a struct if this is more efficient then loading individual fields on their own).

Chris

Am 07.03.19, 09:21 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:

Hi all,

I think PLC4X is pretty stable and major right now and we currently have two main points of improvement. First, is the generative driver topics which is “driven” by chris mostly.
The other direction is to add new drivers.

But there is one thing in the API which I do not like (and which is not very good from usability standpoint) and this is the “Fields” API.
I think we made a big step forward with the last refactoring we did (from Java Types to “Custom Types”).

Currently PlcField is a Merker interface which bytes us on some cross cutting concerns.
E.g. Tims implementation of the Triggered Scraper (PLC4X-88) is currently S7 specific, because he needs to get some information about the (parsed) Field.

Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.

So I suggest to do a (major) internal refactoring of both these (related) sides.

More concrete I propose:

PlcField:

After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).

Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).

BaseDefaultFieldItem:
I propose to Model the Array, List or Map behavior differently than with all these getters.
I would propose to have several subclasses with Methods like
isArray()
isList()
isMap()
and suitable getters (untyped)
get()
get(int)
getMap(key)
and typed
getBoolean()
…

This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).

Perhaps, in the same refactoring we could even introduce a “PreparedField” or something, which would mean that there was already a round trip to the PLC and the field is “valid”, i.e. will not lead to an exception like “unknown address” or something.
This is something we currently handle pretty bad in the scraper (as we do not differentiate between parsing exceptions “wrong format”, temporary exceptions “connection lost for one or two scrape cycles” or some addresses being “unknown or unreadable”).

What do you others think of that?
If others see this similar (I know chris told me multiple times to keep it simle but often times now this simplicity hurts us in applications) I would start to perapre a design doc in confluence.

Best
Julian

[1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java

Re: [DISCUSS] Major refactoring of the "Fields" API

Posted by Julian Feinauer <j....@pragmaticminds.de>.

Hi Chris,

the new Scraper IS currently S7 specific, yes.
This is why I want such a refactoring to make it unspecific (because I also don’t whant it that way).
The reason for this is the following code.

```
this.s7Field = S7Field.of(triggerVariable);
```

Together with the more crucial

```
switch (this.s7Field.getDataType())
```

(both Snippets from TriggerConfiguration.java).

So now you may see why I propose to add some kind of "getDataType()" to the PlcField interface, as this would fix everything and keep things generic.

Of course we need to do it with a bit of a feeling, as all these PLCs may have different types or type-systems, but I'm thinking of it : )

Julian

Am 07.03.19, 09:53 schrieb "Christofer Dutz" <ch...@c-ware.de>:

Hi Julian,

the new Scraper is S7 specific? Was the old one too? Just asking, cause I don't really like the idea of having protocol-specific tools as it's the whole point of PLC4X to be unspecific.

Chris

Am 07.03.19, 09:21 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:

Hi all,

Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.

So I suggest to do a (major) internal refactoring of both these (related) sides.

More concrete I propose:

PlcField:

After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).

Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).

This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).

Best
Julian

[1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java

Re: [DISCUSS] Major refactoring of the "Fields" API

Posted by Tim Mitsch <t....@pragmaticindustries.de>.

Hi everybody

The scraper is not really S7 specific as the sources can be entered via connection-string and the regarding fields as well. We yet only tested functionality with S7 devices because priority has been the highest.
The only thing that is S7 specific right now is the S7_TRIGGER i introduced with my pull-request because we needed something like that for that case. So after exchange with Chris i reworked my code to be a bit more trigger-like, the result can been seen in PR and i think soon in develop ... it's good to make it more generic as discussed in PR and i made two jira issues fort hat.
Btw refactoring of fields as started by Julian is also a part oft he puzzle ... we are getting better and better each day.

Best
Tim 


Am 07.03.19, 12:27 schrieb "Andreas Oswald" <an...@outlook.de>:

    Hi there, 
    
    from my maybe ignorant point of view, I also don’t understand why a feature like the scraper (as I have understood it) should be specific for a special automation family. I´d think that such ideas as reacting on a trigger or doing something on timed trigger basis is pretty basic and straightforward from the view of getting automation tasks done, so why should it be something special for e.g. S7 just because it was first implemented with a special S7 task in mind? 
    
    Just my two cents
    
    Take care 
    
    Andreas
    
     
    
    > Am 07.03.2019 um 09:46 schrieb Christofer Dutz <ch...@c-ware.de>:
    > 
    > Hi Julian,
    > 
    > the new Scraper is S7 specific? Was the old one too? Just asking, cause I don't really like the idea of having protocol-specific tools as it's the whole point of PLC4X to be unspecific.
    > 
    > Regarding the other topics ... I sort of couldn't immediately wrap my head around that ... could you maybe do a branch where you have your proposed changes (doesn't have to work) ... so we can see the difference? 
    > I guess we can understand much easier what you have in mind that way.
    > 
    > But in general I think it's a good idea to support structures (Are you thinking of structures the way they are handled in C ... where there's simply an array of bytes and the "struct" lays a pattern/template/stencil on that and allows to to access individual fields.
    > I think this would be a great feature ... especially as we could use this for automatic optimizations of request (automatically generate a struct if this is more efficient then loading individual fields on their own).
    > 
    > Chris
    > 
    > 
    > 
    > Am 07.03.19, 09:21 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
    > 
    >    Hi all,
    > 
    >    I think PLC4X is pretty stable and major right now and we currently have two main points of improvement. First, is the generative driver topics which is “driven” by chris mostly.
    >    The other direction is to add new drivers.
    > 
    >    But there is one thing in the API which I do not like (and which is not very good from usability standpoint) and this is the “Fields” API.
    >    I think we made a big step forward with the last refactoring we did (from Java Types to “Custom Types”).
    > 
    >    Currently PlcField is a Merker interface which bytes us on some cross cutting concerns.
    >    E.g. Tims implementation of the Triggered Scraper (PLC4X-88) is currently S7 specific, because he needs to get some information about the (parsed) Field.
    > 
    >    Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.
    > 
    > 
    > 
    >    So I suggest to do a (major) internal refactoring of both these (related) sides.
    > 
    >    More concrete I propose:
    > 
    > 
    > 
    >    PlcField:
    > 
    >    After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).
    > 
    >    Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).
    > 
    > 
    > 
    > 
    >    BaseDefaultFieldItem:
    >    I propose to Model the Array, List or Map behavior differently than with all these getters.
    >    I would propose to have several subclasses with Methods like
    >    isArray()
    >    isList()
    >    isMap()
    >    and suitable getters (untyped)
    >    get()
    >    get(int)
    >    getMap(key)
    >    and typed
    >    getBoolean()
    >    …
    > 
    >    This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).
    > 
    >    Perhaps, in the same refactoring we could even introduce a “PreparedField” or something, which would mean that there was already a round trip to the PLC and the field is “valid”, i.e. will not lead to an exception like “unknown address” or something.
    >    This is something we currently handle pretty bad in the scraper (as we do not differentiate between parsing exceptions “wrong format”, temporary exceptions “connection lost for one or two scrape cycles” or some addresses being “unknown or unreadable”).
    > 
    >    What do you others think of that?
    >    If others see this similar (I know chris told me multiple times to keep it simle but often times now this simplicity hurts us in applications) I would start to perapre a design doc in confluence.
    > 
    >    Best
    >    Julian
    > 
    >    [1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
    > 
    >

Re: [DISCUSS] Major refactoring of the "Fields" API

Posted by Julian Feinauer <j....@pragmaticminds.de>.

Hi,

the reason why Tim implemented it that way is a rather technical one.
This should be fixed with the PR 47 which I just submitted [1].

Currently the S7 Driver is the only one which provides type information after Parsing, thus, Tim used that feature directly.

@Tim: You can check the PR and see if you can work with both methods I introduced, then you can make the scaper Type Agnostic.

Julian

[1] https://github.com/apache/incubator-plc4x/pull/47

Am 07.03.19, 12:27 schrieb "Andreas Oswald" <an...@outlook.de>:

    Hi there, 
    
    from my maybe ignorant point of view, I also don’t understand why a feature like the scraper (as I have understood it) should be specific for a special automation family. I´d think that such ideas as reacting on a trigger or doing something on timed trigger basis is pretty basic and straightforward from the view of getting automation tasks done, so why should it be something special for e.g. S7 just because it was first implemented with a special S7 task in mind? 
    
    Just my two cents
    
    Take care 
    
    Andreas
    
     
    
    > Am 07.03.2019 um 09:46 schrieb Christofer Dutz <ch...@c-ware.de>:
    > 
    > Hi Julian,
    > 
    > the new Scraper is S7 specific? Was the old one too? Just asking, cause I don't really like the idea of having protocol-specific tools as it's the whole point of PLC4X to be unspecific.
    > 
    > Regarding the other topics ... I sort of couldn't immediately wrap my head around that ... could you maybe do a branch where you have your proposed changes (doesn't have to work) ... so we can see the difference? 
    > I guess we can understand much easier what you have in mind that way.
    > 
    > But in general I think it's a good idea to support structures (Are you thinking of structures the way they are handled in C ... where there's simply an array of bytes and the "struct" lays a pattern/template/stencil on that and allows to to access individual fields.
    > I think this would be a great feature ... especially as we could use this for automatic optimizations of request (automatically generate a struct if this is more efficient then loading individual fields on their own).
    > 
    > Chris
    > 
    > 
    > 
    > Am 07.03.19, 09:21 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
    > 
    >    Hi all,
    > 
    >    I think PLC4X is pretty stable and major right now and we currently have two main points of improvement. First, is the generative driver topics which is “driven” by chris mostly.
    >    The other direction is to add new drivers.
    > 
    >    But there is one thing in the API which I do not like (and which is not very good from usability standpoint) and this is the “Fields” API.
    >    I think we made a big step forward with the last refactoring we did (from Java Types to “Custom Types”).
    > 
    >    Currently PlcField is a Merker interface which bytes us on some cross cutting concerns.
    >    E.g. Tims implementation of the Triggered Scraper (PLC4X-88) is currently S7 specific, because he needs to get some information about the (parsed) Field.
    > 
    >    Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.
    > 
    > 
    > 
    >    So I suggest to do a (major) internal refactoring of both these (related) sides.
    > 
    >    More concrete I propose:
    > 
    > 
    > 
    >    PlcField:
    > 
    >    After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).
    > 
    >    Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).
    > 
    > 
    > 
    > 
    >    BaseDefaultFieldItem:
    >    I propose to Model the Array, List or Map behavior differently than with all these getters.
    >    I would propose to have several subclasses with Methods like
    >    isArray()
    >    isList()
    >    isMap()
    >    and suitable getters (untyped)
    >    get()
    >    get(int)
    >    getMap(key)
    >    and typed
    >    getBoolean()
    >    …
    > 
    >    This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).
    > 
    >    Perhaps, in the same refactoring we could even introduce a “PreparedField” or something, which would mean that there was already a round trip to the PLC and the field is “valid”, i.e. will not lead to an exception like “unknown address” or something.
    >    This is something we currently handle pretty bad in the scraper (as we do not differentiate between parsing exceptions “wrong format”, temporary exceptions “connection lost for one or two scrape cycles” or some addresses being “unknown or unreadable”).
    > 
    >    What do you others think of that?
    >    If others see this similar (I know chris told me multiple times to keep it simle but often times now this simplicity hurts us in applications) I would start to perapre a design doc in confluence.
    > 
    >    Best
    >    Julian
    > 
    >    [1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
    > 
    >

Re: [DISCUSS] Major refactoring of the "Fields" API

Posted by Andreas Oswald <an...@outlook.de>.

Hi there, 

from my maybe ignorant point of view, I also don’t understand why a feature like the scraper (as I have understood it) should be specific for a special automation family. I´d think that such ideas as reacting on a trigger or doing something on timed trigger basis is pretty basic and straightforward from the view of getting automation tasks done, so why should it be something special for e.g. S7 just because it was first implemented with a special S7 task in mind? 

Just my two cents

Take care 

Andreas

 

> Am 07.03.2019 um 09:46 schrieb Christofer Dutz <ch...@c-ware.de>:
> 
> Hi Julian,
> 
> the new Scraper is S7 specific? Was the old one too? Just asking, cause I don't really like the idea of having protocol-specific tools as it's the whole point of PLC4X to be unspecific.
> 
> Regarding the other topics ... I sort of couldn't immediately wrap my head around that ... could you maybe do a branch where you have your proposed changes (doesn't have to work) ... so we can see the difference? 
> I guess we can understand much easier what you have in mind that way.
> 
> But in general I think it's a good idea to support structures (Are you thinking of structures the way they are handled in C ... where there's simply an array of bytes and the "struct" lays a pattern/template/stencil on that and allows to to access individual fields.
> I think this would be a great feature ... especially as we could use this for automatic optimizations of request (automatically generate a struct if this is more efficient then loading individual fields on their own).
> 
> Chris
> 
> 
> 
> Am 07.03.19, 09:21 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
> 
>    Hi all,
> 
>    I think PLC4X is pretty stable and major right now and we currently have two main points of improvement. First, is the generative driver topics which is “driven” by chris mostly.
>    The other direction is to add new drivers.
> 
>    But there is one thing in the API which I do not like (and which is not very good from usability standpoint) and this is the “Fields” API.
>    I think we made a big step forward with the last refactoring we did (from Java Types to “Custom Types”).
> 
>    Currently PlcField is a Merker interface which bytes us on some cross cutting concerns.
>    E.g. Tims implementation of the Triggered Scraper (PLC4X-88) is currently S7 specific, because he needs to get some information about the (parsed) Field.
> 
>    Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.
> 
> 
> 
>    So I suggest to do a (major) internal refactoring of both these (related) sides.
> 
>    More concrete I propose:
> 
> 
> 
>    PlcField:
> 
>    After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).
> 
>    Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).
> 
> 
> 
> 
>    BaseDefaultFieldItem:
>    I propose to Model the Array, List or Map behavior differently than with all these getters.
>    I would propose to have several subclasses with Methods like
>    isArray()
>    isList()
>    isMap()
>    and suitable getters (untyped)
>    get()
>    get(int)
>    getMap(key)
>    and typed
>    getBoolean()
>    …
> 
>    This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).
> 
>    Perhaps, in the same refactoring we could even introduce a “PreparedField” or something, which would mean that there was already a round trip to the PLC and the field is “valid”, i.e. will not lead to an exception like “unknown address” or something.
>    This is something we currently handle pretty bad in the scraper (as we do not differentiate between parsing exceptions “wrong format”, temporary exceptions “connection lost for one or two scrape cycles” or some addresses being “unknown or unreadable”).
> 
>    What do you others think of that?
>    If others see this similar (I know chris told me multiple times to keep it simle but often times now this simplicity hurts us in applications) I would start to perapre a design doc in confluence.
> 
>    Best
>    Julian
> 
>    [1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
> 
>

Re: [DISCUSS] Major refactoring of the "Fields" API

Posted by Christofer Dutz <ch...@c-ware.de>.

Hi Julian,

the new Scraper is S7 specific? Was the old one too? Just asking, cause I don't really like the idea of having protocol-specific tools as it's the whole point of PLC4X to be unspecific.

Chris

Am 07.03.19, 09:21 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:

Hi all,

Furthermore, when getting a Response, the BaseDefaultFieldItem Interface is quite a Killer.

So I suggest to do a (major) internal refactoring of both these (related) sides.

More concrete I propose:

PlcField:

After parsing, each Driver should report (via PlcField) what he knows about the Field, like the Datatype (see comment below about primitive and non primitive types).

Perhaps we can evene extend this to StructFields, i.e. a Field which is build on a sequence of (aligned) “primitive” Fields (although I’m unsure about the latter).

This could be done very similar then the RelDataType in Calcite [1](except that we would add getters).

Best
Julian

[1] https://github.com/apache/calcite/blob/9721283bd0ce46a337f51a3691585cca8003e399/core/src/main/java/org/apache/calcite/rel/type/RelDataType.java