kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

Hi Antonio,

On 25/04/2019 10:39, Antonio Vázquez Blanco wrote:
> I've been playing around a little bit with KiCad source code lately and 
> in the forums [1] I was encouraged to write to the dev mailing list in 
> order to get feedback on how to improve the current library error checking.

This looks very interesting. However, I do have some concerns about 
embedding the KLC checkers in KiCad directly. The KLC is maintained on a 
totally different basis and schedule to the main KiCad code. Once this 
kind of code is in KiCad and released at a version, it is "stuck" until 
the next release, which could be much later than KLC changes are published.

Of course, I do understand that these checks are already embedded in 
KiCad's code, but these probably pre-date the KLC by some years!

> Summarizing the thread, I made some changes [2] so that a couple of KLC 
> checks are performed on syms but now that I've done that I would like to 
> implement this properly. I am looking for pointers on how should I 
> implement this so that the chances of me getting the code upstream 
> maximize and so that I can reuse the ERC/DRC code as much as possible.

What you have done here is a nice modularisation of the code, but I'd 
suggest making it even more generic. For example:

* Rather than KLC_RULE_G12 (max symbols per lib), have a 
MAX_SYMBOLS_PER_LIB checker with a parametrised maximum
* Rather than KLC_RULE_S41 (general pin requirements), have parametrised 
SYMBOL_PIN_ON_GRID, SYMBOL_PIN_LENGTH, etc. checkers.

As a first draft, the existing functions can be refactored to generic 
checkers and fed the hard-coded constants as they currently are. In the 
longer term, these parameters (e.g. the minimum pin length) could be 
loaded from a config file at run-time and given UI to set them.

For the more "esoteric" rules (e.g. the naming rules which are complex, 
have many special cases and are likely to "churn"), I think a way to run 
external scripts on footprint or symbol, something like the Action 
Plugin system for PCBs, would be better.

This will allow the KLC checkers to be distributed separately to the 
main KiCad codebase. Python checking scripts already exist in the 
kicad-library-utils repo[1], so maybe the script can just call that.

Part of v6 is an overhaul of the DRC system to make it more modular. I 
hope that will also include the ability to have pluggable external E/DRC 
providers.

More technical things about this code in particular:

* `KLC_RULE* rules[] = {...` This leaks all the new'd members. Use a 
vector and smart pointers. Avoid `new` wherever you can.

  std::vector<std::unique_ptr<KLC_RULE>> rules = { ...

* You don't need to use `this->`

* Please document your class and function declarations with Doxygen 
comments.

* Assorted code formatting (e.g. wrong: `if (foo)` vs right: `if( foo 
)`). See the guidelines and includes ways to automate that [2].

* `Verify(LIB_PART* part)` if you're not going to check a pointer before 
dereference, pass a reference instead. Also, try to make it const where 
you can.

* Member variables start with `m_`.

There are other issues in the code, but they already existed, so I don't 
mind this refactor not fixing them.

Ideally, these kind of checkers would get some unit tests, but the 
eeschema unit tests don't work (yet). However, the checkers should be 
written in such a way that a unit test *could* be easily written for 
them. For example, a nice follow-up would be returning error objects 
rather than HTML strings.

Because this code is a re-factor of existing code, I think that, once 
the formatting and generic-ness is dealt with, this can be merged. But I 
do not think going down a road of hard-coding all the KLC checks into 
the KiCad core is a sustainable path.

Cheers,

John

> [1]: https://forum.kicad.info/t/improving-library-editor-checks/16557/5
> [2]: http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html