Skip to content

use multiple steps when milldrilling large holes#692

Open
tomjnixon wants to merge 11 commits intopcb2gcode:masterfrom
tomjnixon:multi_step_milldrill
Open

use multiple steps when milldrilling large holes#692
tomjnixon wants to merge 11 commits intopcb2gcode:masterfrom
tomjnixon:multi_step_milldrill

Conversation

@tomjnixon
Copy link
Copy Markdown

@tomjnixon tomjnixon commented Nov 24, 2024

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:

  • For holes larger than twice the cutter diameter, a piece of material will be left in the centre of the hole. When this breaks loose, it can get stuck between the cutter and the work.
  • For holes around twice the cutter diameter, the cutting geometry is not ideal; the cutter may rub rather than cut the thin unsupported part near the centre.

The idea is to avoid this by cutting larger holes in multiple passes. The existing millhole routine 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:

  --min-milldrill-entry-diameter arg (=150 %)                                                                   
                                        minimum initial hole width for                                          
                                        milldrilling when multiple steps are                                    
                                        used, as a fraction of                                                  
                                        milldrill-diameter                                                      
  --max-milldrill-entry-diameter arg (=190 %)                                                                   
                                        maximum initial hole width for                                          
                                        milldrilling in a single step, and the                                  
                                        minimum size of the initial hole when                                   
                                        multiple steps are used, as a fraction                                  
                                        of milldrill-diameter                                                   
  --milldrill-stepover arg (=50 %)      maximum hole radius increase when                                       
                                        milldrilling in multiple steps, as a                                    
                                        fraction of milldrill-diameter                                          

Multiple passes are used for holes larger than max-milldrill-entry-diameter. When multiple passes are used, the first pass is between min-milldrill-entry-diameter and max-milldrill-entry-diameter, with the radius increasing by up to milldrill-stepover each time.

This is in addition to min-milldrill-hole-diameter, which is unchanged. Originally, I used the value, for min-milldrill-hole-diameteras min-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 (milldrill slows 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-diameter and max-milldrill-entry-diameter also 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-diameter to a large number (the parser doesn't seem to accept inf).

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:

milldrill_iso1
milldrill_xy

And a zoom on one showing the travel between passes (which is one turn above the work):

milldrill_one

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

  • validate the new parameters (0 < stepover <= 100, 100 < min, < max)
  • update the man page
  • add integration test with the above example

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
@tomjnixon tomjnixon force-pushed the multi_step_milldrill branch from 2fd1be2 to b1dade8 Compare November 24, 2024 14:06
@eyal0
Copy link
Copy Markdown
Contributor

eyal0 commented Nov 24, 2024

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!

@coveralls
Copy link
Copy Markdown
Collaborator

Coverage Status

coverage: 73.944% (-0.04%) from 73.984%
when pulling b1dade8 on tomjnixon:multi_step_milldrill
into 8c084af on pcb2gcode:master.

Comment thread drill.cpp Outdated
entry_diameter + step * diameter_step, step == steps);
}

return true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing this is used for is to print this warning

pcb2gcode/drill.cpp

Lines 716 to 717 in b1dade8

cerr << "Warning: " << badHoles << ( badHoles == 1 ? " hole was" : " holes were" )
<< " bigger than the milling tool." << endl;

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 is cutter->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_diameter should 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eyal0
Copy link
Copy Markdown
Contributor

eyal0 commented Nov 24, 2024

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.

@tomjnixon
Copy link
Copy Markdown
Author

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.

@eyal0
Copy link
Copy Markdown
Contributor

eyal0 commented Nov 24, 2024

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
@tomjnixon tomjnixon force-pushed the multi_step_milldrill branch from 4c5a9c9 to 651fb8f Compare November 25, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants