From: Dieter W. <wi...@oe...> - 2001-10-22 11:37:25
|
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 |