plc4x-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sebastian Rühl <sebastian.ruehl...@googlemail.com.INVALID>
Subject Re: Define execute operation on Request; remove read/write operations from Reader/Writer
Date Thu, 13 Sep 2018 09:42:47 GMT
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 <andrey.skorikov@codecentric.de>:
> 
> 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 <andrey.skorikov@codecentric.de>
>> Sent: Wednesday, September 12, 2018 4:58:12 PM
>> To: dev@plc4x.apache.org
>> 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" <andrey.skorikov@codecentric.de>:
>>> 
>>>      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 <andrey.skorikov@codecentric.de>:
>>>      >>
>>>      >> 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
>>>      >>
>>> 
>>> 
>>> 
> 


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message