Hi Julian,
done ... you should be able to add your stuff now.
Chris
Am 24.08.18, 09:55 schrieb "Julian Feinauer" <j.feina...@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" <christofer.d...@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"
<christofer.d...@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"
<sebastian.ruehl...@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.feina...@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"
<christofer.d...@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.feina...@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
>
>
>
>
>
>
>
>
