On Sat, Jun 05, 2010 at 02:50:27AM -0400, Dave wrote:
> >> @@ -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
> >
> >
>
> Question: As you are poking through code like this to try and
> determine what to do, making changes, trying things out .... how do you
> not disturb the whitespace?
I usually use 'git gui' to review each part of my change before
committing. (in git gui's right click menu there are "stage selected
hunk" and "stage selected line"). When I see a part of a change is just
whitespace, I skip that part. If it's too tangled up to tease apart
using "stage selected", then I'll go back to my editor and change it
or clean it up until I can.
This is also useful for making changes into multiple commits .. more on
that below.
> > 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.
>
> I'd be pretty confident that no one is using EMC2 with a 300 baud Modbus
> connection.... but then again I might be wrong! ? I'm having a
> hard time understanding how 300 baud would be useful. If we need to
> support 300 baud then shouldn't we also support 110 baud? :-)
There's a difference between removing something that was offered in the
past and adding something new.
> My goal was to get Modbus to run out of the box without "tuning" these timers.
I think this is a good goal.
> >
> >> - 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.
> >
> >
>
> Ok. There are a lot of typos in this code, probably due to language
> differences. Many of the function names also have typos, which makes
> changing the code a little more difficult if you aren't cutting and
> pasting.
> Should typos and code reformatting normally be a separate commit/patch?
Yes. Each commit should do "one thing". I often find myself making a
series of patches each of which takes me towards my ultimate goal. For
instance,
* clean up typos in strings and/or comments
* make organizational changes that don't change behavior
* make the behavioral changes
if there's a lot of stuff -- for instance, in your submission there were
two major behavioral changes (size calculation and reception algorithm)
that could be split into two "behavioral changes" patches.
Ideally, the software will work at least as well as before after each
commit.
[snipped discussion of response size calculations]
OK, if you can determine which calculation is right for
MODBUS_FC_FORCE_COILS, I'll go ahead and get the length calculation
fixes committed. Thanks to some info from Chris M I now see that in the
context of classicladder's use of MODBUS_FC_DIAGNOSTICS (always using
sub-code 0000 for 'echo') it is possible to calculate the required
value.
> If VTIME is zero, then the timer is not functional so the read is
> suppose to block forever until VMIN characters are received.
.. but later it selects for 100*1000*VTIME microseconds, so if VTIME is
zero then the select will generally fail unless there's a delay before
select.
See my other message in this thread today for a program that uses VTIME
and VMIN.
> >> - }
> >> -#ifndef __WIN32__
> >> - pthread_exit(NULL);
> >> -#endif
> >>
> > You almost certainly didn't intend to remove this code.
> >
> Actually I did at the time. At the time when I cut that out, I didn't
> see any reason to keep any Win32 related code in there.
But that block is 'if not win32', i.e., the pthread_exit call is
compiled in everywhere *but* windows .. so you should leave the
pthread_exit call even if you're removing win32 support.
> Where do we go from here?
I'd like to verify the response size for 'force coils' and then get that
portion of the change committed to the git on linuxcnc.org. Next is
probably the modification of SocketModbusMasterLoop to split serial vs
ip more thoroughly, then the change to SerialRecive, then whatever's
left over.
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
