You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by Andrey Skorikov <an...@codecentric.de> on 2018/10/04 14:34:17 UTC
Re: Define execute operation on Request; remove read/write operations
from Reader/Writer
Hello all,
I propose to refactor the PLC4J API and move operations PlcReader.read,
PlcWriter.write and PlcSubscriber.{subscribe,unsubscribe} for submitting
requests to the PLC away from these interfaces and place one execute()
operation directly on the request type. This has already been discussed
in the mailing list, but no decision to implement the change was made.
I have implemented the proposal in a separate branch and created a pull
request to review the changes. Additional details can be found in the
description of the pull request in github.
Best regards,
Andrey
On 09/13/2018 11:42 AM, Sebastian Rühl wrote:
> 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 <an...@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 <an...@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" <an...@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 <an...@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
>>>> >>
>>>>
>>>>
>>>>
>
Re: Define execute operation on Request; remove read/write operations
from Reader/Writer
Posted by Andrey Skorikov <an...@codecentric.de>.
Technically, we could do that. The API module would become cleaner and
move cohesive, but the SPI module would still depend on it.
Ideally, we would have both the API and the SPI modules independent of
each other, and the core module containing DriverManager acting as a
bridge. Though still not a rocket science, this change would take a
little more effort to implement. Definitely more than five minutes of
time. ;-)
Perhaps we can extract the SPI classes in a separate module like you
proposed, and decouple them in the next pull request. What do you think?
Andrey
On 10/04/2018 06:19 PM, Christofer Dutz wrote:
> What about a dedicated spi module? I mean, doesn't cost us much. Just about 5 minutes ouf time.
>
> Chris
>
> Outlook for Android<https://aka.ms/ghei36> herunterladen
>
> ________________________________
> From: Andrey Skorikov <an...@codecentric.de>
> Sent: Thursday, October 4, 2018 5:50:38 PM
> To: dev@plc4x.apache.org
> Subject: Re: Define execute operation on Request; remove read/write operations from Reader/Writer
>
> Hi Chris,
>
> yeah, moving the SPI classes away from the API module is a good idea.
> This would result in a clean cohesive API module for clients. However I
> am not sure whethere driver-base is a good place for them, for two reasons:
>
> 1. Protocol implementers would still have to depend on both the API and
> the driver-base module, since PlcDriver depends on PlcConnection, for
> example. In other words: it is not sufficient do depend on driver-base
> only, if you want to provide a protocol implementation.
>
> 2. Although the driver-base module contains some useful "support"
> classes, they are not strictly required to implement a protocol driver
> either - only the classes in the SPI package are essential.
>
> Personally, I would prefer to leave the SPI classes in the API package,
> at least for the moment. ;-)
>
> Andrey
>
>
> On 10/04/2018 05:30 PM, Christofer Dutz wrote:
>> Hi Andrey,
>>
>> I just had a second look at your proposed changes. I really like them.
>> I was already sort of getting annoyed by having the need to use the PlcReader.read() method to do the read and do think your suggestion is a good one.
>>
>> One thing we could think about, would be to move all classes in the "spi" package outside the "api" module. I couldn't find any references to them from within the rest of the package.
>> I think the driver-base might be a good home for them.
>>
>> What do you think?
>>
>> Chris
>>
>>
>> Am 04.10.18, 16:34 schrieb "Andrey Skorikov" <an...@codecentric.de>:
>>
>> Hello all,
>>
>> I propose to refactor the PLC4J API and move operations PlcReader.read,
>> PlcWriter.write and PlcSubscriber.{subscribe,unsubscribe} for submitting
>> requests to the PLC away from these interfaces and place one execute()
>> operation directly on the request type. This has already been discussed
>> in the mailing list, but no decision to implement the change was made.
>>
>> I have implemented the proposal in a separate branch and created a pull
>> request to review the changes. Additional details can be found in the
>> description of the pull request in github.
>>
>> Best regards,
>> Andrey
>>
>>
>> On 09/13/2018 11:42 AM, Sebastian Rühl wrote:
>> > 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 <an...@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 <an...@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" <an...@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 <an...@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
>> >>>> >>
>> >>>>
>> >>>>
>> >>>>
>> >
>>
>>
>>
Re: Define execute operation on Request; remove read/write operations
from Reader/Writer
Posted by Andrey Skorikov <an...@codecentric.de>.
Hi Chris,
I moved the Plc{Reader,Writer,Subscriber} interfaces to the driver-base
module, where they are actually used. They are not strictly required for
protocol implementation. The SPI package now contains only the PlcDriver
interface.
Andrey
On 10/04/2018 06:19 PM, Christofer Dutz wrote:
> What about a dedicated spi module? I mean, doesn't cost us much. Just about 5 minutes ouf time.
>
> Chris
>
> Outlook for Android<https://aka.ms/ghei36> herunterladen
>
> ________________________________
> From: Andrey Skorikov <an...@codecentric.de>
> Sent: Thursday, October 4, 2018 5:50:38 PM
> To: dev@plc4x.apache.org
> Subject: Re: Define execute operation on Request; remove read/write operations from Reader/Writer
>
> Hi Chris,
>
> yeah, moving the SPI classes away from the API module is a good idea.
> This would result in a clean cohesive API module for clients. However I
> am not sure whethere driver-base is a good place for them, for two reasons:
>
> 1. Protocol implementers would still have to depend on both the API and
> the driver-base module, since PlcDriver depends on PlcConnection, for
> example. In other words: it is not sufficient do depend on driver-base
> only, if you want to provide a protocol implementation.
>
> 2. Although the driver-base module contains some useful "support"
> classes, they are not strictly required to implement a protocol driver
> either - only the classes in the SPI package are essential.
>
> Personally, I would prefer to leave the SPI classes in the API package,
> at least for the moment. ;-)
>
> Andrey
>
>
> On 10/04/2018 05:30 PM, Christofer Dutz wrote:
>> Hi Andrey,
>>
>> I just had a second look at your proposed changes. I really like them.
>> I was already sort of getting annoyed by having the need to use the PlcReader.read() method to do the read and do think your suggestion is a good one.
>>
>> One thing we could think about, would be to move all classes in the "spi" package outside the "api" module. I couldn't find any references to them from within the rest of the package.
>> I think the driver-base might be a good home for them.
>>
>> What do you think?
>>
>> Chris
>>
>>
>> Am 04.10.18, 16:34 schrieb "Andrey Skorikov" <an...@codecentric.de>:
>>
>> Hello all,
>>
>> I propose to refactor the PLC4J API and move operations PlcReader.read,
>> PlcWriter.write and PlcSubscriber.{subscribe,unsubscribe} for submitting
>> requests to the PLC away from these interfaces and place one execute()
>> operation directly on the request type. This has already been discussed
>> in the mailing list, but no decision to implement the change was made.
>>
>> I have implemented the proposal in a separate branch and created a pull
>> request to review the changes. Additional details can be found in the
>> description of the pull request in github.
>>
>> Best regards,
>> Andrey
>>
>>
>> On 09/13/2018 11:42 AM, Sebastian Rühl wrote:
>> > 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 <an...@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 <an...@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" <an...@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 <an...@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
>> >>>> >>
>> >>>>
>> >>>>
>> >>>>
>> >
>>
>>
>>
Re: Define execute operation on Request; remove read/write operations
from Reader/Writer
Posted by Christofer Dutz <ch...@c-ware.de>.
What about a dedicated spi module? I mean, doesn't cost us much. Just about 5 minutes ouf time.
Chris
Outlook for Android<https://aka.ms/ghei36> herunterladen
________________________________
From: Andrey Skorikov <an...@codecentric.de>
Sent: Thursday, October 4, 2018 5:50:38 PM
To: dev@plc4x.apache.org
Subject: Re: Define execute operation on Request; remove read/write operations from Reader/Writer
Hi Chris,
yeah, moving the SPI classes away from the API module is a good idea.
This would result in a clean cohesive API module for clients. However I
am not sure whethere driver-base is a good place for them, for two reasons:
1. Protocol implementers would still have to depend on both the API and
the driver-base module, since PlcDriver depends on PlcConnection, for
example. In other words: it is not sufficient do depend on driver-base
only, if you want to provide a protocol implementation.
2. Although the driver-base module contains some useful "support"
classes, they are not strictly required to implement a protocol driver
either - only the classes in the SPI package are essential.
Personally, I would prefer to leave the SPI classes in the API package,
at least for the moment. ;-)
Andrey
On 10/04/2018 05:30 PM, Christofer Dutz wrote:
> Hi Andrey,
>
> I just had a second look at your proposed changes. I really like them.
> I was already sort of getting annoyed by having the need to use the PlcReader.read() method to do the read and do think your suggestion is a good one.
>
> One thing we could think about, would be to move all classes in the "spi" package outside the "api" module. I couldn't find any references to them from within the rest of the package.
> I think the driver-base might be a good home for them.
>
> What do you think?
>
> Chris
>
>
> Am 04.10.18, 16:34 schrieb "Andrey Skorikov" <an...@codecentric.de>:
>
> Hello all,
>
> I propose to refactor the PLC4J API and move operations PlcReader.read,
> PlcWriter.write and PlcSubscriber.{subscribe,unsubscribe} for submitting
> requests to the PLC away from these interfaces and place one execute()
> operation directly on the request type. This has already been discussed
> in the mailing list, but no decision to implement the change was made.
>
> I have implemented the proposal in a separate branch and created a pull
> request to review the changes. Additional details can be found in the
> description of the pull request in github.
>
> Best regards,
> Andrey
>
>
> On 09/13/2018 11:42 AM, Sebastian Rühl wrote:
> > 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 <an...@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 <an...@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" <an...@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 <an...@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
> >>>> >>
> >>>>
> >>>>
> >>>>
> >
>
>
>
Re: Define execute operation on Request; remove read/write operations
from Reader/Writer
Posted by Andrey Skorikov <an...@codecentric.de>.
Hi Chris,
yeah, moving the SPI classes away from the API module is a good idea.
This would result in a clean cohesive API module for clients. However I
am not sure whethere driver-base is a good place for them, for two reasons:
1. Protocol implementers would still have to depend on both the API and
the driver-base module, since PlcDriver depends on PlcConnection, for
example. In other words: it is not sufficient do depend on driver-base
only, if you want to provide a protocol implementation.
2. Although the driver-base module contains some useful "support"
classes, they are not strictly required to implement a protocol driver
either - only the classes in the SPI package are essential.
Personally, I would prefer to leave the SPI classes in the API package,
at least for the moment. ;-)
Andrey
On 10/04/2018 05:30 PM, Christofer Dutz wrote:
> Hi Andrey,
>
> I just had a second look at your proposed changes. I really like them.
> I was already sort of getting annoyed by having the need to use the PlcReader.read() method to do the read and do think your suggestion is a good one.
>
> One thing we could think about, would be to move all classes in the "spi" package outside the "api" module. I couldn't find any references to them from within the rest of the package.
> I think the driver-base might be a good home for them.
>
> What do you think?
>
> Chris
>
>
> Am 04.10.18, 16:34 schrieb "Andrey Skorikov" <an...@codecentric.de>:
>
> Hello all,
>
> I propose to refactor the PLC4J API and move operations PlcReader.read,
> PlcWriter.write and PlcSubscriber.{subscribe,unsubscribe} for submitting
> requests to the PLC away from these interfaces and place one execute()
> operation directly on the request type. This has already been discussed
> in the mailing list, but no decision to implement the change was made.
>
> I have implemented the proposal in a separate branch and created a pull
> request to review the changes. Additional details can be found in the
> description of the pull request in github.
>
> Best regards,
> Andrey
>
>
> On 09/13/2018 11:42 AM, Sebastian Rühl wrote:
> > 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 <an...@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 <an...@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" <an...@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 <an...@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
> >>>> >>
> >>>>
> >>>>
> >>>>
> >
>
>
>
Re: Define execute operation on Request; remove read/write operations
from Reader/Writer
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi Andrey,
I just had a second look at your proposed changes. I really like them.
I was already sort of getting annoyed by having the need to use the PlcReader.read() method to do the read and do think your suggestion is a good one.
One thing we could think about, would be to move all classes in the "spi" package outside the "api" module. I couldn't find any references to them from within the rest of the package.
I think the driver-base might be a good home for them.
What do you think?
Chris
Am 04.10.18, 16:34 schrieb "Andrey Skorikov" <an...@codecentric.de>:
Hello all,
I propose to refactor the PLC4J API and move operations PlcReader.read,
PlcWriter.write and PlcSubscriber.{subscribe,unsubscribe} for submitting
requests to the PLC away from these interfaces and place one execute()
operation directly on the request type. This has already been discussed
in the mailing list, but no decision to implement the change was made.
I have implemented the proposal in a separate branch and created a pull
request to review the changes. Additional details can be found in the
description of the pull request in github.
Best regards,
Andrey
On 09/13/2018 11:42 AM, Sebastian Rühl wrote:
> 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 <an...@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 <an...@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" <an...@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 <an...@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
>>>> >>
>>>>
>>>>
>>>>
>