Hey Andrey,
Currently I have added a convenience method for my purposes on the interface
which makes it easier to write requests:
CompletableFuture<PlcReadResponse<?>> response = reader.read(builder ->
builder.addItem("station", "Allgemein_S2.Station:BYTE"));
This way you are not required to write the same code over again. Maybe this
helps a bit?
Sebastian
> Am 13.09.2018 um 10:49 schrieb Andrey Skorikov
> <[email protected]>:
>
> Hi,
>
> I believe that if we move the execute() operation to requests, we could also
> get rid of PlcReader/PlcWriter interfaces altogether, since otherwise they
> would degenerate to very thin interfaces containing nothing but a single
> factory method for obtaining PlcReadRequest.Builders. So, in order to execute
> a read request, instead of:
>
> PlcReader reader = connection.getReader().get(); // ignore .get() for a moment
> PlcReadRequest request = reader.readRequestBuilder().addItem(...).build();
> reader.read(request);
>
> we could write:
>
> connection.readRequestBuilder().get().addItem(...).build().execute();
>
> Best regards,
> Andrey
>
>
> On 09/12/2018 06:21 PM, Christofer Dutz wrote:
>> Hi,
>>
>> This correlation was just a convenience at the start. I never really liked
>> it, but wasn't annoying enough to do it.
>>
>> I would strongly opt for splitting it up into separate classes. Especially
>> as it generally allows producing read-only only driver versions by excluding
>> classes from the lib.
>>
>> Chris
>>
>> Outlook for Android<https://aka.ms/ghei36> herunterladen
>>
>> ________________________________
>> From: Andrey Skorikov <[email protected]>
>> Sent: Wednesday, September 12, 2018 4:58:12 PM
>> To: [email protected]
>> Subject: Re: Define execute operation on Request; remove read/write
>> operations from Reader/Writer
>>
>> Hi Chris,
>>
>> no need for a factory-factory. :) I believe that the core problem is
>> that the PlcConnection interface does too much - it offers
>> protocol-specific transport functionality by providing the
>> PlcReader.read operation, it maintains protocol state, and it serves as
>> a factory for read requests (through PlcReader.readRequestBuilder).
>>
>> Another problem is that the requests, which are obtained through the
>> ReadRequestBuilder are basically independent from a concrete connection
>> instance. Hence it does not make sense to obtain a Builder instance from
>> a PlcConnection. For example, consider the following scenario: first we
>> obtain a PlcConnection, PlcReader and a RequestBuilder off it. Then we
>> close the PlcConnection. What should happen, if we build and try to
>> execute requests from that RequestBuilder? Should it return request
>> instances but throw exceptions when we try to execute them on another
>> (live) connection? Or should it throw exceptions when we call build()?
>> Or should it allow us to execute the built requests on another
>> connection? In the latter case, why should be forced to obtain a request
>> builder from a connection instance anyway?
>>
>> Best regards,
>> Andrey
>>
>>
>> On 09/12/2018 04:21 PM, Christofer Dutz wrote:
>>> Hi Andrey,
>>>
>>> are you proposing a Factory-Factory? Wouldn't that go a little too far?
>>> Currently the request factory method being in the connection, is just an
>>> implementation detail we could have a S7Reader class implementing the
>>> Reader interface and this is generated from the connection, if this is the
>>> problem.
>>>
>>> Chris
>>>
>>> Am 12.09.18, 15:54 schrieb "Andrey Skorikov"
>>> <[email protected]>:
>>>
>>> Hi Sebastian,
>>>
>>> good point! The mutability of PlcReadRequest would be a consequence of
>>> the mutability of the PlcConnection (or something that can handle the
>>> execute). However, in order to construct a immutable PlcReadRequest,
>>> currently we still need to obtain a PlcRequest.Builder from the same
>>> mutable PlcConnection. I think, if we really want PlcReadRequest to be
>>> immutable, then we should be able to construct them independently (not
>>> from a mutalbe object). Perhaps we could separate PlcRequest
>>> construction from its exection by providing a RequestBuilder factory
>>> method not on a mutable PlcConnection but "higher up", for example
>>> somewhere on a PlcDriver?
>>>
>>> Regards,
>>> Andrey
>>>
>>> On 09/12/2018 03:33 PM, Sebastian Rühl wrote:
>>> > Hey Andrey,
>>> >
>>> > One thing that would stand against this: Adding a execute() would
>>> make the PlcReadRequest mutable which is a thing we should avoid (Mutable
>>> because it would need a reference store to something that can handle the
>>> execute).
>>> >
>>> > FYI: I added a mutability test into plc4j-api (which should be added
>>> to plc4j-driver-bases after the refactoring) which tests all Items for
>>> mutability. Currently we have some open issues regarding that.
>>> >
>>> > Sebastian
>>> >
>>> >> Am 12.09.2018 um 14:50 schrieb Andrey Skorikov
>>> <[email protected]>:
>>> >>
>>> >> Hello,
>>> >>
>>> >> currently, PlcReadRequests and PlcWriteRequests are executed by
>>> invoking the corresponding read/write operation on the PlcReader/PlcWriter
>>> objects. Hence the typical workflow for reading a value contains the
>>> following steps:
>>> >>
>>> >> 1. Obtain a PlcReader instance from a PlcConnection
>>> >> 2. Obtain a Builder by invoking readRequestBuilder() on PlcReader
>>> >> 3. Build a request using the Builder
>>> >> 4. Execute the request by invoking read() on the PlcReader, passing
>>> the constructed request as argument
>>> >>
>>> >> PlcReader reader = ... // obtain reader
>>> >> PlcReadRequest request =
>>> reader.readRequestBuilder().addItem(...).build();
>>> >> Future<PlcReadResponse> response = reader.read(request);
>>> >>
>>> >> Since we only can build a request throgh a PlcReader instance,
>>> invoking the read operation on the PlcReader is redundant. Therefore I
>>> suggest removing the read/write operation from the PlcReader/PlcWriter and
>>> defining a execute() operation on PlcRequest instead. The workflow would
>>> look like this then:
>>> >>
>>> >> PlcReader reader = ... // obtain reader
>>> >> PlcReadRequest request =
>>> reader.readRequestBuilder().addItem(...).build();
>>> >> Future<PlcReadResponse> response = request.execute();
>>> >>
>>> >> Please note that there is no more need to "keep" a reference to
>>> PlcReader in this context, since it is implicitly associated with the
>>> request by calling reader.readRequestBuilder().build().
>>> >>
>>> >> Best regards,
>>> >> Andrey
>>> >>
>>>
>>>
>>>
>
