skidl icon indicating copy to clipboard operation
skidl copied to clipboard

ERC warning for parts that are obviously incorrectly connected

Open voneiden opened this issue 5 years ago • 3 comments

Is your feature request related to a problem? Please describe.

I had a case where I was intending to do something along the lines of

ic['A'] & r1 & net 

However, I accidentally did a double assignment while making additions to code written earlier

ic[...multiple pins... 'A'] += ...multiple nets..., net
...
... quite a few lines of some other stuff
...
ic['A'] & r1 & net 

The end result was naturally that r1 was connected to net on both pins. This didn't raise any ERC warnings. OK, not unreasonable, eeschema doesn't raise any warnings from such either, but on the other hand this kind of mistake is much more obvious when viewed visually on a schematic. In fact, I noticed it from netlistsvg output.. heh.

Describe the solution you'd like I experimented a bit with adding a check to dflt_part_erc

    # Check that the total net count is not less than two
    connected_pins = [pin for pin in part.pins if pin.do_erc and pin.net and pin.func != Pin.types.NOCONNECT]
    if len(connected_pins) >= 2:
        connected_nets = {pin.net for pin in connected_pins}
        if len(connected_nets) < 2:
            erc_logger.warning(
                "Part has {p} pins active but has only {n} nets".format(
                    p=len(connected_pins), n=len(connected_nets)
                )
            )

However this broke 10 other ERC related unit tests (quite a few tests that test other areas of the ERC by just using a single mock resistor connected to itself). Not a big deal to fix those, but I'd like a second opinion on whether this is a good idea in the first place. Any foreseeable drawbacks?

voneiden avatar Sep 16 '20 22:09 voneiden

This seems like an oddly-specific check, but I can't see a reason not to add it except for the increase in ERC processing time. Please make a PR and change the unit tests as needed. We can always remove it if it blows up in our face.

xesscorp avatar Sep 17 '20 11:09 xesscorp

Yep. I'll think about it a bit more. Personally I'd like a paranoid ERC, but considering the performance implications maybe such oddly-specific checks should be opt-in, or at least opt-out.

voneiden avatar Sep 17 '20 15:09 voneiden

I think a paranoid ERC is acceptable. The increased processing time pales in respect to the board respin time.

One problem, however, is this won't detect mistakes for multi-pin packages. Say you had a four-resistor array and made these connections:

ic['A'] & r[1,8] & net  # Connect resistor in multi-resistor array.
...
ic[...multiple pins... 'A'] += ...multiple nets..., net

This would short the resistor between pins 1 & 8 of the array package, but it wouldn't be detected because the other resistors in the array would prevent the triggering conditions in the ERC. The same thing could happen with three-pin transistors, etc. I'm not sure how to write a general test that detects these kinds of errors without being too noisy with false positives.

xesscorp avatar Sep 17 '20 23:09 xesscorp