← Back to team overview

kicad-developers team mailing list archive

Re: Improving library editor checks

 


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


References