Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reword the courtyard complexity guideline #13

Closed
wants to merge 1 commit into from

Conversation

bsilver8192
Copy link
Contributor

I spent a while puzzling over whether a shape met the non-intersecting
requirement before realizing it's equivalent to being convex. I propose
including both ways of saying it, in case the non-intersecting version is more
intuitive to others.

I spent a while puzzling over whether a shape met the non-intersecting
requirement before realizing it's equivalent to being convex. I propose
including both ways of saying it, in case the non-intersecting version is more
intuitive to others.
@fruchti
Copy link
Collaborator

fruchti commented Dec 5, 2020

This isn’t necessarily true, as non-convex shapes can be fine, too. Take the pool’s TQFP176 package outline, which is non-convex but doesn’t start intersecting itself at any expansion (which I like to think of as dragging a compass around the edge to add a constant outwards offset):
2020-12-05-234409_1600x882_scrot

You can, however, create some pathological shapes which lead to problems, like:

get-parameter [ courtyard_expansion ]
expand-polygon [ courtyard -2000000 0 -2000000 1500000 1500000 1500000 1500000 0 0 0 0 -1500000 3000000 -1500000 3000000 3000000 -3500000 3000000 -3500000 -5500000 3000000 -5500000 3000000 -3000000 -1000000 -3000000 -1000000 0 ]

2020-12-05-235554_1600x882_scrot

This produces an ‘expand error’ with courtyard expansions >= 0.5 mm.

Obviously this is a very contrived example, so it’s up for discussion whether or not to keep this rule in the convention at all, as it’s very rarely relevant. Perhaps it could be kept for cases with multiple polygons, e.g. adding multiple connectors for one module as individual courtyard polygons, where it’d be a lot easier to run into issues?

@bsilver8192
Copy link
Contributor Author

Ah, I didn't realize that expand-polygon handles that first case. That's handy.

Is there a simple way to describe the shapes that expand-polygon doesn't handle? Maybe "shapes with vertices normal to a segment (excluding its endpoints) and on the outside of that segment"? That probably belongs in the expand-polygon documentation, not horizon-pool-convention though.

@bsilver8192 bsilver8192 closed this Dec 6, 2020
@bsilver8192 bsilver8192 deleted the patch-1 branch December 6, 2020 05:07
@fruchti
Copy link
Collaborator

fruchti commented Dec 6, 2020

shapes with vertices normal to a segment (excluding its endpoints) and on the outside of that segment

If I read that right, a rectangle would fall under this definition, too. I can’t make up a mathematical way to write this down. You could be tempted to say not star-shaped domains are a problems, but some of those are handled fine, too. In fact, the shape above works if you shrink the inner part near the origin:
2020-12-06-125034_1600x882_scrot

get-parameter [ courtyard_expansion ]
expand-polygon [ courtyard 3000000 -3000000 -1000000 -3000000 -1000000 0 -2000000 0 -2000000 1000000 1500000 1000000 1500000 0 0 0 0 -1500000 3000000 -1500000 3000000 3000000 -3500000 3000000 -3500000 -5500000 3000000 -5500000 ]

Expanding this never leaves a polygon with a hole, which Horizon could not handle with the previous example.

Personally, I think the current rule is clear enough, even if it is not completely precise. Basically, it is a reminder to check for problems here, which might arise for too complex shapes. Of course, my view is biased here as I wrote this one, so I’m not opposed to change it to something clearer for other users.

@bsilver8192
Copy link
Contributor Author

I was still underestimating the power of expand-polygon... Is this a good description of what expand-polygon can handle then?

Any shape and expansion which don't create non-empty interior holes.

@carrotIndustries
Copy link
Member

I was still underestimating the power of expand-polygon... Is this a good description of what expand-polygon can handle then?

expand-polygon uses clipper for polygon operations: https://github.com/horizon-eda/horizon/blob/master/src/parameter/program_polygon.cpp#L126 The only limitation from the application's point of view is that the operation results in a single polygon.

@fruchti
Copy link
Collaborator

fruchti commented Dec 6, 2020

We could explicitly state that there must be exactly one courtyard polygon, because multiple polygons will always start intersecting at some point. I haven’t checked but I can imagine the DRC complains when encountering overlapping courtyard polygons within one package.

With that in place, cases like my pathological one above won’t really occur in practice and we can delete the courtyard complexity rule without substitution, I think.

@atoav
Copy link

atoav commented Dec 7, 2020

We could explicitly state that there must be exactly one courtyard polygon, because multiple polygons will always start intersecting at some point.

Wouldn't the pragmatic solution be to just unify those polygons then? What is the precise purpose of the courtyard? For Pick and Place? Or for you as a board designer to act as a guide where to be more careful when placing things?

I can't think of any part that consists of two "islands" but an overlapping/self-intersecting expanded path could be treated as a boolean union. This is what Adobe Illustrator's "Offset Path"-Function does:
courtyard

This would honor the robustness principle: Be conservative in what you do, be liberal in what you accept from others. Whether this needs extra code and whether that time spent is worth it is a different issue I think.

@fruchti
Copy link
Collaborator

fruchti commented Dec 7, 2020

What is the precise purpose of the courtyard?

AFAIK it’s an aid to the designer to ensure assembly tolerances will not lead to problems. I’m happy to be corrected if that’s not the case, though.

I can't think of any part that consists of two "islands" but an overlapping/self-intersecting expanded path could be treated as a boolean union. This is what Adobe Illustrator's "Offset Path"-Function does:

This looks like what clipper/Horizon does. There only is a problem if the expansion results in a ‘hole’ in the polygon, which would, in your example, happen if the ‘corridor’ (the notch still present in the second picture) was narrower. Practically, this will be extremely rare, I think.

Two courtyard polygons would be a logical way for some kind of PCB-mounted module with an additional connection somewhere else. With the connecting wire raised from the PCB, other components could be placed between the module and its extra connection. This won’t be a frequent case either, though. Whether something like this should be a single package at all is another question, too.

bsilver8192 added a commit to bsilver8192/horizon-docs that referenced this pull request Dec 10, 2020
As discussed in horizon-eda/horizon-pool-convention#13, it's complicated and I made some incorrect assumptions after reading this. I think making it clear here will reach anybody who goes looking.

The question of how much detail to specify in horizon-pool-convention seems separate to me, because the docs for the parameter program commands should be clear on their own.
@bsilver8192
Copy link
Contributor Author

I think leaving it at "must be valid" is fine if the docs for expand-polygon are clear about what shapes produce valid expansions. I tried doing that in horizon-eda/horizon-docs#19. I think removing the courtyard complexity requirement completely would be fine too, given how hard it is to violate it.

@fruchti
Copy link
Collaborator

fruchti commented Dec 13, 2020

I’ll remove the rule completely for now, as it seems to be largely unnecessary. If we start noticing problematic PRs, we can always put it back in.

This pull request was closed.
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.

4 participants