RE: [jModbus-devel] Codebase Merge initial comments

[Proctor, K. <Kel...@al...>]

Dieter,

As I've slowly discovered over the last year when you first read someone
else's code you initial reaction is almost always to see a different way you
would have done things and usually dislike the code you are looking at in
some way.  I've found this can with equal likelihood be caused by 1. the
code actually being bad or 2. you not understanding the code properly and
the problem the code solves.

In a few areas you have a very large number of interfaces.  I like the idea
of exposing the internals of the system in certain places to allow for
greater flexibility.  In a few places I'm wondering if you have used a lot
of interfaces to get around multiple inheritance types issues in places or
if you expect all of the said interfaces to possibly be implemented by
external classes by a user in some way.  Please don't take this as a
criticism, I'm just curious as my formal OO training has been very limited,
most of it has been self taught.  I'm just comparing the number of
interfaces vs. real classes in this API vs. the main java API as an example.
Are there places where simple inheritance could be used more cleanly?

Anyway I'm about to go home and I'll see how quickly I can get the ASCII
transport to you.

Other than that see the comments below.

Cheers,

Kelvin

> -----Original Message-----
> From:	Dieter Wimberger [SMTP:wi...@oe...]
> Sent:	Sunday, October 21, 2001 10:45
> To:	jmo...@li...
> Subject:	Re: [jModbus-devel] Codebase Merge initial comments
> 
> 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.
	[Proctor, Kelvin]  
	This is not necessarily true.  The spec states that messages may be
pipelined by some clients and this could happen in the same packet quite
conceivably, or (more unlikely) the packets could be fragmented.  It does
not matter to us however as we should only look at it in terms of the TCP
stream.  The spec says a multi threaded client should read in just the 6
byte header and examine that.  Two checks are then to be performed, 1. the
protocol identifier check and 2. the check that the message length field is
not greater than 255.  If either of these fail the socket is to be closed.

> 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).
> 
	[Proctor, Kelvin]  
	The distinction I would draw here is that the unit ID and the
function code and part of the modbus message, which I define to be only that
part which is common to all transport mechanisms.  I have included the
transaction ID into this as an exception and have decided that the ASCII and
RTU transports simply default this to zero.



> >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.
> 
	[Proctor, Kelvin]  
	Sorry, my mistake in not clearly explaining my self here and not
understanding you code properly.  What I had intended to imply is that the
ModbusMessage must keep an internal buffer that contains the message and
create streams from than (both input and output) that any of the 'message
processing' (read higher level) code desires.  We can't have a stream that
leads down to one of the TCP socket's streams being used at a higher level
as this would not work with the ASCII transport, as a translation of the
stream from ASCII encoded data to binary is required.  

	I had through about writing a classes to extend one of the IO stream
classes but I can't think of a satisfactory way of mapping the start and end
delimiters through that get round the face that they are 'in-band' coding,
as the modbus message can contain any of the 0-255 byte values.  There is
also the issue that the transport needs to calculate and check/add the LRC,
which is not part of the 'modbus message' as I have defined it.

	I have been a little delayed but I should be able to get the ASCII
transport sent to you tonight.  It will be for my old architecture but it
should demonstrate the point.
	 
> >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.
> 
	[Proctor, Kelvin]  
	As above.  I'll send the code for the ASCII module to you soon and
this should act as a good point for this discussion to continue from.

> >a modifying these
> >classes and we'll see what you think.
> 
> I'd hope you see my point....
> 
	[Proctor, Kelvin]  
	Yes, unfortunately the problem is not quite as simple as either of
us would have liked, but I think I can see the solution.

> >--- 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
> 
> 
> _______________________________________________
> jModbus-devel mailing list
> jMo...@li...
> https://lists.sourceforge.net/lists/listinfo/jmodbus-devel

Alcoa World Alumina Australia is a trading name of Alcoa of Australia Limited,  ACN 004 879 298