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 <[email protected]> Sent: Thursday, October 4, 2018 5:50:38 PM To: [email protected] 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" > <[email protected]>: > > 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 > <[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 > >>>> >> > >>>> > >>>> > >>>> > > > > >
