KiCad - The quality beneath KiCad’s surface

Hi all, once again we are back to deliver you a blog post of our favorite electronic design automation (EDA) program, KiCad! In an EDA program, it is possible to design schematics and printed circuit boards and export files to have them fabricated. This is already the third blog post of the series. Previous blog posts gave a high-level description of KiCad high-level description of KiCad, blog post 1, and analyzed its architecture, blog post 2. In this blog post, we will analyze the quality of KiCad’s source code and look at different methods used to ensure its quality.

The need for new code often starts with a bug in the current code or the need for improved functionalities. Both need to be made known before they can be tackled, this is done by creating an issue in GitLab. Thereafter, improvements in the code can be added, through commits, to a merge request (hereafter MR). These can then be merged into the master branch. By having a clear, standard structure for the issues and commits, you ensure that the problems and improvements are clearly communicated and therefore the content can be worked on in a more qualitative and targeted manner. To achieve this, KiCad has clear guidelines for issues, commits, and code style.

After creating a MR and adding commits, the code is at least checked by a main developer. Due to a good culture in the KiCad community, many other developers help out with checking new code and giving tips. More on the quality control by the developer community will be provided later.

Each MR also goes through a pipeline in which style checking and unit testing are performed, details of the pipeline are discussed later on.

Code Hotspots

Hotspots are parts of the code that are most often modified. These parts are often indicative of the most important activity in the project. We have used git to find the most regularly edited files and selected those that contain actual code. We found any files that have since been renamed and filtered out files that were deleted. From these hotspots, it is apparent that Pcbnew is the part of KiCad that sees the most development, followed by Eeschema. If you would like to learn more about the functionalities of these programs, have a look at our second blog post.

We have picked the 10 most edited hotspots and inspected the code on its quality. The variable naming is very clear most of the time, descriptive names are used while not being too verbose to make the code hard to read. Comments are, in our opinion, used appropriately. Most comments explain the reason why the code is written in a certain way rather than explaining what it does, which should be easy to read from the code itself. What did stand out to us is that most of these files are very long, approximately 2000 lines each. Many of them also contain very large functions, often several hundreds of lines. The code might be more maintainable if these files were split into smaller logical units, although there are already very many files in the project so increasing this, even more, might make code hard to find. Another fairly high metric is the maximum indentation, reaching around 10 in many files. This is partial because KiCad’s code style prefers the use of inline lambda functions over many separate helper functions. All in all, we think the code quality in the hotspots is quite high for a very complex real-world project.

KiCad’s culture

The world knows a large variety of cultures, this can be seen at various levels, demographically, nationally, locally, and even within a group of friends this is the case. Over the years the KiCad community has developed its own culture, this can be seen on various communication channels of KiCad. In this section, we will explore some discussions of previous merge requests.

Let us commence with a discussion between two main developers¹. To get a better overview of who these developers are, we suggest reading our first blog post. This discussion was about a functionality aspect of a new tool that is to be implemented. The issue is whether the current implementation of the tool is consistent with ‘similar’ tools. In this MR two developers ask a third main developer to give his opinion on the matter. All three have been working on the project for an extensive period of time and probably know each other very well. They could easily call each other to discuss the matter, however, they do not do so to keep open communication to the KiCad community. This also ensures that community members can give their opinion, however, it can be seen that the beliefs of the main developers weigh more heavily.

This MR is merely an example, as each MR will always be reviewed by one of the main developers. This is done to assure a certain quality level of the code and the program.

The second MR² which we will explore is about the Altium Board importer. It allows users to import PCB designs from the Altium IDE, which is a competitor of KiCad. First, the creator of the MR is helped with debugging by other developers. After a while, a main developer joins to help with debugging and, as is the case in more MRs, a specific member of the main developers checks the code styling. After a successful review by one of the main developers, the code is automatically merged after it finishes the pipeline.

Besides discussions on GitLab, there are also discussions being held via the developer mailing, the forum, and Internet Relay Chat. The most important communication channel for quality aspects is the developer mailing. It is impossible for each developer to read and follow all existing issues and MRs. Therefore, when important adjustments are made, which for example could affect the architecture, the developers are additionally involved via a developers mailing³. Often they are not simple announcements but invitations to think along whether it is a proper, clever, way to improve or extend the code.

To summarize this section, the aforementioned examples show the more or less standard interactions between developers. Also, it shows the openness, helpfulness, and quality checking of the KiCad community.

Dust underneath the closet

You probably have walked once past a house which stunned you, however, you were only able to observe the house from the outside and have no clue or whatsoever what the house is like on the inside. The same goes for applications, it can be very user friendly or good looking, but when taking a look under the hood it is a complete mess. This phenomenon is called technical debt, for instance in the house a chair can be broken or a lot of dust and junk is laying under a closet, these two types of debt differ. The broken chair is known by the owner, but he has no time to fix it at the moment. This type of technical debt is highlighted in the code by ‘//TODO’, only fifty six of these instances exist within the entire source code of KiCad. From this one can assume that a lot of technical debt is either unknown by the developers or just not marked as such.

The mess that is laying under the closet, however, is unknown and is a form of technical debt which is hard to find out about. Though it is present in the system and also affects the development of the system.

We ran SonarQube on the KiCad source files of which the result can be seen in the figure above, based on Python language present in the source files. Unfortunately the free version of SonarQube does not support the C and C++ languages, which is the language KiCad is written in. The figure shows that the technical debt present in the Python source files is already 4 days, when keeping in mind that the percentage of all code written in Python is only 0.5%⁴ this is not very promising for the technical debt present in the rest of the code.

At least the wheels are checked

A way to make sure the application works as intended by the programmer, intended ≠ written, is to periodically run tests. This can be compared to testing every device coming from a manufacturing plant, which is called acceptance testing, before shipping to a customer. Instead of testing the product which is shipped to the customer, the code is tested after every change. KiCad developers use a GitLab pipeline that builds the application and runs some jobs. The build can take more than a few hours, therefore is the advised timeout 6 hours. This pipeline consists of 12 jobs: 1 building job, 8 running test code jobs, one c(pp) style checker and 2 reporting tests. This could indicate that the KiCad software is tested well and checked extensively. This is sadly not the case, because in total there are around 200 tests for a code base of ~2 million lines of code and the overall line coverage is 7.7%, with a Pcbnew coverage of 5.1% and Eeschema at 10%, where a minimal code coverage normally is around 70-80%. Looking into the tests shows that mostly math functions, separate parts and plugins, as well as file parsers are tested and covered. This 7.7% coverage is like testing a car and only checking whether it has 4 tires, however, this is not enough to say anything meaningful about the whole system.

This low code coverage is unfortunate, but also understandable, because KiCad started out as a hobby project and evolved into this big application. When developers are not interested in testing and writing code that is testable from the start, it is hard to add testing afterwards.

A good starting point for adding tests is looking at the aforementioned hotspots and adding tests for that code. These hotspots are changed often and adding tests for them has therefore more benefit over code that is never changed.

The style checker job is another part of the pipeline which too is not according to the books, because it is used as an indication on the last commit(s). Style checking is like checking that the correct color wires are used in the car: code can be perfectly functional without following design standards, but it can be confusing for anyone working on it.

The Clang-format linter checks the changes of a merge request. It is, however, allowed to report errors, because they have a separate styling guide that they could not convert to clang-format rules.⁷⁸ Changing their styling rules to the default (LLVM) style would require a lot of changes of code. Some of which can be done automatically, like indentation or reordering.However, some have to be done by hand and even if that is 5% of all style violations, it is a lot of work, because currently it has more than 200k styling violations.

To summarise the testing and pipeline: it exists, but can be better, however this takes a lot of effort to improve and currently the developers do not see the real added value of that.

Conclusion

Kicad provides detailed guidelines for issues, commits, MRs and code styling. This allows developers all over the world to clearly communicate problems and solutions. KiCad has a CI pipeline which provides style checking and program testing. As mentioned, there is room to improve the tests and we recommend starting with more tests for code located at the hotspots, such as Pcbnew functionalities. Nevertheless, there is a good culture of quality among the Kicad developers. From various issues, MRs and mailings, it seems that there is an open, helpful culture in which various members take extra care of quality.