Re: [jModbus-devel] Codebase Merge initial comments

[Dieter W. <wi...@oe...>]

Hi Kelvin,

Good to hear from you. I add some comments below, as I'd like to oppose to some of your findings.


>--- IO Package ---

>The Modbus TCP Transport is not as fully robust as it might possibly be.
>The only thing you
>can really assume about a packet is that you can read 6 bytes to give the
>complete header.
>If that fails you close the socket.  You are not able to assume that you
>will receive a unit
>ID or a function code, which I do not believe should be part of the
>transport.  The transport
>should have the responsibility for framing and deframing a packet and should
>not be concerned
>with it's contents in any way.  I will modify this to behave like my
>original package which
>has all these protections and checks.

Hmm ok, however, it does not make any difference. Basically you can assume that if you cannot read one character you did not get any package ;)
Remember that the whole Modbus message is coming in _one_ TCP package.
According to the specs the Unit ID and function code have a special position I know, but I rather account it on the header then to the package data.
The Transport does not do anything on the messages actually, because all of it is done within the ModbusMessageImpl and the specialized ModbusRequest or ModbusResponse.
I did drop a check on the first read in the ModbusMessageImpl, I just did not hook it up, because I believe it should be bubbled to the connection through an event mechanism (this would be the right place to handle the problem, either by closing and reporting to the user, or by re-executing the request).


>I'm also of the opinion that the interaction between the transport classes
>and the message
>classes should be done in terms of byte arrays.  (We should also user the
>System.arrayCopy
>function where required as it is implemented locally in a fast manner).

Another hmm. You can simply get the message as bytes and read them out, this isn't much of a problem. However, note that per design you will get a newly allocated byte array from the ByteArray stream. 
It is a matter of resources.....
On a object oriented design basis I'd say that it is completely ok to pass the message object.


>Where the rest
>of the application desires streams based communications this should be done
>through the use
>ByteArray streams.  This would be done as it means that *only* the frame
>data needs to be
>transferred to the messaging classes, leaving all the other rubbish to do
>with the differences
>between the transport in the transport objects.  (Things like frame
>delimnating breaks in the
>RTU modes and CR LF and : breaks in the ASCII would stuff things up and the
>whole situation
>would not work for ASCII as the data is not binary encoded.)  I'll have a go

Now I understand your point better. Well, the problem is just that the whole design as it is becomes obsolete under this viewpoint :(
Well the design as I came up with is in a very object oriented manner, taking care of resources and with reasonably assigned responsibilities.

The real handling of the different transport flavors can be simply done in the Transports. Look, this is what I meant with RTU/ASCII Input and Output Streams.
The specialized Transport implementation can use such filtering streams to translate binary data to the encoding that is necessary for the specific flavor, no specialized message class would _ever_ get involved into this.
Maybe the situation with data transport is a bit different here though. Guess that I/O exception handling has to be re-thought very well. The assumption that all comes in one "package" is not valid, like for TCP.


>a modifying these
>classes and we'll see what you think.

I'd hope you see my point....

>--- Process Image Package ---

>I also like the way you have made all the registers etc.. interfaces as this
>can provide a
>means by which an event/listener interface as I has proposed can be avoided
>completely.  If
>you want to trigger on certain types of changes to a register values then
>you implement the
>register yourself so that so write the set command and get put whatever
>trigger code in there
>that you desire.

Exactly this is the idea. In fact I think that it will be necessary to taylor implementations to the specific needs. Concurrency issues can be handled without affecting the downstream. I had some discussions with my advisor, and I am sure I will try to implement some more sophisticated items to showcase copy-on-write or Read-Write priority handling.

Regards,
Dieter