I don't have any real modbus hardware to test this on, but if there are
problems with the classicladder modbus function I'd like to get the fix
into a future 2.4.x release. Since classicladder is Chris Morley's
project, I feel that it's primarily up to him what patch should
ultimately go in.
Chris or anyone else, do you have hardware to test this on, particularly
hardware from a different vendor than the Siemens PLC that Dave tested?
I have an Arduino with FreeMODBUS firmware and may be able to
temporarily use an Automation Direct VFD. (I was able to use function 4
on the Arduino in classicladder before Dave's modifications; I haven't
tried with the patch, or any functions besides 4)
Before I get in to my remarks about the proposed patch, I want to thank
Dave for doing the work and sharing the results of that work. He fixes
important bugs and I want to make a big deal out of things like where he
adds spaces! But code organization really is important -- probably the
idiosyncratic way that much of classicladder is written was tough for
Dave to understand the first time (I know it has been for me as I read
the existing code while reviewing this patch). We can improve this over
time, but only by holding new submissions to a higher standard than is
seen in the existing code.
> Because I removed all hard coded timers, the Modbus Com thread can now
> put significant load on the CPU if run without either a delay after
> transmit or an inter transaction delay.
This is particularly worrisome to me. This means you're busy-waiting,
and you've identified the consequence of it. I didn't see what part of
your patch busy-waits but it must be there.
More notes about specific parts of the proposed change:
> +/* Modified by Dave Cole 5/24/10 to increase serial receive reliability,
> + correct errors in EMC2 implementation version*/
There's no need to add comments like these. The git history will
record who made the change and the reason given for the change.
Repeating it in the source file is unhelpful. (lots of my notes, such
as this one, apply in multiple places)
> @@ -40,8 +43,6 @@
> #endif
> GtkWidget *LabelParam[ NBR_OBJECTS ],*ValueParam[ NBR_OBJECTS ];
>
> -
> -
> #define NBR_IO_PARAMS 6
> GtkWidget *InputParamEntry[ NBR_INPUTS_CONF ][ NBR_IO_PARAMS ];
> GtkWidget *InputDeviceParam[ NBR_INPUTS_CONF ];
Try to avoid making unrelated whitespace changes
> -static char * SerialSpeed[] = { "300", "600", "1200", "2400", "4800",
> "9600", "19200", "38400", "57600", "115200", NULL };
> +static char * SerialSpeed[] = { /*"300",*/ "600", "1200", "2400", "4800",
> "9600", "19200", "38400", "57600", "115200", NULL }; // Dave Cole
Unless we're nearly certain no real users use 300 baud (i.e., because it
never could have worked before (why?)), we should leave it in.
Again, there's no need to mark lines that you changed with your name.
git tracks this for us.
> - sprintf( BuffLabel, "After transmit pause -
> milliseconds" );
> + sprintf( BuffLabel, "Dwell between Xmt and
> Rcv-millisecs" );
I don't think these wording changes are improvements. "pause" and
"delay" are better than "dwell", and the original didn't have to
abbreviate "Xmt" and "Rcv".
> - MessageInStatusBar("Openned Configuration window. Press again
> to update changes and close");
> + MessageInStatusBar("Opened Configuration window. Press again to
> update changes and close");
This is an obvious typo fix which is fine but even better as a separate
commit.
> - LgtResp++;//for function code
> + // LgtResp++;//for function code where does LgtResp come
> from????
Delete code, don't comment it out.
Don't add comments that mostly reflect that the programmer didn't
understand the code, such as 'where does LgtResp come from????'.
Incidentally, it was declared just above at the top of the function and
initialized to zero:
| int LgtResp = 0, NbrRealBytes;
but like you I find the manner in which the final value of LgtResp is
computed to be hard to love.
> switch( CurrentFuncCode )
> {
[changes snipped]
It was hard for me to see what changes you intended here. I wrote a
test program which incorporates the previous definition of this
function, your modified version, and a function I wrote from scratch
based on reading a modbus protocol documentation.
This table summarises the functions for values where they differ (27
cases in all were tested and the rest agreed):
func size orig dave jeff
0x05 1 -> 6 5 5 # FORCE_COIL
0x0f 1 -> 6 2 5 # FORCE_COILS
0x0f 7 -> 6 2 5
0x0f 8 -> 6 2 5
0x0f 15 -> 6 2 5
0x06 1 -> 6 5 5 # WRITE_REG
0x10 1 -> 6 5 5 # WRITE_REGS
0x10 7 -> 6 5 5
0x10 8 -> 6 5 5
0x10 15 -> 6 5 5
0x08 1 -> 6 4 -1 # DIAGNOSTICS
So in most cases your new code agrees with mine (0x5, 0x6, 0x10).
We disagree on 0xf and 0x8.
First, 0xf, Write Multiple Coils AKA Force Coils. According
to the documentation I am reading, the Response PDU consists of
1 byte - function code (0xf)
2 bytes - starting address
2 bytes - quantity of outputs
giving a total of 5 bytes. How did you arrive at 2 bytes? Did your
testing cover any MODBUS_FC_FORCE_COILS requests?
Next, 0x8, Diagnostics. According to the documentation I am reading,
the response consists of
1 byte - function code (0x8)
2 bytes - subfunction code
? bytes - data (depends on subfunction and data in request)
therefore, I don't believe it is possible to calculate the number of
bytes in a diagnostics reply PDU based only on the function code and
number of elements. I also don't see an opportunity to send a
Diagnostics request in classicladder. Did your testing cover any
MODBUS_FC_DIAGNOSTICS requests?
> + case MODBUS_FC_FORCE_COILS: //14
Don't add comments that purport to give the value of a constant or
enumerant. It's even worse when the comment is incorrect, as it is
here:
protocol_modbus_master.h:#define MODBUS_FC_FORCE_COILS 15
> - FindNextReqFromTable( );
> + FindNextReqFromTable( );
these two lines differ because the "+" line has added whitespace. Try
to avoid making whitespace changes, because they make diffs noisy.
> @@ -193,46 +201,147 @@ void SerialSend( char *Buff, int BuffLength )
> void SerialSetResponseSize( int Size, int TimeOutResp )
> {
> if ( PortIsOpened )
> - {
> - newtio.c_cc[VMIN] = Size; //Nbr chars we should receive;
> - newtio.c_cc[VTIME] = TimeOutResp/100; // TimeOut in 0.1s
> -// tcflush(fd, TCIFLUSH);
> + { // "fd" (file device) control for serial port per termios
> docs
> + newtio.c_cc[VMIN] = Size; //Nbr chars we should receive (This
> doesn't seem to work, read returns immediately, regardless)
> + newtio.c_cc[VTIME] = TimeOutResp/100; // TimeOut in 0.1s (Read
> returns immediately and doesn't block as it should? )
> if ( ModbusDebugLevel>=2 )
> printf("Serial config...\n");
> - tcsetattr(fd,TCSANOW,&newtio);
> + tcsetattr(fd,TCSANOW,&newtio);// TCSANOW - means make changes
> immediately
> }
> }
Did you ever look at the value actually stored in VTIME as I suggested?
One thing that occurs to me as I read it now is that for TimeOutResp
less than 100 it's setting VTIME to 0. (TimeOutResp+99)/100 will round
up instead.
The other reason for the old code to fail would have been bad (too
small) lengths calculated in GetModbusResponseLenghtToReceive but in all
the cases I examined the original code returned the same value or a
longer value than the new code, so this is probably a red herring.
> +int SerialReceive( char * Buff, int framelength , int TimeOutResp )
the body of this function is essentially all-new so you should be
consistent on indentation throughout.
[most of body snipped except where I want to comment]
> if ( PortIsOpened )
.. exits without returning a value if this condition is false
> -tv.tv_usec = newtio.c_cc[VTIME]*100 *1000; //micro-seconds
I still think that the old function seemed "pretty right", but my VTIME
concern is repeated here. If VTIME was calculated as 0 above, tv will
be {0,0} indicating that select should return immediately. Using
TimeOutResp * 1000 here would make more sense.
> + while ((select_ret = select(fd+1, &myset, NULL, NULL, &tv)) ==
> -1)
> + {
> + if (errno == EINTR)
If any other error occurs (such as EBADF or EINVAL, one of which you can
readily make occur by using a USB serial interface and unplugging it
while the program is running) you'll loop forever here (and again
below).
> + while (select_ret) // select_ret is >1 or true if select() has
> found chars are in buffer
comment is wrong. the loop will execute as long as select_ret != 0.
Since you handle the possible negative results, other accurate comments
would say >0 or >=1. On the other hand, if you really think it's
important to call attention to the positive nature of select_ret(?) then
put it in the code and ditch the comment:
while(select_ret>0)
> + if (read_ret == -1)
> + { // read failed
> + if ( ModbusDebugLevel>=2 )
> + printf("Read Port failure\n");
> + return 0;
> + }
please use good indentation practice in new code.
> - int ResponseSize;
> + int ResponseSize=0;
this is good because otherwise ResponseSize can be returned
uninitialized when select returns <= 0. On the other hand, it's
unrelated to the main thrust of this patch (making serial work) so
prefer to submit as a separate patch.
> @@ -308,80 +311,111 @@ void SocketModbusMasterLoop( void )
I had trouble making out just what you did in this function, because the
diff is like an omelet, but it looks like the old code had a structure
if(socket) A1
else /* serial */ A2
B
if(socket) C1
else /* serial */ C2
D
with common code interleved, and you changed it to
if(socket) {
A1
B
C1
D
} else /* serial */ {
A2
B
C2
D
}
and then went on to modify B and D in one or both sides of the 'if'.
That's fine, and it's not entirely your fault that the diff comes out so
hard to read.
> + ResponseSize = SerialReceive(
> ResponseFrame, 1/*adr*/+GetModbusResponseLenghtToReceive()+2/*crc*/,
> ModbusTimeOutReceipt );
> + NbrFrames++;
> +
> if ( ResponseSize==0 )
> printf("ERROR CLASSICLADDER- MODBUS NO
> RESPONSE (Errs=%d/%d) !?\n", ++CptErrors, NbrFrames);
A ResponseSize different than expected--not just a zero-byte response--
should be an error. It's unlikely that a short transmission would pass
the CRC test in TreatModbusMasterResponse but it will happen at a rate
of about 1 in 2^16 random short packets.
[I see now that this code is unchanged compared to the original, so
don't sweat it.. on the other hand, something to fix for the future...]
> + else // no question to ask - pause for 300 ms
> + DoPauseMilliSecs( 50 );
Again, comments are worse than useless when they lie about what the code
does.
> - }
> -#ifndef __WIN32__
> - pthread_exit(NULL);
> -#endif
You almost certainly didn't intend to remove this code.
Jeff
------------------------------------------------------------------------------
ThinkGeek and WIRED's GeekDad team up for the Ultimate
GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
lucky parental unit. See the prize list and enter to win:
http://p.sf.net/sfu/thinkgeek-promo
_______________________________________________
Emc-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/emc-developers
