Hi Chris,
yes, JSR 310 definetly made Java a better place, the only problem is that you than tie yourself to Java 8. But if everybody else agrees we can do that. For us this is no problem as we are on Java 8 naturally but I dont know about others. And I also agree with your argument regarding the Byte. The only thing I had in mind is that the interface is somewhat "overwhelming". I see your point with the generic getter and we can also skipt then everybody who wants something like that shoult take the "get()" method and do the cast on its own risk. Julian ________________________________ Von: Christofer Dutz <christofer.d...@c-ware.de> Gesendet: Montag, 20. August 2018 20:50:00 An: dev@plc4x.apache.org Betreff: Re: Refactoring of Drivers Hi Julian, well I would definitely not remove the getByte as this is sort of a core data-type for PLC programming. Also I think most of the others shouldn't come with much payload. If we have the largest type behind the scenes, the "asInteger" or "asLong" sort of come for free. Not quite sure about the generic getter as it will probably produce ClassCastExceptions or require quite some reflection or cascades of instanceof statements. Regarding the Dates and times ... doing some reading it seems that both Date and Calendar seem to be pretty flawed and the "new kid on the block" is JSR 310 (Date & Time API) Found a good German article on this: https://www.heise.de/developer/artikel/Die-neue-Date-Time-API-in-Java-8-2198399.html?seite=all I wish I had had a look at this before just now ;-) ... I especially like the "LocalDate", "LocalTime" and "LocalDateTime" seems with this the natural differentiation between Date and Time is now possible without hacks. Chris Am 20.08.18, 17:02 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hi Chris, I totally agree with all you are writing. We do not necessarily need a isXXX method if we provide a method Object get() and everyone is able to handle it "generic" downstream. I also agree that Calendar is reasonable the only think I wanted to point out is that we should have a discussion about the API and which are the default types we offer. In our current implementation we use only the "widest" types, i.e., Long and Double because we are not that much constricted with resources and it eases some things. For me the interface could look like getByte()* getShort()* getInt() getLong() getFloat()* getDouble() getBoolean() getString() getDate() // Should we use Calendar or LocalDate? Java 8 DateTime >> Java.util.date but what about downwards compatibility? <T> T get(Class<T> clazz) Object get() * = could be omitted from MY perspective What do you think? Julian Am 20.08.18, 16:30 schrieb "Christofer Dutz" <christofer.d...@c-ware.de>: Hi Julian, do we really need the "isXYZ" methods? Cause even if a SHORT is read, I should still be able to get a "Short", "Integer", "Long", "Float", "Double", ... from that. Ok if I call getShort() on a String, this should produce a Exception. But this is an exception thrown because we are doing something silly and it would produce the same Exception for all protocols. I also think we should have a set of types defined by the API ... The driver knows how to interpret this information and convert it to a set of basic Java types. I doubt that there is any PLC with a local-datetime as I have never come across one that would support that. So returning Calendar is a good option. Per default it has the default time-zone, if a plc would make timezone information available, additionally the timezone could be set. I would also prefer to use Calendar as default type for Dates and Time as using the Date is being seen as deprecated. Of course would we have to change something in the API. This was the reason, why I was so hesitant to do a first release as I knew that there will be changes and I wante to do most of the big ones before a firtst release. Chris Am 20.08.18, 11:31 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hi Chris, I didnt mea nto ignore GC at all but I usually hold it with Donald E Knuth (Preliminary optimization ist he root of all evil) and I am careful to base a desgin on such considerations except there are really good reasons for. I also dont like throwing an Exception at all, we need a better solution. But what I can imagene in our use cases is that we have a DB Table containing lots of "Adresses" and frequencies and then scraping goes on. And for us it would be best to get a Hint about the type from the address object itself (It knows the type) or from the ReadResponse as you sggest. What I would not like would be to store the Connection Info, the Address and a Return Type in my config. Thus for us it would also be viable to have methods like isInteger isDouble isBoolean which indicates wheter one can call the respective typesafe getter. And by CustomTypes I did not meant this "JPA" idea I meant that we should then agree on a fixed catalog of supported "basic" types. For example in the ADS Implemenentation Calendar is used. And we should add a getter for all these basic types but if someone would e.g. need a LocalDateTime or something we have no real possibility to support that, expect possibly something like <T> T get(Class<T> clazz) But overall I could agree on Solution which makes the ReadResponse as a value Object with several "typed" Getters as this comes pretty close to our current solution. But this would mean to consider a major API Change, or? Julian Am 20.08.18, 10:35 schrieb "Christofer Dutz" <christofer.d...@c-ware.de>: Hi Julian, I wouldn't like to throw exceptions for individual drivers as this sort of undermines the "code against any PLC you want" paradigm. Isn't a ReadResponseItem exactly such a "Value" object? And I do care about keeping the GC do as little as possible work, as especially on Edge devices this could be a problem due to the nature of limited CPU and Memory resources. We could implement the getLong etc. as default implementations. Custom Types is probably going to be a challenge and currently I haven't invested too much time in them. But just for the sake of ensuring that we are talking about the same thing: You are probably talking about something like "struct" in C. So I request something that is returned as a custom data structure instead of requesting each item individually ... correct? While working on the EtherNet/IP protocol, I just came across this problem (if you only provide classId and instanceId, this should be able to return all the attributes of this instance. Maybe it would be cool to solve this problem a little more generic. Remember this "JPA" layer on top of the driver we were talking about? You could register Data structures with the Driver and instead of a response containing multiple response-items, you would get one POJO back. Now the driver could optimize this internally. So if a driver supports custom types, this feature is used, if it doesn't the items are individually requested. Just some thoughts on this. Chris Am 20.08.18, 10:09 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hi Chris, I am thinking about this, basically since I saw the TypesafeRequestItem. I'm not sure about giving it up as it is a really central part of the API but on the other hand Java Generics make live hard from time to time. What we currently do in our implementation is to hide away the internal java type behind a "Value" Interface whch has typesafe getters as you state it. This gives all flexibility as each Value Implementation can decide which getters to implement and which not. On the other hand the main drawbacks are 1. If you don't provide a getter you throw an Exception which is pretty bad for the caller, if he is uncertain about your type 2. For very high throughputs you put some pressure on GC as you allocate a LOT of short lived objects (this should be minor in our case). Another note with regards to the ADS Implementation is that there are also other types which can be requested (Some Specific Date Format, I think) which we should support also. Currently I would prefer a "bridge" with an interface like <T> T getAs(Class<T> clazz, Object value) Which does the conversion with "instanceof" checks. On the other hand, all of these approaches do not solve the Custom Types problem I think. Last time when thinking about this I developed the Idea of this pluggable Encode / Decoder Factory : ) @Chris: Do you have an Idea how we could make your idea work with 1. The possibility for the caller to get a hint about the types he can get and 2. Also for custom types? @Sebastian: You do use custom types in ADS, dont you? Best Julian Am 20.08.18, 09:57 schrieb "Christofer Dutz" <christofer.d...@c-ware.de>: Hi Julian, I was thinking about exactly this, this morning in the train on my way to work. An idea I had was to eventually entirely get rid of the TypesafePlc requests and to have 3 or 4 types of items. In general a: IntegerItem FloatingPointItem DateItem StringItem ... "Integer" in the IntegerItem not relating to the Java Integer, but more the "Non-Floating-Point Numeric Value". Or even merge the numeric values together and provide accessors in the desired type. Something like this: numericItem.getFloat() numericItem.getInteger() numericItem.getFloat() numericItem.getDouble() ... Or even: numericItem.getBooleanArray() ... The payload would always be provided in a normalized form that is capable of carrying the largest value of that type. So the dirvers would produce normalized items and the items themselves contain what's needed to convert that to one of the other types. Good idea? Or not so good ... at the moment I sort of like it, but I might not have thought about everything. Chris Am 20.08.18, 09:47 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hi Chris, okay, then I had a wrong assumption. Previously, I worked a lot with measurement file formats from automotive industry and there you can encounter all and everything in one file. If it is as you states then I agree that a small set of static util methods will do the job. Only one question remains for me: How do we deal with the "requested" type and the "interal Java Type". As example, all of these statements should be valid, I think: RequestItem<Short.class>("DB4.DW4:INT") RequestItem<Integer.class>("DB4.DW4:INT") RequestItem<Long.class>("DB4.DW4:INT") But the internal LEConverter.to2ByteInt(...) would return something like a Short.class. Wouldn't we need to do the casts explicit as otherwise we could receive a ClassCastException or something like that? Best Julian Am 20.08.18, 09:16 schrieb "Christofer Dutz" <christofer.d...@c-ware.de>: Hi Julian, well I don't think this is that much of a problem. Every driver type (and sometimes depending on the capabilities of the remote) will map one set of types to - let's call them internal Java types. If one protocol uses LE, it will for all of its supported types. I have never come across a protocol, where some are LE and some are BE. So it's rather: // Inside the code of driver XYZ Switch(type) { case LINT: byte[] bytes = // read 4 bytes return LEConverter.toIeeeFloatingPoint(bytes); case UINT: ... } Or something like that ... What do you think? Chris Am 17.08.18, 17:03 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hi Chris, this is exactly the idea I have in mind. For me in your step 1 I again see two steps. First, you have the decoding logic (e.g. your public static float parseIEEE754Float(byte[] in)) and then you have all this ugly branching like If (bytes.length = 4) { If (isDecimal()) { If (isBigEndian()) { // Here call the static helper method } } } which I would love to avoid (and which makes it perhaps more comfortable for users to implement their drivers). But perhaps I am making things to complicated and we usually have only a small set of possibilities. Then i agree that we could keep things as they are. Best Julian Am 17.08.18, 15:38 schrieb "Christofer Dutz" <christofer.d...@c-ware.de>: Hi Julian, to me it sounds like two separate things: 1) Decoding what's coming from the outside 2) Converting the decoded types to other types as far as that's possible So the driver should know what the bytes mean that come from the PLC and on top of that we could convert that into something else. We would need such a two phase conversion to do that anyway, otherwise we would sort of need the cartesian set of all combinations of converters. I do agree that this "interpret this integer as a Boolean", or "translate this float into an int" sounds universally usable. Correct? Chris Am 17.08.18, 15:03 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hi Chris, you are right with what I want to do, let me explain my motivation. Your example is right but I think there are many situations where this is not sufficient, especially with regards to the new Address Syntax for S7. Basically the new syntax allows me to state something like this: "I know that the value is 2 byte unsigned integer in little Endian Order and I want it back as Long". So the first idea was that I wanted to avoid having many methods for all combinations of Endianness, bit-length, Signed / Unsigned and Decimal / Float. And the second idea to also provide narrowing or widening or even conversion out of the box. I came across this issue when thinking about the migration of the current conversion in the S7 driver which is like a large if (XXX.class.isInstance(...)) else... and thought it would be better for the drive to just say something like parse(Class<?> target, byte[] in, Representation repr) to avoid the m times n problem for Java Types and byte Representation. But if you (and the other driver implementors) do not see this concern that much I can also shift my effort to something else. Best Julian Am 17.08.18, 14:41 schrieb "Christofer Dutz" <christofer.d...@c-ware.de>: Hi Julian, please let me repeat how I understood your proposal: You observed that in multiple drivers the conversion between byte-array data to the actual Java type is pretty similar and would like to wrap that mapping code in some commonly shared code base? I agree ... if a float is transformed as IEEE 754 Floating Point, it doesn't matter what driver this belongs to. But on the other side the code for doing this conversion too isn't that complex. I think in this case eventually even a class with static methods should be enough... sort of public static float parseIEEE754Float(byte[] in); public static int parseLE32BitInt(byte[] in); ... Maintaining a registry component that has to be injected into the drivers of type conversions where drivers can register custom converters sounds a little overkill to me. If a driver requires other conversions, it can implement them itself and if it makes sense to add them to the driver-base version, that code is simply moved there. What do you think? Chris Am 17.08.18, 14:14 schrieb "Julian Feinauer" <j.feina...@pragmaticminds.de>: Hey al, I like to open another discussion as I am currently working on another refactoring of the Drivers, namely the extraction of "binary" encoders and decoders as common concern. After our discussion about the addition of the binary representation to the S7 driver I observed that several drivers use very similar code to transform java types to byte representations of specific flavor (Big Endian, ...). Thus my aim is to provide a “library” of common encoders and decoders between Java Types and byte representations that every driver can use but also register custom Java Types and their representation (as it is e.g. needed for ADS, I think). Do you agree with this? Julian
