ChangeClearSize() is broken for changing a via's solder mask clearance

[Robert Drehmel]

The special case ChangeClearSize(Selected, 0mm) is broken when used to change a via's solder mask clearance.
It does work as expected for pins.
A patch is found attached which makes the idiomatic sequence to easily provide the minimum solder mask clearance work again:

- selecting all pins and vias
- ChangeClearSize(Selected, 0mm)
- ChangeClearSize(Selected, +0.15mm)

[Bert Timmerman (bert-timmerman)]

Hi,

Converted into a

[Peter Clifton (pcjc2)]

The patch looks extremely suspicious to me, please hold off on this one.

[Peter Clifton (pcjc2)]

Just to clarify... reading the code in question (not just the patch) makes me want to tear my eyes out.

I appreciate that the patch brings the via case in line with the pin case, but two wrongs do not make a right here...

The "magic" behaviour of setting "0" to mean "oh - put it at the width of the pin", is evil, and should be removed from both cases.

[Peter Clifton (pcjc2)]

And to further clarify, even if this was desirable behaviour... the code is still an exercise in obfuscation, and should be re-implemented cleanly.

[Robert Drehmel (r58qft)]

Peter, I fully agree on the "should be re-implemented cleanly" part. The patch was meant to touch as little as possible of the fragile code surrounding it. ;) Additionally, the magic value shouldn't be "0", IMO, but rather some keyword.

[Bert Timmerman (bert-timmerman)]

Hi,

If I understand the above correctly it looks like there is some confusion with "MinMaskGap(Selected, 0.5mm)", which, according to he manual, should handle the soldermask gap, opposed to "ChangeClearSize(Selected, 0mm)" which is suppose to handle clearance with copper objects.

The descriptions regarding to soldermask clearance looks similar to the bug report #1744832 "Vias tented" (https://bugs.launchpad.net/pcb/+bug/1744832).

Kind regards,

Bert Timmerman.

[Chad Parker (parker-charles)]

Bert-

According to the manual this action is supposed to do double duty. If the solder mask layer is on, it's supposed to change the soldermask clearance. If the soldermask layer is off, then it changes the polygon clearance.

However, I've found that ChangeClearSize(SelectedVias, 0mm) appears to set the mask aperture to 0.05 mm instead of zero. So this appears to be broken nonetheless.

[Chad Parker (parker-charles)]

I've identified the problem(s). There are several places in the code path where a value of 0 used to indicate that the change should be relative instead of absolute.

When the action is called with the key strokes ctrl-k or ctrl-K, the "delta" argument get's set to a sign, either "+" or "-". When GetValue interprets this, it returns a value of 0. In ActionChangeClearSize, there's a statement, if(value == 0) ... that detects this, and assumes that it means that the function was called via keystroke. If you deliberately set this value to zero, then it gets misdetected.

Then in change.c, ChangeSelectedClearSize, "Absolute" is set to zero if "absolute" was detected properly in ActionChangeClearSize. In ChangeViaMaskSize, the zero-ness of "Absolute" is again used to detect if this should be a relative or absolute change.

There are several ways to fix this... I'm not sure what's best. Perhaps detecting the absolute-ness early, and then passing that through, all the way to the end. There are a lot of places where things like this are used, so, perhaps it could be worth actually creating a "size delta" structure of some sort...

[Chad Parker (parker-charles)]

Here's the solution I propose. It's not too hard, but it does touch a lot of things.

In change.c, "Absolute" should essentially become a boolean, and the value that something will be set to or incremented by be stored in "Delta". This will touch just about every function in change.c, as nearly all of them use the Absolute and Delta global variables.

Thoughts?

[Bert Timmerman (bert-timmerman)]

Hi Charles,

I can agree to "Absolute" becoming boolean (#10), what to do with the value stored in Absolute ?

Looking in ChangeViaClearSize() I see:

Coord value = (Absolute) ? Absolute : Via->Clearance + Delta;

where "Absolute" does a binary switch (if, then, else) already.

This would become something like:

Coord value = absolute ? new_size_var : Via->Clearance + Delta;

Needs to be documented to give a concise representation of the "why", "what" and "how" of the new implementation and behaviour (with doxygen comments, and the User Manual if we break current behaviour).

See the doxygen comment for the function, it would surprise me if the return value is "TRUE" ... ever !

I think it should be a pointer value of PinType (a copy-paste error in the comment I assume, which at the time of doxygenation was overlooked by me).

#9 (above) confuses me: I think I read that sometimes this "Absolute" value is mis-used? for detecting the value originates from keystrokes. This sounds as overloading to me.

When "Absolute" becomes a boolean it should be just for the purpose of the binary [absolute, relative].

And what members would a "size delta" struct contain ?

If there needs to be another boolean "Keystroke" to pass on the information that the value originated from keystrokes ... make it so.

If it becomes to cumbersome to pass all these booleans from function to function, than an int with bit-fields containing them may be a solution ... more stuff to put in documentation.

Bottom line: please keep it as simple as possible ... and document the code ... if I misunderstood, please elaborate with code ;-)

Kind regards,

Bert Timmerman.

[Chad Parker (parker-charles)]

re #9: I hadn't fully thought this through, I was thinking "out loud". Basically, I was thinking that instead of having 'Absolute' and 'Delta' be global variables as they are now, you could wrap them into some kind of structure and pass that structure around instead.

I may have thought out loud a little too much. The problem isn't detecting that it was a keystroke, it's just that the detection method is overloaded and masks other functionality.

What I'm thinking I'll do is essentially change "Delta" to "Value". Then the statement would become:

Coord new_value = Absolute ? Value : Via->Clearance + Value;

[Chad Parker (parker-charles)]

The other thing is, I don't like that these are global values... but given the way it's implemented right now, there's no real way around it.

I better solution would be to change ObjectOperation such that you can pass user data through an argument, sort of like how you can pass user data to the callbacks in the gtk hid.

[Chad Parker (parker-charles)]

Another issue I've found here is that there is code in place when changing the polygon clearance that tries to enforce a minimum clearance. This presently prohibits the setting of zero, and actually belongs in the DRC (in my opinion). I'll open a separate bug report for that.

[Chad Parker (parker-charles)]

I've pushed a branch that is ready for review and test: LP929123. The branch contains a new set of tests that should address the issue in this report.

[Bert Timmerman (bert-timmerman)]

pushed to master