Just a small note: In ADS it is implemented already this way (Option 3) Sebastian
> Am 17.08.2018 um 10:38 schrieb Christofer Dutz <[email protected]>: > > Hi all, > > Yesterday I have worked hard on implementing the feature of detecting the > type of S7 as part of the connection process. > Fortunately this was a lot simpler than expected as it turned out that > Siemens documents the structure of the SZL (Engl. SSL) List elements in the > handbooks of their devices. > > But it did bring up something I noticed when last working on the S7 driver. > > I implemented the driver keeping in mind the good concepts of software design > and tried to follow them as good as possible. > One of these rules is not to couple logic and model. > > As a result of this, we have the model classes (Driver internal model) and in > the corresponding “XyzProtocol” class the logic for serializing and > deserializing. > > This worked well until I started implementing the automatic fragmentation and > simulation in the S7MessageProcessors. > > Now I needed little helpers that were able to estimate the size of the > serialized form based on the model itself. > > The result of this, is that now we have at minimum 3 places, which are highly > coupled that need maintaining and testing. > > This did make things tricky when implementing the SSL List processing and did > blow up the size and complexity of the S7Driver class. > > Now to my proposal: > > In general I see 3 options for doing this: > > > 1. (The current way) We have model classes without any logic, A driver > implementation that contains the serialization/deserialization logic, Some > helpers tightly linked > 2. For each model item, we create a separate class which contains the logic > for serialization/deserialization and size estimation > 3. We add serialization/deserialization to the model classes > > I currently favor option 3. Here my explanation why: > > Well option 1 was clean in the beginning, but I am seeing more and more > problems with maintaining it. > > If we went for option 2, we would also have to implement a set of factory > classes, that help finding the serialization/deserialization class for a > given input. This can be tricky especially when using inheritance. Also does > it make things a little more complicated when digging into the code for new > people. > > Option 3 would have everything related to a protocol element in one place. We > would simply allow calling “toProtocol()”, “fromProtocol()”, > “getSizeInBytes()”. This does mix model and logic, but in this case I would > more treat it as additional form of serialization (after all each Object in > java has “toString()” and the a little hidden serialization that allows > serializing an object to a byte stream. This would simply add another option > to that list. And after all the serialization/deserialization doesn’t really > do much logic. Another downside would be that in order to know what type of > object is coming, I have to shift a little knowledge to the upstream object. > So the S7Message has to know which types of Parameter could come, instantiate > the correct one and then have it fill itself with the data. Thinking more > about it, I would probably add a second constructor to each model type, > taking in the Netty ByteBuffer and implement a private “fromProtocol” method > as a sort of convention. And the only reason the model exists, is to provide > this one single from of serialization. So the argument that “but we could add > a different serialization later on” is not valid in this case as this would > actually mean a new driver. > > So what do you think? > > Please don’t just fall back to fundamentalist: “You always have to do > something path X” or “You never should Y”. If you do please provide evidence > of your concerns so we can discuss them. > > In the end I want to make our code: > > 1. Easily maintainable > 2. Easily testable > 3. Easily understandable > > Chris > > > > > >
