Removing deprecated driver code, etc. #267

wants to merge 36 commits into


None yet
2 participants

cbiffle commented Mar 10, 2012

This is the second phase of what I sent you last night. I've removed about 1800 lines of code in total. All drivers now speak in terms of explicit tool indices, instead of leaving them implicit.

I had to modify the G-Code generator to allow T codes on a few more commands. Hope that's okay.

cbiffle added some commits Mar 9, 2012

Added a (failing) test to demonstrate dithering.
The Makerbot-derived printer drivers contain a math error that
manifests several different ways: Z-hold unexpectedly engaged during
a print; wobbles on otherwise straight edges; increased loss of
steps (including Z steps) during rapid motion, as in fills.

This test case demonstrates the bug.  The next commit fixes it.
Removed the excess-tracking code from the drivers.
This fixes the dithering problem; the Makerbot4GAlternateDriverTest
now passes.
Fixed compilation errors in MachineLoaderTest.
Which, incidentally, doesn't pass.
Fixed incorrect override that disabled stepper fan.
I've added @Override annotations to all overrides.  This revealed
a bug: the enableMotor/disableMotor, despite being documented as
overrides, weren't.  As a result, the Sanguino3GDriver behavior
was being invoked, generating DC motor control messages even when
the axis has been hijacked for a stepper extruder.  Because this
driver reuses the same motor control channel for the stepper fan,
this meant the fan turned on...until the first motion.  Then it
started toggling on and off with the stepper motor.

That's kind of silly, so with this change it obeys the flag passed
to enableStepperExtruderFan(boolean).
Improved code reuse in Makerbot4G*Driver.
While Makerbot4GAlternateDriver was technically a subclass of
Makerbot4GDriver, it was basically copied and pasted, duplicating
most of the code.  The duplicated code has diverged in some places;
I've preserved those where they seem significant, even though I'm
not sure they're correct.  (In particular, setMachine behaves
slightly differently in the two drivers.)

This became pretty apparent under a debugger, where an instance of
Makerbot4GAlternateDriver had *two* copies of all its fields...
because they were defined as private in both classes.  This helped
to mask the bug (fixed in a previous commit) that disabled the
extruder cooling fan.

More subtly, the two drivers used to override slightly different
subsets of Driver.  This meant that my fix last night to stop
toggling the DC motor controls didn't help the original Makerbot4G

Anyway, things are better now.
@Deprecated/@Override cleanup in Driver classes.
I've deprecated some new methods on Driver.  These methods -- or,
more specifically, the fact that two versions of each of these
methods existed -- hid the stepper fan bug I fixed in a previous
commit.  Interestingly, many of the Driver implementations
deprecated these methods, but the annotations never got propagated
back to the interface, and so some were missed.

At the same time, I added @Override where appropriate to a subset
of the Driver classes (those I use).  The interesting thing about
that change is the methods that *didn't* get @Override.  Driver
defines no multiple-tool-aware version of e.g. getMotorRPM, but
some of the drivers attempt to override it...resulting in dead
code.  To be fixed in a subsequent commit.
Cleaned up DriverBaseImplementation, mostly.
Made it clear that DriverBaseImplementation is not to be used
on its own by marking it abstract and making the constructor

Put big scary comments on methods that appear to have been
attempts at overrides, but which actually overload methods in
Driver.  Because the rest of RepG manipulates Drivers through
the Driver interface -- not as objects of type
DriverBaseImplementation -- this means all these functions are
dead!  Which is too bad, because they're awesome.  Marked them
for eventual transfer to Driver.

Removed all commented-out code and fields.

Tightened access modifiers as far as possible (e.g. fields to
private, methods to protected).  This revealed that some code
and fields were dead, but not being detected as such because
they were declared as visible; removed all that.

Routed all internal setting of currentPosition through the
setInternalPosition method, which appears to have been provided
for this purpose.  Inlining it would also be a reasonable choice.

Propagated @Deprecated annotations forward from Driver.

There's still some stuff left to do here, but the change does too
many things already.
Cleaned up Sanguino3GDriver, mostly.
Tightened access modifiers as much as possible.

Removed do-nothing overrides that just call the super impl.

Removed a dead null check (line 349).

Removed commented-out code.

Propagated @Deprecated forward from Driver.

Did not remove all dead code.  This driver contains the same
attempted overrides as DriverBaseImplementation; I've left
them private, so that they light up on a dead code analysis
pass but aren't lost to the mists of time (or git log).  I
expect to expose these shortly.
Cleaned up Makerbot4GDriver.
Tightened access modifiers as much as possible.

Removed commented-out code.

Propagated @Deprecated forward from Driver.
Miscellaneous cleanups in driver package:
Added @Overrides.

Removed unused classes (one of which appears to have a years-old
comment asking why it existed).

Fixed equals/hashCode contract violation in Version.
Fixed infinite recursion in setMotorDirection.
Clearly, this method is never called.
Modernizing Driver's API.
ReplicatorG is lurching toward supporting multiple toolheads.
The Sanguino3G driver, in particular, has versions of methods
like readToolStatus or getMotorSpeedPWM that take an explicit
toolhead index.

Unfortunately, as shown by my previous commit that moved those
methods to private, nothing ever called them.  This is because
the rest of the code uses the Driver interface, which has only
*some* of these methods.  The migration was partial.

This commit finishes it, with the exception of some little-used
features that I don't understand well enough to test (collets,
for one).  All Driver methods that apply to toolheads now take
an explicit index.  The old versions are still here, but they're
explicitly @Deprecated, so I can easily find all callers to
modernize them, too.

In a surprising bit of honesty, the code describes the practice
of interpreting tool -1 to indicate "current tool" as a "fast
hack to get software out."  I am perpetuating the fast hack in
this commit: all the deprecated methods forward to the modern
ones, but with a tool index of -1.  I will fix this in a
subsequent change.

Note that the RepRap drivers are not fully prepared to support
multitool, and this commit does not change that.
Modernized DriverQueryInterface.
While the Driver interface grew new methods to support multiple
tools, DriverQueryInterface was left behind.  Fortunately, it's
a much less popular type, so fixing this is a relatively small
change.  Because classes tend to implement both Driver and
DriverQueryInterface, and Driver already had the methods in
question, I was able to simply remove the old-style methods
from DriverQueryInterface.

As a side effect, a few G-Code commands are now multi-tool
aware -- specifically, the commands for setting spindle speed.
Removed old-style tool status methods from Driver.
This was too easy: the methods appear to be completely unused.
We may consider removing them down the road.
Removed old-style methods for motor speed/dir from Driver.
Fortunately, most code was already using the new-style methods.
Removed old-style motor enable/disable methods from Driver.
All code was already using the new-style methods.
Removed old-style temperature control methods from Driver.
The commands support a deprecated form that infers the current
tool selection.  They're already equipped to pass -1 into the
Driver, which is still supported by all drivers.
Removed old-style valve control methods from Driver.
Deprecated the no-argument valve command constructors so I can
track them down later.
Removed old-style spindle methods from Driver.
This completes the removal of all deprecated methods.
Most (all?) commands now take explicit tool indices.
This is a step toward eliminating the tool=-1 hack.
Added driver name and stop info to DriverQueryInterface.
This eliminates a few uses of the deprecated machine.getDriver()
method, but not all.  It seems to get pretty active use these days.
Added code to defend against the negative tool hack.
I'm pretty sure that all sources of negative tool indices have
been stamped out at this point.  This code will detect further
uses and report them in the log.
Fixed a typo introduced in 06a13f0.
P and T sure sound a lot alike. :-\
Cleaned up MCode parser to use 'tool' field.
My changes from the past few days were explicitly extracting
the 'T' code from the command, when code at the top of the
method had already done this.
Fixed HBP control hack in ExtruderPanel.
Did you think a negative tool index means current tool?  Not always!
In this case it means "heated build platform."  I love magic
Removed bogus NaN check in ExtruderPanel.
All comparisons with NaN return false.  FindBugs will tell you this.
Y'all should seriously consider running some analysis tools over this

FarMcKon commented Mar 12, 2012

I think we will pull about 95% of these changes in, they are very helpful.

As I mentioned before, I am going to leave those deprecated calls in our version of the repo for 6 months or so. Most were only marked deprecated around Dec/Jan, and we need some time to let people get up to speed on their removal. They are left in place as reference as our changes merge and get pulled out into the wider community.

Thanks for the awesome code cleanup. This is work several of us have daydreamed about doing, but haven't made time for. It's a huge help to the whole community. Thanks!

cbiffle commented Mar 12, 2012

My pleasure! Eclipse, FindBugs, and friends made short work of it.

I should note that these changes, in addition to cleaning up deprecated code, actually fix several subtle bugs. I noticed in diffing the S3G output that a few confusing commands are no longer being emitted. I've tracked it down to the duplicate methods on Driver -- some drivers implement one or the other, inconsistently, and the code that uses the drivers is similarly inconsistent. As a result, important code in Sanguino3GDriver (for example) was never getting invoked!

I understand your desire to keep the deprecated methods around, but having the methods in the codebase is dangerous. Maintainers may fix bugs in one implementation but not the other, or code may use the wrong one. If you're set on keeping the code, I suggest picking one of the two sets of methods (probably the one that takes a toolhead index, the ones I call "new-style") as canonical, and ensuring that every implementation of the old-style methods calls directly through to the canonical ones -- and that no drivers ever override the non-canonical method, because the code may never get invoked!

If this sounds like a reasonable step in the right direction, I can help with the change.


FarMcKon commented Mar 12, 2012

Yeah, I agree all deprecated calls need to re-direct to the canonical version of that function call. If you want to grab that test/verification and run with it, that would be awesome.

hack on, - Far McKon

cbiffle commented Mar 12, 2012

Okay, since this is going to take a non-trivial chunk of time and cause my original cleanup branch to require some difficult merging, I want to have another go at convincing you otherwise.

The system lives in version control. If you're concerned about third-party forks keeping up with the deprecated API removal, we can always fix the system in a branch, or lay down a tag where the deprecated API still exists and move on. A heads-up on the development list could warn people to merge their changes. In practice, any driver that needs changes is already broken like the Sanguino3GDriver was -- it is probably not getting called the way the authors expect!

In general, I'm suspicious of deprecation, because it's open-ended: it's too easy to deprecate something and never clean it up. (This is particularly true when you're focused on maintaining compatibility that may or may not be necessary -- Java's Thread.stop comes to mind, deprecated since 1.1 and still going strong.) The only reliable time to deal with it is right now -- anything else can get pushed back by things that look more important in the short term, like getting a feature out the door. I know you guys are understaffed, and that's part of what worries me, which is why I'm trying to clean things up as soon as possible.

All code has a cost. In the case of this code in particular, it has a significant semantic cost by complicating Driver's interface and call structure, as well as an overhead on maintenance by opening up many wrong ways to do something. The wrong ways are not theoretical -- as I mentioned, Sangino3GDriver uses a few of them and has bugs as a result. One of them, the stepper cooling bug introduced sometime around 0031, could actually kill users' hardware. The code is not tested thoroughly enough to be confident that we won't re-introduce bugs like this as long as the wrong way to do it exists.

If you're dead-set on keeping the old-style methods around, I can try to make it as bulletproof as possible given the constraints. However, from the code, it doesn't look like everyone is paying attention to compiler warnings, so I can't guarantee that anything I do to the code (like propagating @deprecated into all the right places or writing something to the log) will actually catch anyone's attention. The best way to get attention is with a compiler error; anything else can be muted or ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment