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

[FEATURE] Add preflight that fails when zones fills are out of date #431

Closed
matthijskooijman opened this issue May 10, 2023 · 2 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@matthijskooijman
Copy link

Is your feature request related to a problem? Please describe.
When committing changes, people sometimes forgot to do zone fills. Especially in a release this can be problematic, since then reviews might have been done on an old version of the zone fills and might miss problems the only surface when the zones are refilled, or the zone fill might be forgotten before producing fabrication outputs.

Describe the solution you'd like
Adding a preflight that does a zone fill and then fails if anything changed. This way I can add a push workflow that tests all commits to see if they were properly filled.

Note that there is already a check_zone_fills option, but that does not actually check anything, it just updates the zonefills on the in-memory pcb before doing any further processing (and also fill_zones which does the same, but also saves the result to disk). Neither of these solve my request.

Describe alternatives you've considered
Pre-commit hooks could also be used, but are opt-in on the developer's machine, so are less secure.

@matthijskooijman matthijskooijman added the enhancement New feature or request label May 10, 2023
@set-soft
Copy link
Member

Hi @matthijskooijman !

The name check_zone_fills is misleading, but I think it was inherited from the old kiplot tool. I have a tendency to favor compatibility instead of intuitive names.

I agree that a real check is a good idea, but I think you can solve it with the current functionality. Take a look at the following test case: tests/yaml_samples/diff_zones_1.kibot.yaml

kibot:
  version: 1

preflight:
  check_zone_fills: true

outputs:
  - name: 'diff_pcb_show'
    comment: "Show differences for the zone fill"
    type: diff
    layers: copper
    options:
      cache_dir: .cache
      old: ''
      old_type: file
      new_type: current

  - name: 'diff_pcb_check'
    comment: "Check the zone fill doesn't generate important changes"
    type: diff
    layers: copper
    options:
      cache_dir: .cache
      diff_mode: stats
      threshold: 100
      old: ''
      old_type: file
      new_type: current

This is a diff between the current file on disc and the current PCB on memory, abusing the default values ;-)
We use two outputs, one to generate a nice visual diff, and the other to measure the difference. It generates an error if the difference is above the threshold. In a workflow always saving the artifacts (even on fail) you'll get a nice PDF showing why it failed.

One think I could do is to just add internal template for this, so you just need a couple of lines in the config.

set-soft added a commit that referenced this issue May 18, 2023
- Enables the check_zone_fill preflight
- Creates graphical diff
- Computes the number of changes

Closes #431
@set-soft
Copy link
Member

Ok, I added a new internal template, so the following config will do the work:

kibot:
  version: 1

import:
  - file: CheckZoneFill

We just need to fine-tune the threshold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants