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
