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" <christofer.d...@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.feina...@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
