Removing deprecated driver code, etc. #267
Conversation
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.
This fixes the dithering problem; the Makerbot4GAlternateDriverTest now passes.
Which, incidentally, doesn't pass.
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).
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 driver. Anyway, things are better now.
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.
Made it clear that DriverBaseImplementation is not to be used on its own by marking it abstract and making the constructor protected. 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.
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.
Tightened access modifiers as much as possible. Removed commented-out code. Propagated @deprecated forward from Driver.
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.
Clearly, this method is never called.
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.
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.
This was too easy: the methods appear to be completely unused. We may consider removing them down the road.
Fortunately, most code was already using the new-style methods.
All code was already using the new-style methods.
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.
The methods were unused.
The method was unused.
Deprecated the no-argument valve command constructors so I can track them down later.
This completes the removal of all deprecated methods.
This is a step toward eliminating the tool=-1 hack.
This eliminates a few uses of the deprecated machine.getDriver() method, but not all. It seems to get pretty active use these days.
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.
P and T sure sound a lot alike. :-\
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.
Did you think a negative tool index means current tool? Not always! In this case it means "heated build platform." I love magic numbers.
All comparisons with NaN return false. FindBugs will tell you this. Y'all should seriously consider running some analysis tools over this code.
|
Awesome. 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! |
|
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. |
|
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 |
|
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. |
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.