Hi Seb, responses inline below.
On May 24, 2015, at 3:28 PM, Sebastian Kuzminsky <s...@highlab.com> wrote:
> The patch file you attached claims to be the third patch in a series of
> 3, but numbers 1 and 2 were not attached. Was that intentional?
>
> It applies cleanly to origin/2.7 and builds, so maybe everything's ok
> and i shouldn't freak out.
When I used the git command (git format-patch HEAD~~) to create the patch it
created three patches. Mine was the third so that is what I sent. The other
two were from other changes in the branch I checked out apparently…? I just
did it again for this updated patch and I got 2 patches, mine being the second
(attached below).
> There's a typo (missing white space) in the --help output for —disable.
Fixed.
> Inconsistent indentation in the new fields in the haldata_t struct.
Fixed.
> There's a spurious indentation change in the main loop ("the meat of the
> program"). Not a big deal, but it does make the patch a bit harder to
> follow.
Fixed.
> Why do you prefix all your comments with "---"? My eyes keep trying to
> parse that as command-line arguments.
I used that to be able to easily search and identify my changes. I meant to
remove them. They are now gone.
> If the comp is enabled and transitions to the disabled state, it does
> one last write to turn off the spindle. Presumably in this use-case an
> estop causes the VFD to lose input power and causes the gs2 comp to move
> to the disabled state, but the VFD has enough power in its caps to
> accept this last command before it dies?
Generally, the VFDs internal caps can keep it running for many seconds after it
loses power (~10-15s), so issuing a zero spindle speed command is an extra
safety measure. If the command fails because the VFD is off, then no harm is
done. Also, we can't assume the hardware will power down the VFD... in some
machines it may not be wired to do so, or the relay could fail, or etc. etc....
Better safe than sorry.
> In the main loop, when the comp tries to initialize the VFD after
> becoming enabled, it responds to communication failures by running
> "continue" before setting isInitialized to True, so shouldn't that
> handle the communications retry that you added to gs2_set_accel_time()?
As I recall, the extra retry in gs2_set_accel_time() is there purely to avoid
the error message that is printed in gs2_set_accel_time() if the first attempt
fails. It is correct that the "continue" in the main loop will come back and
try again even without the auto-retry in gs2_set_accel_time(), but not before
an error message is printed, and the error message was annoying me :-)
> Thanks for working to make the gs2 driver better. This will be a good
> addition once these minor nitpicks are addressed.
Thanks,
-Tom
------------------------------------------------------------------------------
_______________________________________________
Emc-developers mailing list
Emc-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/emc-developers
