feat(stats): collect drill stats

[KN4CK3R]

A WIP prototype for #372. Let me know if this goes into the right direction.

Usage:

var fs = require('fs')
var gerberParser = require('gerber-parser')
var statter = require('..')

var parser = gerberParser()
var stat = statter();

fs.createReadStream('fixtures/boards/arduino-uno/arduino-uno.drd')
  .pipe(parser)
  .pipe(stat)
  .on('data', (result) => console.log(result));

Output:

{
  totalDrills: 169,
  tools: {
    '1': { shape: 'circle', params: [Array], hole: [] },
    '2': { shape: 'circle', params: [Array], hole: [] },
    '3': { shape: 'circle', params: [Array], hole: [] },
    '4': { shape: 'circle', params: [Array], hole: [] },
    '5': { shape: 'circle', params: [Array], hole: [] },
    '6': { shape: 'circle', params: [Array], hole: [] }
  },
  drillsPerTool: { '1': 72, '2': 62, '3': 20, '4': 9, '5': 2, '6': 4 },
  minDrillSize: 0.024,
  maxDrillSize: 0.126
}

closes #372

[kasbah]

Looks like the right direction to me. Would be good to get @mcous' view. We definitely need some tests added and the stats added to the output of pcb-stackup.

[mcous]

Agree with @kasbah's feedback! Sort of in a weird spot with this, though, where I don't intend to maintain the stream-based interface of gerber-parser, but also have no timeline at all for the new stuff 😅.

Take a look at the v5 branch and the published pre-release version of @tracespace/parser and let me know if you'd rather build stats collection on top of that. Benefits:

• Typescript
• Synchronous inspection of the full syntax teee

If "yes", then we can figure out how to switch the main dev branch over to v5 and decide on a publishing strategy.

If "no, I'd prefer to work on the stuff that is currently in production," that's cool, too, and this PR seems like a good start on that

[kasbah]

I am strongly in favor of doing this work on the v5 branch. Apologies that I wasn't fully aware of the state of it, otherwise I would have mentioned it in the issue description.

[KN4CK3R]

I have moved the code to the v5 branch. I'm not familiar with Typescript and currently I get the message:

Error: 'ToolDefinition' is not exported by packages/parser/esm/index.mjs, imported by packages/stats/src/collector.ts
But why? ToolDefinition is marked as export. Could you help me with that? If it is really not usable, do I have to check the type field?

[mcous]

Very cool! I'm going to be hard to reach for the next couple weeks, but in the meantime, I think this should unblock your TS error

[MrHBS]

@KN4CK3R Are you still working on this?

[KN4CK3R]

Yes, I was busy but will push the current version soon.

[KN4CK3R]

Suggestions on what to change or add?

[mcous]

I appreciate your patience (and staying on top of upstream changes) as I attempt to get some momentum going on v5 again.

Here is an initial batch of feedback! Additionally, we will need unit tests in the branch before I'm willing to merge it.

I'm pretty close to putting up a PR that includes a rewrite of the plotter as well as a handy page showing how various simple gerber and drill files get parsed and plotted, which I think could help you interpret the parse trees a little better. I'll post an update in this thread once that's up over the next day or two

[KN4CK3R]

I have updated the code and added your suggestions. Please have a look.

[mcous]

Looks like CI is failing. Please ensure you give the contributing guide a read through, especially the section on how to run unit tests, type checking, and linting. You can check everything at once locally with pnpm all.

I left some more comments below. Other than those comments, these are the outstanding issues with this PR before we can move it forward:

• PR does not include unit tests
• New package does not include README
• Code linting is not passing
• Type checks are not passing
• package.json does not have build configured - missing scripts.clean and scripts.build
• I think this logic is small enough that everything could live in index.ts for now; while the LoC count is small, a single file is easier to read

[codecov]

Codecov Report

Merging #373 (53cf603) into v5 (b7a8779) will decrease coverage by 23.17%.
The diff coverage is 95.00%.

❗ Current head 53cf603 differs from pull request most recent head a72a604. Consider uploading reports for the commit a72a604 to get more accurate results

@@             Coverage Diff             @@
##               v5     #373       +/-   ##
===========================================
- Coverage   93.53%   70.35%   -23.18%     
===========================================
  Files          36       43        +7     
  Lines        4348     4676      +328     
  Branches      604      478      -126     
===========================================
- Hits         4067     3290      -777     
- Misses        281     1386     +1105     

Impacted Files	Coverage Δ
packages/stats/src/index.ts	95.00% <95.00%> (ø)
packages/xml-id/src/index.ts	47.22% <0.00%> (-30.56%)	⬇️
packages/parser/src/syntax/map-tokens.ts	97.05% <0.00%> (-2.95%)	⬇️
packages/parser/src/syntax/rules.ts	98.11% <0.00%> (-1.26%)	⬇️
packages/parser/src/syntax/macro.ts	99.50% <0.00%> (-0.50%)	⬇️
packages/parser/src/syntax/drill.ts	99.59% <0.00%> (-0.41%)	⬇️
packages/plotter/src/tree.ts	100.00% <0.00%> (ø)
packages/parser/src/constants.ts	100.00% <0.00%> (ø)
packages/plotter/src/graphic-plotter/plot-shape.ts
packages/plotter/src/graphic-plotter/plot-path.ts
... and 30 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7a8779...a72a604. Read the comment docs.

[mcous]

Getting there! Take a look at the automated codecov comments on this PR: they point to branches that the unit tests don't currently cover. It looks to me like we're missing test coverage for the following logic:

• Slots get counted as routes
• Tool definitions set the current tool
• Non-drill files get skipped
• Multiple drill files have their stats combined
• Non-circular tools do not count as drill hits
• Missing current tool means graphics aren't counted as drill hits
• Defined but unused tools do not count towards stats

I also pushed up some changes to fix some formatting issues that got committed. Please ensure your editor and git is configured to:

• Use 2 spaces for tabs
• Commit with LF line-endings

[mcous]

Functions that mutate an input parameter to do their main work are usually a bad idea. I would solve this problem a different way, perhaps with a function that takes a single tree and returns the stats and another function that combines the stats

[KN4CK3R]

I thought about that too but the merging bloats the whole thing for not much value:

function _mergeStates(
  s1: DrillStatsState,
  s2: DrillStatsState
): DrillStatsState {
  return {
    usedTools: {...s1.usedTools, ...s2.usedTools},
    drillsPerTool: _mergeAdd(s1.drillsPerTool, s2.drillsPerTool),
    routesPerTool: _mergeAdd(s1.routesPerTool, s2.routesPerTool),
    totalDrills: s1.totalDrills + s2.totalDrills,
    totalRoutes: s2.totalRoutes + s2.totalRoutes,
    minDrillSize: _cmp(s1.minDrillSize, s2.minDrillSize, Math.min),
    maxDrillSize: _cmp(s1.maxDrillSize, s2.maxDrillSize, Math.max),
  }

  function _mergeAdd(
    r1: Record<string, number>,
    r2: Record<string, number>
  ): Record<string, number> {
    const rec = {...r1}
    for (const [key, count] of Object.entries(r2)) {
      const oldCount = rec[key] ?? 0
      rec[key] = oldCount + count
    }

    return rec
  }

  function _cmp(
    n1: number | null,
    n2: number | null,
    cmp: (...values: number[]) => number
  ): number | null {
    if (n1 === null && n2 === null) {
      return null
    }

    if (n1 === null) {
      return n2
    }

    if (n2 === null) {
      return n1
    }

    return cmp(n1, n2)
  }
}

[mcous]

I agree that DrillStatsState doesn't lend itself to merging, but it's also a completely internal interface. If DrillStatsState's shape is such that it leads to doing things the way _updateDrillStats is currently written, I think DrillStatsState should be changed

[mcous]

I agree that DrillStatsState doesn't lend itself to merging, but it's also a completely internal interface. If DrillStatsState's shape is such that it leads to doing things the way _updateDrillStats is currently written, I think DrillStatsState should be changed