Api Change - Chris Copy

General

  • I ensured all of our classes have the "Plc" prefix, as I did encounter situations in which very generic type names had collisions and I had to use the fully qualified class name (including the package name). The "Plc" prefix reduces this risk.
  • Introduced a PlcFieldRequest / Response classes, allowing PlcRequests and Responses that don't use field information (When calling functions, for example)
  • Removed the "RequestItems". Now the data for the plc read responses is contained in the PlcReadResponse/PlcWriteRequest itself.
  • Added the concept of a named-field which is in general a Field description combined with a string alias.
  • All sub-types of the PlcFieldRequest/Response use these aliases to reference the field they want to access. Resulting in an API that is more similar to JDBCs RecordSet.

PlcField (Aka Address)

  • I Renamed Address to PlcField
  • I still think there is no immediate need to define any fields or methods in this interface as I haven't seen any common ground yet. Even if we are currently adding "TYPE" information to most fields, in Modbus the type is directly tied to the address you are reading. So If you are reading a "Register" or a "Coil", the datatype is fixed.

PlcNamedField

  • This class is only used for constructing the PlcFieldRequest objects as it allows passing in pairs of name and field into the PlcFieldRequests constructor.

PlcConnection

  • I removed the "getLister" method as I haven't seen any protocol or use-case where this actually made sense.

  • I added a getFunctionExecutor method that should return a component for executing functions on PLCs that support this.
  • I changed the "parseAddress" to "prepareField". This abstract method has to be implemented by every Driver and it handles parsing of the field description for each protocol. Some times I could imagine, that more work is involved than simple parsing of a string (For Beckhoff ADS and EtherNet/IP this method could actively connect to the corresponding resource so in future read operations only the connection-id has to be provided)

PlcRequest & PlcResponse

  • As mentioned before, I introduced a new Level PlcFieldRequest/Response which deals with request/responses which target fields (such as read, write, subscribe).
  • The requests contain a Map of Named-Fields
  • The responses contain a reference to the corresponding request
  • The responses contain a response code for every field in the request
  • A PlcReadResponse also contains a map of byte data that contains the raw data.
  • (The data should be in a general format shared by all drivers, so the drivers requiring special formats need to convert this when constructing the objects)

PlcReadResponse & PlcWriteRequest

  • The getObject() is a convenience method to return values of the natural type determined by the field definition itself. This is the smallest possible type fit to contain the PLC type.
  • The setObject() is a convenience method. Actually thinking about it a little bit more, we might even have just the setObject version and get rid of the rest.
  • A "typeConverter" is passed in by each driver, that handles the encoding/decoding of values for that particular driver.

Example Code

Here comes some example code to demonstrate how a Programm using this API could look like.

public static void main(String[] args) throws Exception {
    PlcDriverManager manager = new PlcDriverManager();
    try (PlcConnection conn = manager.getConnection("s7://192.168.167.211/0/0")) {
        Optional<PlcReader> optionalReader = conn.getReader();
        if (!optionalReader.isPresent()) {
            return;
        }
 
        PlcReader Reader = optionalReader.get();
 
		// Example using prepareField and prepareNamedField ...
        PlcReadRequest request = new PlcReadRequest(
            conn.prepareNamedField("parameterX", "%DB8.DBX3:INT"),
            conn.prepareNamedField("parameterY", "%DB9.DBW4:DOUBLE"),
            new PlcNamedField("others", conn.prepareField("%DB2.DBW2:BYTE[10]")));
 
        CompletableFuture<PlcReadResponse> future = reader.read(request);
 
        PlcReadResponse response = future.get();
 

        // Access an item by it's name.
        System.out.println("Parameter X: " + response.getInteger("parameterX"));
 
        // Iterate over all items
        for(String fieldName: response.getFieldNames()) {
			switch(fieldName) {
				case "parameterY": 
	                System.out.println("Parameter y: " + response.getDouble(fieldName));
					break;
				case "others": 
	                for(int i = 0; i < response.getNumValues(fieldName); i++) {
    	                // Intentionally using short instead of byte
        	            // In this case it will return Byte[i] but convert that into a Short.
            	        System.out.println(" - " + response.getShort(fieldName, i));
                	}
					break;
			}
        }
    }
}




  • No labels

11 Comments

  1. Julian Feinauer

    Hi Chris,

    I like many of your changes, personally I think we are on a very good way.

    Thanks for putting all this together!

    For the discussion, here are some things that came to my mind while going through your UML / Example Code:

    1. Do we really need the Reader / Writer? Perhaps it becomes clearer to me when you explain your design considerations behind this. One alternative which I would like more would be to have two different methods for the driver (again think of Jdbc with executeQuery(), executeStatement(), ...), e.g., conn.read(...) and conn.write(...). Furthermore, all drivers would have to implement two abstract methods canWrite and canRead (Apache Drill does this for its DataSources) to Signal if one can read / write to this connection.
    2. I am missing one method in your getXXX / setXXX Methods for the RequestItems / ResponseItems, namely the getObject(): Object. This is good four our use case where we want to simply take the value as object and later on somewhere use an instanceof to get the concrete type. The getRawData method is not suitable as the data is still encoded and we dont want to bother with that (Plc4J should do that for us!).
    3. I suggest to rename getRawData to getRaw, seems more natural to me (but its personal flavor, so just a suggestion)
    4. In your Code Example I would like an overloaded prepareField(name, fieldString) more to create a named field implicitly.

    Is this the right place here for the discussion?

    1. Unknown User (cdutz)

      Hi Julian,

      1) The reason for the PlcReader and PlcWriter (and the others) is that in contrast to JDBC (where every driver supports almost the same set of features, with PLCs there might be great difference. We had discussed in the past, if we should have the driver contain all operations and just return errors if writing isn't supported, but I prefer adding one layer and keeping the connection itself as simple as possible. 

      I did experiment with a "prepareQuery" and "executeQuery" method that could parse a String query similar to JDBC does and to return PlcRequest objects depending on the query string ... so if you prepare a query with "SELECT 'FieldAddress' AS name, ...." it produces a PlcReadRequest and with "UPDATE ''FieldAddress' = :name, ..." it would produce a PlcWriteRequest. The problem would be that from the API, all would be a PlcRequest and an "execute" method would return a PlcResponse. However now we would have to cast that to the more concrete types in order to do anything with that response.

      2) The reason I didn't add a getObject, was that I thought that having the payload "raw" would allow us having the simplest code. If we had the value in form of Object (this being the type that is the closest match to the Type defined in the Field definition) we would have to to all sorts of type-checks in the getter methods. Maybe we could settle for the largest types for Boolean, Integer, Floating-Point, String, Time, Date, Date-Time and not support all possible types, but then you would simply get this base type back and not what you defined. ... But thinking about it a little more ... we could implement a getObject that returns the type defined in the field ... would that be an option?

      3) No objections ...

      4) No objections ... 

      1. Unknown User (cdutz)

        And regarding the Reader/Writer etc. we are thinking of building special versions of drivers, that don't contain any code for writing (for security reasons) ... so not every driver will support all the functional domains. This way we can encapsulate the code for each domain in it's corresponding class.

        Please have a look at how this feature is implemented in current PlcConnections ... for example the S7Connection implements the PlcReader and PlcWriter interface and in the AbstractPlcConnection the getReader method is implemented to simply check if a connection implements the PlcReader interface. So you could just cast the connection to PlcReader and use it directly, but I like this sort of additional abstraction layer.

        1. Julian Feinauer

          Hi Chris,

          thank you for the explanation. I did not consider the security aspect but it makes sense.

          And regarding your comment for the S7Connection I think this knowledge is of not that big of a use at it would be very Plc Depdendent to rely on this behavior.

          Just as a note, another alternative would be to provide a MarkerInterface for all Drivers that implement PlcReader and PlcWriter directly.

          But we can also keep it as is.

          1. Unknown User (cdutz)

            I'd opt for keeping it as it is (at least this part of it)

            1. Julian Feinauer

              okay, i'm fine with it.

  2. Julian Feinauer

    Hi Chris,

    another note to your UML.

    I like to provide the generic structure of the driver and to provide a PlcTypeConverter interface (you remember I also worked around that a bit).

    But I am unsure about the method signatures and I'm unsure if we can provide this without a generic interface ("PlcTypedAddress") as it is unclear how to tell the converter all the logical information about the byte array.

    Furthermore, often times encoding and decoding works on a larger byte array and bytes are "consumed" subsequently, one example from the S7 Driver:

            int i = 0;
            final int length = s7Data.length;
            while (i < length) {
                if (datatype == Boolean.class) {
                    result.add((s7Data[i] & 0x01) == 0x01);
                    i += 1;
    				List<Object> result = new LinkedList<>();

    Code from https://github.com/JulianFeinauer/incubator-plc4x/blob/master/plc4j/protocols/s7/src/main/java/org/apache/plc4x/java/s7/netty/util/S7TypeDecoder.java

    The point here is that we increase the index i with each read operation.

    This is also not reflected in the API (could be done by switching to ByteBuffer which manages this implicitly).

    So although I would like such an interface I think we cant define its methods reasonable well.

    So I suggest to either change it to a merker interface or remove it for this current refactoring.

    1. Unknown User (cdutz)

      I don't have any problems with using a ByteBuffer instead of a byte-array.

      All I was suggesting, was to shift all decoding to when and where data is actually accessed. Otherwise we would decode to some intermediate form and then convert this when accessing it.

  3. Unknown User (sruehl)

    Im not quite sure using these hardcoded types in the method names in PlcReadResponse. I would more prefer supplying the type as param `is(Class clazz, String name)`. From an api perspective this is quite the same and more extensible.

    1. Unknown User (cdutz)

      This way there is no way to use an unsupported type. With your preferred way, we could write "is(Hurz.class, String name)". I am not quite sure about the "is-methods" as I'm trying to build the data structures that they will provide what the user requests. So if he reads a BYTE resource but wants to treat that as an Integer in his code (to avoid casting), the response would take care of that. I am currently trying to remove all of these "instanceof" statements, that were blowing up our code in all sorts of places ... let us stick with this option and we'll be able to add the generic versions afterwards. This way has the benefit of being in one place and not distributed throughout the modules as it is now.

      1. Unknown User (sruehl)

        im fine with that, we could even add it ass a default method which calls the other methods right now so we won't loose anything but as you said lets just go this way for now.