use multiple steps when milldrilling large holes#692
use multiple steps when milldrilling large holes#692tomjnixon wants to merge 11 commits intopcb2gcode:masterfrom
Conversation
this avoids leaving a chunk of material in the middle of the hole when milldrilling holes larger than twice the cutter diameter, which can cause various problems
2fd1be2 to
b1dade8
Compare
|
I haven't looked at the code yet, I just want to say, "WOW!". Great work on adding a feature into some complex code! I will take a look at this! |
| entry_diameter + step * diameter_step, step == steps); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Is this indeed always true? I haven't worked out all the math but, if min_entry and max_entry are small enough, is it possible that we do multiple steps and yet the tool is better than the hole/slot to mill?
I honestly don't even remember why returning true/false here was important. I think that it might be in order to display a warning to the user about attempting to mill a hole that is smaller than the milling tool!
There was a problem hiding this comment.
The only thing this is used for is to print this warning
Lines 716 to 717 in b1dade8
If the rest of the logic is correct (and the parameters are validated), returning true is correct because that bit of code always cuts multiple passes, and there is no reason to enlarge a hole that has already been cut too big.
I also wasn't sure what to do with this condition. As it is I think it makes more sense for milldrill_one to return void, and move the condition to milldrill. Does that make sense?
An alternative would be just returning the last status returned by milldrill.
It could be higher still, but i can see some logic to deciding this in the milldrill function, for example if you decide to do something smart, like enlarging circular holes (so that they are never plunged), but allowing slot milling (which makes its own clearance), that would be the right place for it.
Alternatively, the logic for why it's "correct" is:
- in
milldrill_one, the condition for returning false iscutter->tool_diameter * 1.001 >= holediameter - so the true condition is
cutter->tool_diameter * 1.001 < holediameter - in milldrill, the true condition is:
max_entry <= holediameter - which is equal to
cutter->tool_diameter * max_milldrill_entry_diameter <= holediameter max_milldrill_entry_diametershould be >= 100% (though in reality it would be odd to set it that low), so the conditions are nearly the same.
... but the existing condition is not exactly right, because it will return false (and issue a warning) if using a 1mm cutter to mill a 1mm slot, which is perfectly valid. I think we should fix this while we're thinking about it and understand it.
There was a problem hiding this comment.
The 1.001 is there to deal with rounding. There are all sorts of floating-point problems with pcb2gcode and this is how we deal with it!
If you have a suggestion for another improvement, I'm all ears.
There was a problem hiding this comment.
Right, that tolerance is required to not produce very small arcs, but the condition for the warning should a bit different, i think.
Currently it works like this for a 1mm cutter:
- holesize >= 1.001mm: use arcs, no warning
- holesize < 1.001mm: no arc, issue warning
The warning is not quite right if holesize = 1.0mm
Instead it should be:
- holesize >= 1.001mm: use arcs
- holesize < 1.001mm: no arcs
- holesize >= 0.999mm: no warning
- holesize < 0.999mm: no warning
This way, a 1mm hole doesn't use arcs, but also doesn't get a warning.
|
I appreciate your efforts! Your code fits so well into the codebase that it looks as if I wrote it! I imagine that you did that on purpose, to maintain consistency. That's good, it helps readers of the code so that they don't need to go back and forth between "dialects". The coverage tool is complaining but I think that it's not a real complaint. Coveralls links are having some trouble at the moment. I'll check it again later. |
|
Thank you for the kind words, and for having a look; it seems like quite a nice codebase to work on! I'll check the coverage manually when i've added the new test; coveralls may well be right. |
|
Coveralls is working again and the main chunk of your code seems to have good coverage: https://coveralls.io/jobs/156533579/source_files/36521372103#L575 The coverage shows yellow sometimes because not all branches are taken. Don't worry much about that. Sometime the coverage will detect that, for example, some path with an exception was missing or something. |
- pull this condition up to millhole, where it makes more sense - correct the logic, so that if the hole and the cutter are the same size, no warning is issued
it's easier if this side always does multiple passes
this makes sure that if using a 1mm bit with max-milldrill-entry-diameter=200%, a 2mm hole will always use one pass as expected this probably makes the divide-by-zero check in the other case redundant...
add tolerance to the step calculation (which additionally means that the tolerance works for multiple steps), and use that to select the strategy, avoiding having to handle the steps < 1 case in the multi-step arm
this would only happen really rarely (in the tolerance region), and doesn't cause a problem, but correct is better
4c5a9c9 to
651fb8f
Compare
This avoids leaving a chunk of material in the middle of the hole when milldrilling holes larger than twice the cutter diameter, which can cause various problems.
It isn't quite finished, but it would be good to get some feedback on the general ideas before polishing it.
Currently when holes are milldrilled, they are always cut in a single pass. For holes larger than about twice the cutter diameter, this can cause problems:
The idea is to avoid this by cutting larger holes in multiple passes. The existing
millholeroutine is used to make each pass, so the code change is quite minimal, but the parameters are the tricky bit.parameters
The new parameters are:
Multiple passes are used for holes larger than
max-milldrill-entry-diameter. When multiple passes are used, the first pass is betweenmin-milldrill-entry-diameterandmax-milldrill-entry-diameter, with the radius increasing by up tomilldrill-stepovereach time.This is in addition to
min-milldrill-hole-diameter, which is unchanged. Originally, I used the value, formin-milldrill-hole-diameterasmin-milldrill-entry-diameter, but this doesn't quite work right.For example, it might be acceptable to use a 1mm cutter to make 1.1mm holes. This will go slowly (
milldrillslows it down to the vertical feed), and recut a lot of chips, but maybe it works.If these parameters were the same, making a 2mm hole with a 50% stepover would result in two passes, at 1.1mm, and 2mm. This is not ideal, because there's no reason in this situation for the first pass to be so small -- the first pass is not ideal for chip evacuation, and the second pass removes much more material than the first.
The current parameters would result in passes at 1.5mm and 2mm, which should be better on both counts.
min-milldrill-entry-diameterandmax-milldrill-entry-diameteralso seem like they could be one parameter, but having a range increases the chance that a single pass can be used, and avoids having passes too close together for holes just larger than the maximum for a single pass.These are all relative to the cutter size, because that seems like the most natural way and makes the defaults make sense. I was also thinking about adding support for multiple milldrill cutter sizes (which would be handy if you want to make small slots/holes and mounting holes on one board), and percentages make more sense here too.
compatibility
Currently this change does affect the default behaviour; if the old behaviour was considered to be problematic than maybe that's OK? There's currently no clean way to switch to the old behaviour, except for setting
max-milldrill-entry-diameterto a large number (the parser doesn't seem to acceptinf).results
Here's what making holes and slots sized 2.17, 2.59, 2.73, 3.57, 3.99 and 4.13mm with a 1.4mm bit (chosen to show the maximum variation in stepover) and default settings looks like from the side/top:
And a zoom on one showing the travel between passes (which is one turn above the work):
default parameters
The defaults are a bit arbitrary, and relatively conservative. I'm most unsure about the default stepover; 50% seems conservative compared to slot milling (100%), but these situations are a quite different, I haven't had a chance to try them out yet; if anyone else wanted to try it and report back that would be very helpful.
todo