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 2018/08/22 14:19:32 UTC
Internal Type System
Hey all,
we had several discussions about type handling [1,2] in general and in drivers and especially the addition of type information for the S7 Driver [3].
I am currently implementing the “typed” addresses for the S7 Driver [3] and wanted to propose the approach there as it could form the basis of the general “internal” typesystem for all drivers.
Motivation:
Provide all drivers with a common type system to reduce the amount of driver specific code for “type” handling.
The approach is the following:
I added an abstract class TypedAddress which implements the marker Interface Address (which is returned from the Driver).
TypedAddress contains all necessary “generic” information to be able to decode values from a byte[] array, i.e., the offset for byte / bit, the number of bits used by the value and a Category.
Category encapsulates the “logical type”, i.e., if the value is INTEGER or DECIMAL, SIGNED or UNSIGNED, STRING, DATE or whatsoever.
For the S7 driver the TypedAddress is extended by the TypedS7Address which adds S7 Specific properties like memory area.
In case of the S7 flow all subsequent usages of Address in the lower layers (XXXProtocol) are made with regards to the TypedS7Address.
Furthermore, I suggest to use the Wrapper pattern for the address interface, see e.g. [5] from java.sql as it makes these instance-checks in the driver code “nicer”.
What do you think of these changes, do you agree with the approach I use?
@Sebastian
I had a look through the Ads Implementation and I think this approach would be feasible for the ads implementation too. Is this correct?
@Chris
I’m lacking the feeling of when to start a [DISCUSSION] or how the general guidelines are here and especially in this project so I’m thankful for advice
Best
Julian
[1] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3C1B180308-0BFA-4203-869C-ABE64D77B203%40c-ware.de%3E
[2] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3CF8E442F3-906C-47D6-9F08-A393F6D11557%40pragmaticminds.de%3<http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3CF8E442F3-906C-47D6-9F08-A393F6D11557%40pragmaticminds.de%253>E
[3] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201808.mbox/%3C481F0FC1-0A9B-44F8-B594-C8E825EB5026%40c-ware.de%3E
[4] https://github.com/JulianFeinauer/incubator-plc4x/tree/add-typed-addresses-s7
[5] https://docs.oracle.com/javase/7/docs/api/java/sql/Wrapper.html
Re: Internal Type System
Posted by Sebastian Rühl <se...@googlemail.com.INVALID>.
FYI: here are some types used in TwinCAT 2/TX1200 https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
so now I've invested most of this day in brainstorming and documenting. I think now I found a way to document what I've done.
Please have a look at this page [1] and provide some feedback (or alternate proposals)
I've intentionally left away the Subscription part as that wouldn't add much value and most of the design considerations would apply to that too.
Chris
[1] https://cwiki.apache.org/confluence/display/PLC4X/Chris%27+Proposal
Am 24.08.18, 11:41 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
Ok ... so I think I finished a first Version ... even added a class diagram ... really cool that we sort of get "all-da-pluginzz" at Apache __
https://cwiki.apache.org/confluence/display/PLC4X/Chris%27+Proposal
Chris
Am 24.08.18, 10:14 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi Julian,
done ... you should be able to add your stuff now.
Chris
Am 24.08.18, 09:55 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hey Chris,
sounds like a good idea.
I already registered for the confluence so can you please grant me permissions (julian.feinauer)?
Thanks
Julian
Am 24.08.18, 09:27 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
so I just setup some first confluence pages for our project. The URL to the base of all API Redesign content is here.
https://cwiki.apache.org/confluence/display/PLC4X/API+Redesign
Chris
Am 24.08.18, 09:11 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
how about not doing the redesign in real core right now, but in a more informal way.
We do have a confluence instance, in which we could all whip up our ideas and discuss them here?
Sebastian and I already have accounts, Julian would need to create one. I think I then have the mojo to grant you write permission.
In the end we'll sum up. On what we agreed to here.
Chris
Am 24.08.18, 09:02 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
Ok ... so I think I finished a first Version ... even added a class diagram ... really cool that we sort of get "all-da-pluginzz" at Apache __
https://cwiki.apache.org/confluence/display/PLC4X/Chris%27+Proposal
Chris
Am 24.08.18, 10:14 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi Julian,
done ... you should be able to add your stuff now.
Chris
Am 24.08.18, 09:55 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hey Chris,
sounds like a good idea.
I already registered for the confluence so can you please grant me permissions (julian.feinauer)?
Thanks
Julian
Am 24.08.18, 09:27 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
so I just setup some first confluence pages for our project. The URL to the base of all API Redesign content is here.
https://cwiki.apache.org/confluence/display/PLC4X/API+Redesign
Chris
Am 24.08.18, 09:11 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
how about not doing the redesign in real core right now, but in a more informal way.
We do have a confluence instance, in which we could all whip up our ideas and discuss them here?
Sebastian and I already have accounts, Julian would need to create one. I think I then have the mojo to grant you write permission.
In the end we'll sum up. On what we agreed to here.
Chris
Am 24.08.18, 09:02 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi Julian,
done ... you should be able to add your stuff now.
Chris
Am 24.08.18, 09:55 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hey Chris,
sounds like a good idea.
I already registered for the confluence so can you please grant me permissions (julian.feinauer)?
Thanks
Julian
Am 24.08.18, 09:27 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
so I just setup some first confluence pages for our project. The URL to the base of all API Redesign content is here.
https://cwiki.apache.org/confluence/display/PLC4X/API+Redesign
Chris
Am 24.08.18, 09:11 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
how about not doing the redesign in real core right now, but in a more informal way.
We do have a confluence instance, in which we could all whip up our ideas and discuss them here?
Sebastian and I already have accounts, Julian would need to create one. I think I then have the mojo to grant you write permission.
In the end we'll sum up. On what we agreed to here.
Chris
Am 24.08.18, 09:02 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Julian Feinauer <j....@pragmaticminds.de>.
Hey Chris,
sounds like a good idea.
I already registered for the confluence so can you please grant me permissions (julian.feinauer)?
Thanks
Julian
Am 24.08.18, 09:27 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
so I just setup some first confluence pages for our project. The URL to the base of all API Redesign content is here.
https://cwiki.apache.org/confluence/display/PLC4X/API+Redesign
Chris
Am 24.08.18, 09:11 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
how about not doing the redesign in real core right now, but in a more informal way.
We do have a confluence instance, in which we could all whip up our ideas and discuss them here?
Sebastian and I already have accounts, Julian would need to create one. I think I then have the mojo to grant you write permission.
In the end we'll sum up. On what we agreed to here.
Chris
Am 24.08.18, 09:02 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
so I just setup some first confluence pages for our project. The URL to the base of all API Redesign content is here.
https://cwiki.apache.org/confluence/display/PLC4X/API+Redesign
Chris
Am 24.08.18, 09:11 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
how about not doing the redesign in real core right now, but in a more informal way.
We do have a confluence instance, in which we could all whip up our ideas and discuss them here?
Sebastian and I already have accounts, Julian would need to create one. I think I then have the mojo to grant you write permission.
In the end we'll sum up. On what we agreed to here.
Chris
Am 24.08.18, 09:02 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
how about not doing the redesign in real core right now, but in a more informal way.
We do have a confluence instance, in which we could all whip up our ideas and discuss them here?
Sebastian and I already have accounts, Julian would need to create one. I think I then have the mojo to grant you write permission.
In the end we'll sum up. On what we agreed to here.
Chris
Am 24.08.18, 09:02 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Sebastian Rühl <se...@googlemail.com.INVALID>.
Hi Julian,
Depending on the type of change this might something I could implement to ADS locally if its just a change of the Address format.
Chatting with Chris we stumbled over the size attribute in a ReadRequestItem. This might become obsolete too as in its form right now it isn’t that helpful.
FYI: I was about to read something like this from the plc https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>
What we can see here that we need read 8 words from the plc. We can do that by suppling the IndexGroup/IndexOffset and a Length(ADS) of 16 Bytes. Then I would need to chunk the response into 2 bytes. Im still not sure if this is something I would integrate into the plc4x directly or into a layer above aka JPA/Plc4xJPA (Java Persistence Layer alike) [maybe for the basic structs of ADS it might be worth it]. Then there are complex types which are mixed https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id= <https://infosys.beckhoff.com/index.php?content=../content/1031/tcplclibutilities/html/tcplclibutilities_timestruct.htm&id=>. Here a chunking into 8 would not work as you would need to read fixed different chunks (aligned to it type) from a byte stream. For this I would then use a „RAW“ read and slice it in the application level (or PLC4XJPA).
Maybe working on the Plc4xJPA (working title you name it ;) would be a good idea to get some impression of requirements from that side too (I’ll will scope that on my next TODOs).
What is still missing in your PR suggestion „3) Define 2 types of ItemRequest/-Responses:“. But that might be ok as these changes are an addition to that and as I wrote in the first sentence this might be local to the S7 address format in the first step anyway so in my opinion you are good to go (Maybe leave out the renaming of the query for now to keep the PR footprint a bit lower).
Regarding API design: Its hard. It should be simple yet powerful. So if the first iteration doesn’t fit well we just refactor it :)
Sebastian
> Am 23.08.2018 um 16:48 schrieb Julian Feinauer <j....@pragmaticminds.de>:
>
> Hey Chris,
>
>
>
> yes, this is definitive more complex then what I intended to do.
>
> What I can suggest is that I prepare a PR for the following changes
>
>
>
> 1) Renaming Address to Query
>
> 2) Refactoring of S7 Driver to add
>
> 1) The new Parser
>
> 2) A "rich" S7Query which contains the S7Type and an optional array size
>
> 3) Use this Query everywhere the S7Address is used currently
>
>
>
> And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
>
> Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
>
>
>
> @Chris: Do is sum this up correctly?
>
> @Sebastian: Do you agree with that or what exactly do you need for ADS?
>
>
>
> Julian
>
>
>
> Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
>
>
>
> Hi Julian,
>
>
>
> Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
>
> So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
>
>
>
> And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
>
> I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
>
> So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
>
> But I would prefer the term "Query".
>
>
>
> Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
>
> I think this could make things a lot easier. How about something like this?
>
>
>
> PlcReadRequest request = PlcReadRequest.builder()
>
> .addItem(conn.prepareItem("%DB8.DBX3:INT"))
>
> .addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
>
> .build();
>
>
>
> This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
>
> The second could return a different type of request item.
>
>
>
> Regarding accessing the items inside a response:
>
> Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
>
> so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
>
> but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
>
>
>
> Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
>
>
>
> On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
>
> Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
>
>
>
> Or you just ignore this information totally and process all of it regardless which requestItem it was.
>
>
>
> But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
>
> What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
>
>
>
> I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
>
> What do you think? ...
>
>
>
>
>
> Chris
>
>
>
>
>
>
>
> Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
>
>
>
> Hi Chris,
>
>
>
> (and Sebastian implicitly)
>
>
>
>
>
>
>
> thank you for your response and your suggestions.
>
>
>
> I agree with all your points and I have the impression that our ideas converge more and more.
>
>
>
>
>
>
>
> First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
>
>
>
> But coming back to your points:
>
>
>
>
>
>
>
> 1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
>
>
>
> Address -> Field
>
>
>
> RequestItem -> Is this then still needed?
>
>
>
> Request -> Query / Statement(?)
>
>
>
> 2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
>
>
>
> 3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
>
>
>
> 4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
>
>
>
>
>
>
>
> Object raw = Response.getField("%DB8.DBX3:INT").get()
>
>
>
>
>
>
>
> which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
>
>
>
> The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
>
>
>
>
>
>
>
> Object raw =
>
>
>
> assert raw instanceof Array.class;
>
>
>
>
>
>
>
> As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
>
>
>
>
>
>
>
> SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
>
>
>
> Result res = Response.getMultiResponse()
>
>
>
>
>
>
>
> Is this something like you had in mind?
>
>
>
>
>
>
>
> Best
>
>
>
> Julian
>
>
>
>
>
>
>
> PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
>
>
>
> That way the Code
>
>
>
>
>
>
>
> If (address instanceof S7Address) {// or similar
>
>
>
> myAddr = (S7Address)address;
>
>
>
> }
>
>
>
>
>
>
>
> could become
>
>
>
>
>
>
>
> if (address.isWrapperFor(S7Address.class)) {
>
>
>
> myAddr = address.unwrap(S7Address.class);
>
>
>
> }
>
>
>
>
>
>
>
> Or if you are sure enough simply the "unwrap" part.
>
>
>
> This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
>
>
>
>
>
>
>
> [1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
>
>
>
>
>
>
>
>
Re: Internal Type System
Posted by Julian Feinauer <j....@pragmaticminds.de>.
Hey Chris,
yes, this is definitive more complex then what I intended to do.
What I can suggest is that I prepare a PR for the following changes
1) Renaming Address to Query
2) Refactoring of S7 Driver to add
1) The new Parser
2) A "rich" S7Query which contains the S7Type and an optional array size
3) Use this Query everywhere the S7Address is used currently
And keep the rest of the API as is. This solves my current problem of getting unsigned values from my PLC : )
Then, Sebastian can jump on that for his ADS Implementation and generify from S7 where it makes sense.
@Chris: Do is sum this up correctly?
@Sebastian: Do you agree with that or what exactly do you need for ADS?
Julian
Am 23.08.18, 16:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi Julian,
Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
But I would prefer the term "Query".
Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
I think this could make things a lot easier. How about something like this?
PlcReadRequest request = PlcReadRequest.builder()
.addItem(conn.prepareItem("%DB8.DBX3:INT"))
.addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
.build();
This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
The second could return a different type of request item.
Regarding accessing the items inside a response:
Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
Or you just ignore this information totally and process all of it regardless which requestItem it was.
But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
What do you think? ...
Chris
Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hi Chris,
(and Sebastian implicitly)
thank you for your response and your suggestions.
I agree with all your points and I have the impression that our ideas converge more and more.
First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
But coming back to your points:
1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
Address -> Field
RequestItem -> Is this then still needed?
Request -> Query / Statement(?)
2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
Object raw = Response.getField("%DB8.DBX3:INT").get()
which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
Object raw =
assert raw instanceof Array.class;
As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
Result res = Response.getMultiResponse()
Is this something like you had in mind?
Best
Julian
PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
That way the Code
If (address instanceof S7Address) {// or similar
myAddr = (S7Address)address;
}
could become
if (address.isWrapperFor(S7Address.class)) {
myAddr = address.unwrap(S7Address.class);
}
Or if you are sure enough simply the "unwrap" part.
This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi Julian,
Well we have one request, but this can contain multiple items each item has a "query/address/statement" to define WHAT should be read.
So we definitely need the items or we would have to fire 1000 requests in order to read 1000 values.
And regarding the "Field" name ... the thing is that it's not just an Address, but also information on how to interpret and stuff like that ...
I could even imagine that we could even - one day maybe - introduce more complex stuff like this: "MAX($DB1.$W54:INT; 256)" ... but that's just a crazy idea.
So I would also be ok with "Statement" ... especially as we could call the parseAddress method: "prepareStatement" which definitely rings a bell ;-)
But I would prefer the term "Query".
Maybe it would even be better to replace parseAddress with something that doesn't produce Address objects, but RequestItems instead.
I think this could make things a lot easier. How about something like this?
PlcReadRequest request = PlcReadRequest.builder()
.addItem(conn.prepareItem("%DB8.DBX3:INT"))
.addItem(conn.prepareItem("%DB5.DBW5:INT[4]"))
.build();
This way the first address could produce a single-value Request item (Or a multi-value one - just as we have it now - one with size = 1) and
The second could return a different type of request item.
Regarding accessing the items inside a response:
Well if you have multiple items in a request and you want to identify the response of a particular one, you sort of have to pass in something
so the system can decide which one you want. The other option would be to have the request item have a reference to a response item future,
but then we could only use one request once. This way we can prepare one request and keep on using that to produce multiple responses.
Passing in the request item for getting the corresponding responseItem just seemed reasonable for this task.
On the other side, you could also just take all items and use their "getRequestItem" method to find out what the current item is ... the API allows this for exactly this reason.
Otherwise the response item wouldn't have to be linked with the requestItem it belonged to.
Or you just ignore this information totally and process all of it regardless which requestItem it was.
But just having a look at getValue in PlcResponse, I don't quite like the idea of streaming through everything and filtering every time. I would prefer a Map ...
What we use as Key ... well I'm not that focused on the requestItems, but I do greatly prefer them over Strings.
I think we should split this up into multiple independent refactorings and start with the Request side as I fear we are sort of doing a breadth-first discussion.
What do you think? ...
Chris
Am 23.08.18, 15:16 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hi Chris,
(and Sebastian implicitly)
thank you for your response and your suggestions.
I agree with all your points and I have the impression that our ideas converge more and more.
First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
But coming back to your points:
1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
Address -> Field
RequestItem -> Is this then still needed?
Request -> Query / Statement(?)
2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
Object raw = Response.getField("%DB8.DBX3:INT").get()
which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
Object raw =
assert raw instanceof Array.class;
As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
Result res = Response.getMultiResponse()
Is this something like you had in mind?
Best
Julian
PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
That way the Code
If (address instanceof S7Address) {// or similar
myAddr = (S7Address)address;
}
could become
if (address.isWrapperFor(S7Address.class)) {
myAddr = address.unwrap(S7Address.class);
}
Or if you are sure enough simply the "unwrap" part.
This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
Re: Internal Type System
Posted by Julian Feinauer <j....@pragmaticminds.de>.
Hi Sebastian,
as we also need this feature for an use case I am willing to implement this ASAP but I want to make sure how to do it properly to avoid long conceptual discussions when a PR is available.
Julian
Am 23.08.18, 15:53 schrieb "Sebastian Rühl" <se...@googlemail.com.INVALID>:
Hi Together,
JUST FYI:
Having worked on ADS (payload was an open topic) today I ran into the same problems like you with the S7.
So to make it work properly the driver here also would require the proposed changes on „Address“.
So all in all we should start soon implementing the changes.
Sebastian
Re: Internal Type System
Posted by Sebastian Rühl <se...@googlemail.com.INVALID>.
Hi Together,
JUST FYI:
Having worked on ADS (payload was an open topic) today I ran into the same problems like you with the S7.
So to make it work properly the driver here also would require the proposed changes on „Address“.
So all in all we should start soon implementing the changes.
Sebastian
Re: Internal Type System
Posted by Julian Feinauer <j....@pragmaticminds.de>.
Hi Chris,
(and Sebastian implicitly)
thank you for your response and your suggestions.
I agree with all your points and I have the impression that our ideas converge more and more.
First, I thank you for your review and this is not intended as productive code was a vehicle for me to test the concept and see where changes are necessary (and you are simply moving to fast with the master... :> ).
But coming back to your points:
1) I agree with the change, but query also seems unintuitive to me, something like "field" would be better but the request could be renamed to query or statement (to keep it somewhat similar to jdbc). So it would be
Address -> Field
RequestItem -> Is this then still needed?
Request -> Query / Statement(?)
2) I see how you come to this but I'm unsure if I like it. On the other hand the current way also has it flaws (as I specify an offset which is only used for the first item).
3) Sounds reasonable for me (we are usually also more on the second use case) thus I like expecially the second variant.
4) Personally, I find the current Response "API" too verbose as I need to give it (in the multi case) the Request to get my results and all that stuff. Thus I definitely support changes here but I'm a bit unsure how the optimal API should look. I would like the idea of getting them by "field" references (see above), i.e., something like
Object raw = Response.getField("%DB8.DBX3:INT").get()
which would make it easier to use it from a config (otherwise one would probably store a Map<ConfigItem,RequestItem>) and do this unnecessary indirection.
The only part where I'm uncertain here is how to incorporate the "Array" requests. The most natural thing would be to return an Array of Objects in that situation. I.e.,
Object raw = Response.getField("%DB8.DBX3:INT[4]").get();
assert raw instanceof Array.class;
As (see my comment to 1) I would also prefer the name "Result" or even ResultSet for its convenience from jdbc perhaps we could introduce the result and use the Response as Result Factory, i.e.,
SingleResponse<T> res = Response.getSingleResponse<>(Class<T> clazz)
Result res = Response.getMultiResponse()
Is this something like you had in mind?
Best
Julian
PS.: Regarding the wrapper pattern, I meant to add it to the Address / Field interface.
That way the Code
If (address instanceof S7Address) {// or similar
myAddr = (S7Address)address;
}
could become
if (address.isWrapperFor(S7Address.class)) {
myAddr = address.unwrap(S7Address.class);
}
Or if you are sure enough simply the "unwrap" part.
This is something between syntactic sugar and the visitor pattern which is heavily used in jdbc (and Apache Calcite [1]) to come from the generic interfaces to the Implementation specific implementing classes (e.g. Driver, Connection, Statement, ...).
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/schema/Wrapper.java
Am 23.08.18, 10:24 schrieb "Christofer Dutz" <ch...@c-ware.de>:
Hi all,
I was just discussing things here with Sebastian, as he was just sitting next to me.
Now I'm writing down what we were talking about.
There were multiple things we both not that happy with and here's our proposal:
1) Rename Address to Query (Naming could be discussed)
2) Adding Type and Array Size (Optional) to the query string ($DB1.$W54:UINT[4])
3) Define 2 types of ItemRequest/-Responses:
1) Containing generic type information: Able to do sanity-checks when parsing, response item contains only a generic get() or get(itemNumber) methods
2) Without generic type information: No sanity check. Saves data in a generic way (maybe byte-array) and provides type-accessors: getInteger(), getDouble(), getFloat(),...
4) Splitting up response items in single item responses and multi item responses
Now to the reasoning:
1) At the beginning we only had Addresses. But now we are extending the simple Addresses with information on how to interpret the address. This no longer feels like a pure "address".
2) Well adding the explicit type to any former address (I would suggest to do it the same way for every driver) ... I think we discussed the need for this. The next thing was that when using the API the size/length field and how we access these values sort of felt uncomfortable. Therefore I would like to use the opportunity to add the size to the query string ... in above example the UINT[4] would mean that 4 UNIT values should be read (and not 4 bytes).
3) The two types of request (and matching response items) would simply help the two types of users: I for my part more construct large multi-item ReadRequests where handling all the generic types does make things more complicated for me. I prefer to have items and to access them with typed accessors. On the other side when accessing simpler or single-item requests, there is a great benefit for code simplicity.
4) When using the response items it always felt strange to have these convenience single-item-accessors which throw exceptions if the response is multi-item.
Now to your suggestions:
I think we are both on the same page conceptually ... think all I am proposing, is to add the array access to the string and to rename the entire concept.
Especially I think we both agree on storing the information in the response-item in byte-form and to cast that to the desired type when needed.
Regarding the wrapper pattern ... I don't know if I understood this. But to me it looks as if we would have to do type-checks depending on the parameter type ... so we have to explicitly write a conditional branch for every supported type and throw an exception for not-supported ones. So if I use:
responseItem.unwrap(Hurz.class)
This would probably not work ... or did I just get the concept wrong?
I did have a look at your changes and have some observations:
1) Your fork could need updating ;-)
2) Your version of TypedAddress is very focused on S7 as for example an ADS address could be a simple string (Symbolic addressing), for EtherNet/IP there is no such byte-offset and bit-offset, there's just ClassId+InstanceId[:AttributeId] ... also doesn't it support reading of arrays ... I think we should leave the Address in it's simple state as a simple place-holder.
3) S7BitsWith ... well S7 uses the name of a Data-Transport-Size ... but I think in our case the X,B,W,D and L are syntactic sugar that should be used while parsing a query- or address string. We shouldn't even need it afterwards.
4) The S7Type is generally ok the way it is however, should we add all the types and add which S7 types support them. For example an LINT will only work on S7 1500 and 1200. If you update your Fork, you should be able to see the S7ControllerType and S7DataType classes, that I added some time ago. It's sort of an aggregate of your S7Type and S7BitsWidth (I think) ... please have a look at that and tell me what you think.
And regarding your question ... you can start discussion threads whenever you want ... usually you don't have to flag it that way, but it certainly doesn't hurt. In other projects we usually have a "[DISCUSS]" thread alongside a "[VOTE]" thread, so the actual voting isn't mixed with discussions about what's being voted on. I tried to start topics that touch a certain area with a []-prefix to trigger people for example with EtherNet/IP knowledge. So as I said ... there's no formal process on Discussion threads ... only [VOTE] threads should be reserved to actual things voted on.
Chris
Am 22.08.18, 16:19 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hey all,
we had several discussions about type handling [1,2] in general and in drivers and especially the addition of type information for the S7 Driver [3].
I am currently implementing the “typed” addresses for the S7 Driver [3] and wanted to propose the approach there as it could form the basis of the general “internal” typesystem for all drivers.
Motivation:
Provide all drivers with a common type system to reduce the amount of driver specific code for “type” handling.
The approach is the following:
I added an abstract class TypedAddress which implements the marker Interface Address (which is returned from the Driver).
TypedAddress contains all necessary “generic” information to be able to decode values from a byte[] array, i.e., the offset for byte / bit, the number of bits used by the value and a Category.
Category encapsulates the “logical type”, i.e., if the value is INTEGER or DECIMAL, SIGNED or UNSIGNED, STRING, DATE or whatsoever.
For the S7 driver the TypedAddress is extended by the TypedS7Address which adds S7 Specific properties like memory area.
In case of the S7 flow all subsequent usages of Address in the lower layers (XXXProtocol) are made with regards to the TypedS7Address.
Furthermore, I suggest to use the Wrapper pattern for the address interface, see e.g. [5] from java.sql as it makes these instance-checks in the driver code “nicer”.
What do you think of these changes, do you agree with the approach I use?
@Sebastian
I had a look through the Ads Implementation and I think this approach would be feasible for the ads implementation too. Is this correct?
@Chris
I’m lacking the feeling of when to start a [DISCUSSION] or how the general guidelines are here and especially in this project so I’m thankful for advice
Best
Julian
[1] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3C1B180308-0BFA-4203-869C-ABE64D77B203%40c-ware.de%3E
[2] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3CF8E442F3-906C-47D6-9F08-A393F6D11557%40pragmaticminds.de%3<http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3CF8E442F3-906C-47D6-9F08-A393F6D11557%40pragmaticminds.de%253>E
[3] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201808.mbox/%3C481F0FC1-0A9B-44F8-B594-C8E825EB5026%40c-ware.de%3E
[4] https://github.com/JulianFeinauer/incubator-plc4x/tree/add-typed-addresses-s7
[5] https://docs.oracle.com/javase/7/docs/api/java/sql/Wrapper.html
Re: Internal Type System
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
I was just discussing things here with Sebastian, as he was just sitting next to me.
Now I'm writing down what we were talking about.
There were multiple things we both not that happy with and here's our proposal:
1) Rename Address to Query (Naming could be discussed)
2) Adding Type and Array Size (Optional) to the query string ($DB1.$W54:UINT[4])
3) Define 2 types of ItemRequest/-Responses:
1) Containing generic type information: Able to do sanity-checks when parsing, response item contains only a generic get() or get(itemNumber) methods
2) Without generic type information: No sanity check. Saves data in a generic way (maybe byte-array) and provides type-accessors: getInteger(), getDouble(), getFloat(),...
4) Splitting up response items in single item responses and multi item responses
Now to the reasoning:
1) At the beginning we only had Addresses. But now we are extending the simple Addresses with information on how to interpret the address. This no longer feels like a pure "address".
2) Well adding the explicit type to any former address (I would suggest to do it the same way for every driver) ... I think we discussed the need for this. The next thing was that when using the API the size/length field and how we access these values sort of felt uncomfortable. Therefore I would like to use the opportunity to add the size to the query string ... in above example the UINT[4] would mean that 4 UNIT values should be read (and not 4 bytes).
3) The two types of request (and matching response items) would simply help the two types of users: I for my part more construct large multi-item ReadRequests where handling all the generic types does make things more complicated for me. I prefer to have items and to access them with typed accessors. On the other side when accessing simpler or single-item requests, there is a great benefit for code simplicity.
4) When using the response items it always felt strange to have these convenience single-item-accessors which throw exceptions if the response is multi-item.
Now to your suggestions:
I think we are both on the same page conceptually ... think all I am proposing, is to add the array access to the string and to rename the entire concept.
Especially I think we both agree on storing the information in the response-item in byte-form and to cast that to the desired type when needed.
Regarding the wrapper pattern ... I don't know if I understood this. But to me it looks as if we would have to do type-checks depending on the parameter type ... so we have to explicitly write a conditional branch for every supported type and throw an exception for not-supported ones. So if I use:
responseItem.unwrap(Hurz.class)
This would probably not work ... or did I just get the concept wrong?
I did have a look at your changes and have some observations:
1) Your fork could need updating ;-)
2) Your version of TypedAddress is very focused on S7 as for example an ADS address could be a simple string (Symbolic addressing), for EtherNet/IP there is no such byte-offset and bit-offset, there's just ClassId+InstanceId[:AttributeId] ... also doesn't it support reading of arrays ... I think we should leave the Address in it's simple state as a simple place-holder.
3) S7BitsWith ... well S7 uses the name of a Data-Transport-Size ... but I think in our case the X,B,W,D and L are syntactic sugar that should be used while parsing a query- or address string. We shouldn't even need it afterwards.
4) The S7Type is generally ok the way it is however, should we add all the types and add which S7 types support them. For example an LINT will only work on S7 1500 and 1200. If you update your Fork, you should be able to see the S7ControllerType and S7DataType classes, that I added some time ago. It's sort of an aggregate of your S7Type and S7BitsWidth (I think) ... please have a look at that and tell me what you think.
And regarding your question ... you can start discussion threads whenever you want ... usually you don't have to flag it that way, but it certainly doesn't hurt. In other projects we usually have a "[DISCUSS]" thread alongside a "[VOTE]" thread, so the actual voting isn't mixed with discussions about what's being voted on. I tried to start topics that touch a certain area with a []-prefix to trigger people for example with EtherNet/IP knowledge. So as I said ... there's no formal process on Discussion threads ... only [VOTE] threads should be reserved to actual things voted on.
Chris
Am 22.08.18, 16:19 schrieb "Julian Feinauer" <j....@pragmaticminds.de>:
Hey all,
we had several discussions about type handling [1,2] in general and in drivers and especially the addition of type information for the S7 Driver [3].
I am currently implementing the “typed” addresses for the S7 Driver [3] and wanted to propose the approach there as it could form the basis of the general “internal” typesystem for all drivers.
Motivation:
Provide all drivers with a common type system to reduce the amount of driver specific code for “type” handling.
The approach is the following:
I added an abstract class TypedAddress which implements the marker Interface Address (which is returned from the Driver).
TypedAddress contains all necessary “generic” information to be able to decode values from a byte[] array, i.e., the offset for byte / bit, the number of bits used by the value and a Category.
Category encapsulates the “logical type”, i.e., if the value is INTEGER or DECIMAL, SIGNED or UNSIGNED, STRING, DATE or whatsoever.
For the S7 driver the TypedAddress is extended by the TypedS7Address which adds S7 Specific properties like memory area.
In case of the S7 flow all subsequent usages of Address in the lower layers (XXXProtocol) are made with regards to the TypedS7Address.
Furthermore, I suggest to use the Wrapper pattern for the address interface, see e.g. [5] from java.sql as it makes these instance-checks in the driver code “nicer”.
What do you think of these changes, do you agree with the approach I use?
@Sebastian
I had a look through the Ads Implementation and I think this approach would be feasible for the ads implementation too. Is this correct?
@Chris
I’m lacking the feeling of when to start a [DISCUSSION] or how the general guidelines are here and especially in this project so I’m thankful for advice
Best
Julian
[1] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3C1B180308-0BFA-4203-869C-ABE64D77B203%40c-ware.de%3E
[2] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3CF8E442F3-906C-47D6-9F08-A393F6D11557%40pragmaticminds.de%3<http://mail-archives.apache.org/mod_mbox/plc4x-dev/201807.mbox/%3CF8E442F3-906C-47D6-9F08-A393F6D11557%40pragmaticminds.de%253>E
[3] http://mail-archives.apache.org/mod_mbox/plc4x-dev/201808.mbox/%3C481F0FC1-0A9B-44F8-B594-C8E825EB5026%40c-ware.de%3E
[4] https://github.com/JulianFeinauer/incubator-plc4x/tree/add-typed-addresses-s7
[5] https://docs.oracle.com/javase/7/docs/api/java/sql/Wrapper.html