On 6/7/2010 11:59 AM, Jeff Epler wrote:
> 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.
>
>
I had no idea that could be done... I need to try that.... .
>>> 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.
>
>
You are making the assumption that 300 baud was working in the original
code. I didn't test the original code at 300 baud. Perhaps there
are other timing issues at 300 baud that come into play. The errors at
300 baud were unlike anything else I
saw at higher baud rates..
>> 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.
>
>
I see your point.
> [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.
>
Yes, that does not seem right... I'll need to check that out ..
> 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.
>
>
Arrrgghhhh, I missed that entirely ...........
>> 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
>
OK, sounds good. I'll try to get some time to work on this during the
weekend.
Thanks,
Dave
------------------------------------------------------------------------------
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
