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
