Menu
▾
▴
RE: Re: [jModbus-devel] Codebase Merge initial comments
|
From: Proctor, K. <Kel...@al...> - 2001-10-23 04:45:12
|
Dieter,
Based on you comments below regarding the use of filter streams I think I
can now see a nice way to handle this, along the lines of what you had
proposed.
I would propose a slightly different looking ASCIIInputStream class. See
what you think.
public class ASCIIInputStream extends FilterInputStream {
public ASCIIInputStream(InputStream in) {
//constructor.....
}
public void readFrame(byte[] buff) {
// This function will write a whole frame (including the
LRC) into
// the byte array, having converted from ASCII into binary.
// i.e. it will strip the leading : and the trailing CR LF
as well as
// do the binary conversion
}
}
The LRC check and creation could exist in the transport class, which would
be responsible for rejecting frames.
I'm not sure I see any point in being able to read the frame a single byte
at a time before the LRC is calculated and checked. If there is a good
reason to be able to do this can you point it out to me. This also raises
the point that if reading a single byte is not a useful thing to do then why
would you make it extend FilterInputStream if some of it's methods can not
apply to this class. ([general OO question] Is this a valid criteria to
impose when considering extending a class, I'm not sure?)
Part of my reasoning is this: Below you have proposed that the start and
end frame characters be passed up from the ASCII Stream to the Transport,
where deframing and checksum checks could occur. I'm wondering how this
would be done. If we are returning a byte (that as part of the message may
have any value) how do we signal that this is the end or start of the frame
character? It could be possible by setting the int returned to a different
negative value (not -1 which indicates failure) such as -2 for start of
frame and -3 for end of frame. This presents the problem of what to do for
the block read functions (which should be used wherever possible to avoid
byte banging which is inefficient, but we will have to do some of that
anyway for the conversion so it doesn't really matter).
Other than the above difficulty I like the proposal you have put forward.
From below:
The fix you have proposed for the TCP transport is good. With no possible
way of regaining sequence if it is lost it is important to make sure we
preform all checks with the TCP transport, the other at least have
delimiters.
The package structure is quite nice. It is at first a bit baffling with the
net and io packages. Once I considered this was only written with the TCP
implementation in mind it made much more sense.
The example with the interfaces would be the Modbus interface. No classes
implement this interface, thus I'm wondering if it would be an interface or
simply a class with public static final fields? This is a very minor thing
but I'm wondering if there was any special reasoning behind it that I can't
figure out.
Another example would be the ModbusTransaction interface. Under what type
of use would you see a class other than ModbusTransactionImpl implementing
this interface. This is not to argue that it should not be setup this way
but I'm curious how this sort of thing would be used and what the
application would be. Likewise for ModbusMessage and ModbusMessageImpl.
With the ModbusCoupler class it makes reference to a Singleton pattern. I'm
wondering what this is as I have never come across the term. I probably
know it by a different (crazy Australian) name so I though I should just
check it's meaning.
Anyway I had better get back to doing some real work (which in this
unfortunate case is writing a GUI to a database in VB, not very nice....)
Cheers,
Kelvin
> -----Original Message-----
> From: Dieter Wimberger [SMTP:wi...@oe...]
> Sent: Monday, October 22, 2001 21:36
> To: jmo...@li...
> Subject: Re: Re: [jModbus-devel] Codebase Merge initial comments
>
> Kelvin,
>
> I completely agree with you that it is not easy to share code or work with
> code from other people. I also think that I have bombed you a bit, because
> I delivered a complete package with completely different approaches to
> everything.
> I am long enough into Open Source (I am contributing and leading some
> projects) that I know this not easy, but it bears a huge potential for
> learning from each other.
> The only thing that is important when opening the source is that you have
> detached emotionally from your creation. In any other case, you will take
> things discussed personally, and this will block any learning process.
>
> Regarding the model, well, usually the best way to see what a model does
> is to check out the UML Class diagrams. They visualize what is often very
> hard to understand from the code.
> I will try to walk through the packages to explain the motivation for the
> interface:
>
> io package:
> There is the ModbusTransaction and the ModbusTransport. Both are supposed
> to be used to code against, representing rather a contract then anything
> else. The common method in those cases do not necessarily share more then
> the Interface, so it is natural to proceed like this.
>
> msg package:
> The ModbusMessage is probably not necessary, but it could allow to replace
> the implementation without problems, if the rest of the code is written
> properly against the contract.
> People always think things can be further optimized :)
>
> The abstract classes are to get a differentiated model with clearly
> defined entities and responsibilities.
>
> net package:
> No interface, however, I am thinking of introducing either a superclass or
> an interface called ModbusConnection, which both the slave and the master
> connection should be implementing or subclassing.
> This will allow handling troubles on the low level side.
>
> procimg package:
> There are a lot of interfaces ok :) Well the reason is again to define
> contracts to code against.
> Without knowing the actual specifics of the implementation, the other
> parts of the code will be able to read/write values. I think you have
> already realized the benefit of this, at least I have understood that from
> the comment you added in the other mail.
>
>
> util package:
> No interfaces.
>
>
> Now, to summarize, I would say the major driver for the use of Interfaces
> is to
> 1. define contracts
> 2. hide implementation specifics, respectively expose just what should be
> exposed
>
> Multiple inheritance is not really an issue, and I have seen enough misuse
> of that in C++ to be able to understand the "no multiple inheritance" move
> of the Java language creators. No tears for those classes that inherit
> complete sets of parents only to gather one or two methods.....
>
>
> Probably you can specify exactly were you see the problem with the
> inheritance, because I am not exactly sure which part you are adressing.
> Don't worry about the critism, I will try to learn from the discussions,
> and not take it personally :)
>
>
> >The TCP issue [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.
>
> Ok, I have read the specification, and I think this can be solved very
> easily and at the
> right place:
> ModbusTCPTransport Class:
>
> public ModbusRequest readRequest()
> throws ModbusIOException {
> try {
> int transactionID = m_Input.readShort();
> int protocolID = m_Input.readShort();
> int dataLength = m_Input.readShort();
> if(protocolID!=0 || dataLength>256) {
> throw new ModbusIOException();
> }
> [...]
>
>
> }//readRequest
>
> We can either work with this Exception, or take a look if we need another
> one, to make sure we can differentiate the problems and close unilaterally
> when something is seriously wrong. Another option is to include a Flag,
> anything that differentiates, if such a differentiation to lead behaviour
> is necessary.
>
> Regarding the timeouts, well this could be achieved with keeping a
> reference to the Socket (i.e. m_Socket) and using
> m_Socket.setSoTimeout(0);
> Also not a big issue :)
>
> The remaining code actually does what the Specs request, however, the
> threading issues are handled by the JVM, rather then by us.
>
> Would you agree that this will properly do the job?
>
>
> > [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.
>
> Hmm probably I did not express myself very clear either. Yes you can have
> an absolutely standard construct that handles this problem. Java I/O
> proposes to "wrap" Streams into other streams for aggregating certain
> functionality.
> Here a piece from the Java API, java.io.FilterInputStream:
> "A FilterInputStream contains some other input stream, which it uses as
> its basic source of data, possibly transforming the data along the way or
> providing additional functionality"
>
> The catchword is transforming here.
> For ASCII and RTU this is exactly what you need, as the interface to the
> next level is cleary reading
> binary data, and not the encoded one.
> Imagine you have
> public class ASCIIInputStream extends FilterInputStream {
>
> private InputStream m_Input;
>
> public ASCIIInputStream(InputStream in) {
> m_Input=in;
> }//constructor
>
> public int read() {
>
> 1. read first character and update LRC
> 2. read second character and update LRC
> 3. transform the two into a byte
> 4. return byte
>
> }//read
>
> private void updateLRC(int char) {
> //mechanism to update LRC sum
> }//updateLRC
>
> public int getLRC() {
> //return actual LRC Checksum
> }//getLRC
>
> public void resetLRC() {
> //if necessary..
> }//resetLRC
>
> }//class ASCIIInputStream
>
>
> The ModbusASCIITransport would simply use this one, and the correct
> mechanisms
> necessary to implement the specs ;)
> Definately the above is a rough sketch, as I do not know whether each
> character is
> used for the LRC, or just the bytes, and there must be a mechanism to pass
> through
> the Frame Start colon and the CRLF in one turn, but that is if at all some
>
> if-elseif-else construct.
>
> I hope now it is clearer _why_ anything above this would never be
> confronted with this low
> level details.
> The responsibilities will be clearly assigned:
> 1. The ModbusASCIIInputStream is responsible for transformation of the
> encoding.
> it also calculates/updates the LRC on the fly.
> 2. The ModbusASCIITransport is responsible for handling the framing,
> including
> start, end recoginition, and LRC checking (by retrieving the checksum
> from
> the underlying stream.
> 3. The ModbusMessage implementation never sees encoded data or anything
> related to the framing.
> It is only responsible to read the data it knows and needs. If it does
> not work out, the Exception
> will anyway be passed through the ModbusTransport, and that is again
> the right instance with the
> responsibility to handle this.
>
> For the RTU this will be more or less the same, schema .....
>
> Don't get me wrong, but I think the complete construct is quite
> beautifully working hand in hand :)
>
>
> I hope that I could shed some more light now, and explain more clearly how
> this "working hand in hand" actually happens.
> If not, do not hesistate to ask, and also go ahead and put things into
> question, because this will definately be the way that leads us to an
> excellent and well thought out design respectively implementation.
>
>
> Thanks and best 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
|
