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] KiRi integration #391

Closed
FrankvVeelen opened this issue Feb 12, 2023 · 87 comments
Closed

[FEATURE] KiRi integration #391

FrankvVeelen opened this issue Feb 12, 2023 · 87 comments
Assignees
Labels
enhancement New feature or request

Comments

@FrankvVeelen
Copy link

As an alternative to the kidiff tool (which works okay, but doesn't work for analyzing differences between more than a few commits/versions, I think it'd be cool if kibot could integrate Kiri, as it (at least for me) works a bit nicer with tools like jenkins (generating a html page on which i can then analyse differences between commits).

If you need help with implementing this and could give me a few pointers where i should start please let me know.

@FrankvVeelen FrankvVeelen added the enhancement New feature or request label Feb 12, 2023
@set-soft
Copy link
Member

When I took a look at Kiri it used a mechanism for schematics that isn't good for KiBot integration.
I know Kiri is much better for interactive usage, but not for CI/CD.
I'll keep the request, but I'm not sure about it.

@Nudelsalad
Copy link

Would be an awesome feature! Does Kiri even interfere with kibot if it is triggered separately? With the --no-server flag given the output could be maybe nicely relinked with navigate_results maybe?;) There are workarounds to get this working but up to your answer I guess there is nothing in the pot in the near future?

@set-soft
Copy link
Member

Hi @Nudelsalad !

I see @leoheck moved to kicad-cli for KiCad 7. Which removes the need for PlotKiCadSch, a real overkill since this is an OCaml tool.

Still using KiCad-Diff for the PCB, which is redundant tool since KiBot uses KiDiff.

Things are getting closer, but I don't have time right now.

@leoheck
Copy link

leoheck commented Nov 14, 2023

Hi guys, KiCad-Diff is a small script, and for Kiri, you don't even need the GUI, I don't see any big problem of having it installed.

Kiri still uses KiCad-Diff because kicad-cli for layouts takes a looong time to export each layer, and considering a big set of layers per commit and also a big set of commits, it takes a lot of time.

But it is also possible to use kicad-cli for this. If KiCad-Diff is not installed (or maybe you have to pass a flag to Kiri), it will use kicad-cli instead. It may work ok if comparing 2 commits.

For instance, here, we just compare 2 commits, removing previous runs, and displaying the commands kiri is running, without launching the browser.

kiri -r -D --kicad-cli -t 2 -l 

And this already takes 12.2s on my machine which is a good machine I guess (Dell XPS, i9)
The Board used in this example has 22 layers. Not all of them are important but they are/were present in the Kicad project.

@set-soft
Copy link
Member

Hi @leoheck !

I'll take a look at this. I'll post my questions here. First one: why dos2unix is a mandatory dependency?

@leoheck
Copy link

leoheck commented Nov 24, 2023

Nope, I just ended up adding it as a workaround since most of the time people do things that they don't know, and they blame my tool for that and I don't always have time to debug everything. You could remove the lines using it. I can end up removing or adding a test to not use it if it is missing.

@set-soft
Copy link
Member

Would be an awesome feature! Does Kiri even interfere with kibot if it is triggered separately? With the --no-server flag given the output could be maybe nicely relinked with navigate_results maybe?;) There are workarounds to get this working but up to your answer I guess there is nothing in the pot in the near future?

About the link to navigate_results, you mention "workarounds", which ones do you mean?

From what I see you need to start the KiRi webserver and read the pages from it. As a workaround I tried google-chrome --allow-file-access-from-files index.html and got it working, but is quite specific to a particular case.

@leoheck
Copy link

leoheck commented Nov 27, 2023

I am not sure how KiBot works or how it is used, but Kiri could just generate the images and the website without having to launch it... then the result (the .kriri folder,) can be exported, there is also an argument to make a pack of it. Inside, this folder it has a python script to launch the webserver using the previously generated files.

@set-soft
Copy link
Member

@leoheck : I was playing with KiRi and found a couple of issues:

  1. When the files are in KiCad 5 format it tries to use plotkicadsch, even when it used kicad-cli for the other commits (forced by --kicad-cli). You can see it using the following repo
  2. In the above case when plotkicadsch fails (not installed) it tries to use xdootool. But if the process fails the keyboard is left changed to us. I suspect one of the problems is that KiRi thinks this is KiCad 5, but started KiCad 7.

It would be nice if --kicad-cli forces the use of kicad-cli even for KiCad 5 files.

Other questions:

  • I see you use shell scripts and Python, why not just Python? Is much more powerful and is even more portable, you won't need to worry for things like readlink, sed, realpath, etc. And your shell scripts are Bash specific!
  • By using something like Python you'll be able to drastically reduce the "bin" pollution, you have 19 things in bin, KiRi can't be installed in a regular system, in particular: you even have an env.sh script, that could collide with anything.

@set-soft
Copy link
Member

I am not sure how KiBot works or how it is used, but Kiri could just generate the images and the website without having to launch it... then the result (the .kriri folder,) can be exported, there is also an argument to make a pack of it. Inside, this folder it has a python script to launch the webserver using the previously generated files.

One of the KiBot outputs can generate web pages to browse all the generated stuff. The problem here is that we can't directly link the KiRi output from this page.

From what I understand the big problem is that KiRi needs to read contents from separated files and this seems to be forbidden for Java Script. Embedding all the SVG files seems to be an overkill and this is why a server is needed.

@leoheck
Copy link

leoheck commented Nov 27, 2023

It would be nice if --kicad-cli forces the use of kicad-cli even for KiCad 5 files.

Yeah, definitely, I will give it a look.

I see you use shell scripts and Python, why not just Python? Is much more powerful and is even more portable, you won't need to worry for things like readlink, sed, realpath, etc. And your shell scripts are Bash specific!
By using something like Python you'll be able to drastically reduce the "bin" pollution, you have 19 things in bin, KiRi can't be installed in a regular system, in particular: you even have an env.sh script, that could collide with anything.

Yeah, python is awesome, but shell script is easier for me. Actually, this tool started as an experiment, but then later I adapted it to work on other systems and for others too. It just started as a bunch of commands that I put together in a script. Having everything made in Python would be better definitely it would also make it possible to add on Kicad package manager (people would definitely love this). I thought many times to start changing it to Python only, but it would take a lot of time and effort for me to do it. Maybe if I have some spare time in the future. It is not a big and complex tool. The tool is simple, actually, it gets the commit list from git and iterates through them exporting the schematics/layers to images.

@set-soft
Copy link
Member

Hi @leoheck !

I tried the following toy test:
t1.zip

Running the run_kiri.sh script I got a nice list of commits and can browse the schematic changes. But the PCB looks the same for all the commits. Looking at the logs I see things like:

# 4/6 1db11f8 | 2023-11-27 11:12:45 | KiBot test | Reference

    Retrieving commits:
    - Source path: /home/salvador/0Data/0Test/#391/t1/.
    - Output path: /home/salvador/0Data/0Test/#391/t1/.kiri/1db11f8
...
    # Plotting schematics 1db11f8
...
   # Plotting layout _local_, F.Cu

Always plotting local, not the corresponding commit. Can you reproduce this bug?

@leoheck
Copy link

leoheck commented Nov 27, 2023

Schematics are hard to spot the differences actually.

But I can see them here...

image
image
image

You can easy see the same with this script

schs=($(find .kiri/*/_KIRI_/sch -name "*.svg"))
for sch in ${schs[@]}; do
        # echo "Diffing ${schs[1]} and ${sch}"
        diff -q --report-identical-files ${schs[1]} ${sch}
done

Right now, when the local files have not commited changes, they are included automatically on the list of commits.

@set-soft
Copy link
Member

Hi @leoheck !

Schematics are ok, the problem is with the PCB.
The log says that is always plotting the layout for local, so you can't see any difference ;-)

@set-soft
Copy link
Member

I also found another problem: If I include the same sub-sheet twice KiRi generates:

light_control|light_control.kicad_sch||light_control|light_control
sub|sub.kicad_sch||sub2|light_control-sub2
sub|sub.kicad_sch||sub|light_control-sub

This is good, but when you look into the generated files only two of them are generated (light_control.svg and sub.svg). You may claim that sub and sub2 are the same, but this isn't correct, changes in the annotation (reference numbers) can't be displayed if you don't have both rendered.

Here is the test case: test.zip

@Nudelsalad
Copy link

About the link to navigate_results, you mention "workarounds", which ones do you mean?

From what I see you need to start the KiRi webserver and read the pages from it. As a workaround I tried google-chrome --allow-file-access-from-files index.html and got it working, but is quite specific to a particular case.

Hi @set-soft and @leoheck! Nice to see this discussion going on,
My workaround is not really a solution to these problems, but here is a little insight:
I installed Kiri directly into the kibot docker container and let them both run their jobs in gitlab-ci. There, the output jobs are pushed to gitlab pages, and with separate links in the readme file or in the index.html I generated, you can access both outputs. The kibot link points to Test-navigate.html in the kibot/web directory and the kiri link points to index.html also in the kiri/web directory.
As you might see this is a dirty solution and the container is quite big with all my libs and the full kibot image ~3.2GB
My scope was to get it up and running in a short time, not to dig very deep.

Unfortunately I am having some performance issues with kiri output on large projects. I am not sure why it is really lagging on a large schematic. Thinking it must be an issue on the gitlab side, I also spun up the local kiri server and removed all the auxiliary project files. This left me with no clue as I have both a powerful server and a powerful desktop machine. But only on the schematic side! On the other hand, looking at the PCB seems fine and smooth.
I'll mail you both the example :)

@leoheck
Copy link

leoheck commented Nov 27, 2023

Schematics are ok, the problem is with the PCB.

Ah, sorry. I am running a fresh install of the OS, I don't have Kiri installed the traditional way and I am not sure if it has everything that it needs.

Testing here, I saw no changes as you are saying. But after cleaning the cache, (flag -r) it started to show the changes. Could you try that too? The local changes should not need this, I will have to check this behavior too, thanks for finding this.

@leoheck
Copy link

leoheck commented Nov 27, 2023

This is good, but when you look into the generated files only two of them are generated (light_control.svg and sub.svg). You may claim that sub and sub2 are the same, but this isn't correct, changes in the annotation (reference numbers) can't be displayed if you don't have both rendered.

Yeah, handling multiple schematic pages is hard, especially the way Kicad stores these things. This behavior can be improved too. In the past, I thought that just one is enough since I was interested in circuit changes only. Perhaps there is a better way to display this.. maybe showing all the instances of each page... however only the designators are going to change, I feel no one will check these changes. I may be wrong tho.

@leoheck
Copy link

leoheck commented Nov 27, 2023

Unfortunately I am having some performance issues with kiri output on large projects.

Is this for a single comparison or for comparing all git history? You can run Kiri with -t 3 to compare only the last 3 changes, including the local if it has chances.

You could also try kiri-docker if you are using linux or a linux shell.

Check it here: https://github.com/leoheck/kiri-docker
There is this kiri command that is a wrapper to the normal kiri command.
It mount the local repo inside the docker, launch kiri there, and displays the result in the local browser.
This could work for you, maybe.

@set-soft
Copy link
Member

however only the designators are going to change, I feel no one will check these changes. I may be wrong tho.

If the changes are in the reference numbers and KiRi doesn't show them the user will be confused about what's the difference. In the future KiCad could also allow changing other things, like the component value.

@set-soft
Copy link
Member

Testing here, I saw no changes as you are saying. But after cleaning the cache, (flag -r) it started to show the changes. Could you try that too? The local changes should not need this, I will have to check this behavior too, thanks for finding this.

Tried it and got the same. Did you try using the run_kiri.sh script? I suspect the issue could be related to --kicad-cli

@set-soft
Copy link
Member

Unfortunately I am having some performance issues with kiri output on large projects. I am not sure why it is really lagging on a large schematic. Thinking it must be an issue on the gitlab side, I also spun up the local kiri server and removed all the auxiliary project files. This left me with no clue as I have both a powerful server and a powerful desktop machine. But only on the schematic side! On the other hand, looking at the PCB seems fine and smooth.
I'll mail you both the example :)

I got your example, do you mean that doing zoom and pan in the browser is really slow?

I noticed that in my machine at work (not really fast, 3rd gen i7). But if you look at the SVGs they are upto 3.6 MiB. The script is comparing 2 of them. Perhaps the method used is not the best (is this the feColorMatrix stuff @leoheck ), or just the pan & zoom class (svg-pan-zoom). But yes, is slow.

@leoheck
Copy link

leoheck commented Nov 28, 2023

Hi guys (@set-soft @Nudelsalad), I tried to summarize the issues here, leoheck/kiri#104
Do you mind checking if it is missing something or if I misunderstood anything?

======

@set-soft I tried your script once (and again now), the local layers are not being displayed for some reason (the image was just red), as you observed. However, they were generated. I also could not see the differences in the layers of other commits with the browser.

Found the issue, the layouts of the _local_ have different names. the names are not being normalized. They should be normalized to this format layer-00.svg

image

@leoheck
Copy link

leoheck commented Nov 28, 2023

image

@set-soft do you have this rename here? I don't have it right now, since I reinstalled the OS, this may the the issue on my side. I will check if I can skip it to use the normal rm tool... since it is more spread and maybe reliable. this causes issues on other distros since not always people have the rename that is the perl script since there are others available.

image

@leoheck
Copy link

leoheck commented Nov 28, 2023

for some reason, the local changes produce an image with a different kicad version. but there are no local changes made by the user.
image

However, the changes are not being displayed yet.

@leoheck
Copy link

leoheck commented Nov 28, 2023

@set-soft I could find an issue when generating pcb layers with kicad-cli it is fixed in the repository already, could you check if it works for you?

@leoheck
Copy link

leoheck commented Nov 28, 2023

image

kicad-cli can be used with kicad 5 layout but not with schematics since it plost only kicad_sch and not sch. This is a poor decision they made. Poor because they have the kicad parser and the kicad exported. They could have just used it.

But I will try to fix the issue with the keyboard changing when xdotool fails.

@set-soft
Copy link
Member

Hi @leoheck !
I applied a patch to the kiri.js to avoid reloading the list of pages every time we select a page. Also to avoid an event on-click plus another on-change. Take a look at a7ad89c

About the canvas color: It should be configurable, but I want to solve some issues first

@set-soft
Copy link
Member

Hi @leoheck !
The following patch solves a big issue with sub-sheets: e35fb66
Using it we can recycle the same sheet in the project, or use sub-sheets with the same name (but different path).
The drawback: the names of the generated files are different.
But is an easy fix: just don't rename the files generated by KiCad! They have the sheet hierarchy because this allows pages reuse.

@set-soft
Copy link
Member

This is another important patch: 0cad908
It avoids fetching a sheet page that isn't available in a commit, in this case we use the blank.svg image.

@leoheck
Copy link

leoheck commented Dec 11, 2023

Oh, you are making good progress here, thank you, I will test these last updates. I am going to test this later today since I have too many things todo today. Cool?

@Nudelsalad
Copy link

Very cool!
Is it then possible to ignore this layer in the sheet viewer? As only those with changes are interesting to review

@set-soft
Copy link
Member

Very cool! Is it then possible to ignore this layer in the sheet viewer? As only those with changes are interesting to review

Well I think the answer is no. You usually want to know if some layer/sheet was added/removed. If you see a sheet/page all in red, then it was removed, and if is all green it was added.

@leoheck
Copy link

leoheck commented Dec 11, 2023

Yeah, but if we can detect this somehow, we could make the keyboard shortcut to navigate through layers/pages to jump over then ones that didn't change. but they could be still there (with a different font color, maybe) if the user wants to click to check.

@set-soft
Copy link
Member

Yeah, but if we can detect this somehow, we could make the keyboard shortcut to navigate through layers/pages to jump over then ones that didn't change. but they could be still there (with a different font color, maybe) if the user wants to click to check.

The 901c76b implements the color part.

image
image

@Nudelsalad
Copy link

Just great @set-soft!
I would like to add another thought of mine.
I understand that it is quite hard to handle everything with svgs and that there is not much room for further expansion. Also, I do not want to make more feature RQs while you are in the process of integrating this into Kibot, this is just an idea:

Wouldn't it be cool if the zones were dimmed or completely hidden?
I do not know how you process your files, but I just looked up kicad-cli features for an export option of svgs without filled zones and this feature is not implemented yet - but would be essential I guess.

image

In my opinion, the presentation of the new changes is a bit confusing as zones can overlap tracks and vice versa.

As I am not fully involved in the development of your tools, is this possible? Is it worth creating a feature rq for kicad-cli export? Do we share the same opinion? Does it even depend on kicad-cli?

I also have in mind that this is the thread for the integration of kiri into kibot and nothing else, but having two experts working on the same codebase seemed to me a justifiable reason.

Thanks in advance!

P.S.: As an example, the cadlab.io guys seem to have managed this somehow, although it seems not to be a comparison between svg files, but more a wrapper for whole projects while on a net and change basis the changes seem to be displayed Link

@set-soft
Copy link
Member

Hi @leoheck !
The following patch is interesting, but a little bit disruptive: ad3f8bb
I moved the commits HTML generation to the JavaScript. The goal is to have an index.html that is the same for every project. In order to achieve this I'm creating a file called commits containing the commits. The format is documented in the JS, is:

HASH|DATE|AUTHOR|DESCRIPTION|SCH_CHANGED|PCB_CHANGED

And here is an example:

_local_|2023-12-12|Salvador E. Tropea|Local changes not committed|True|True
ab61573|2023-11-27|KiBot test|Cambio 2|True|True
3b3741c|2023-11-27|KiBot test|Cambio 1|True|True
1db11f8|2023-11-27|KiBot test|Reference|True|True

The only thing that remains to be moved is the sch/pcb info, this will be the next step. Doing this the index.html file will be independent of the project. This will also simplify your shell script a lot.

@set-soft
Copy link
Member

Hi @Nudelsalad !

Wouldn't it be cool if the zones were dimmed or completely hidden?

In order to keep this idea please open an issue in the KiDiff project. This is the underlying tool.

@set-soft
Copy link
Member

The only thing that remains to be moved is the sch/pcb info, this will be the next step. Doing this the index.html file will be independent of the project. This will also simplify your shell script a lot.

And now is done: d073b15
The index.html is always the same, the JS loads the data from the generated files and adjusts the HTML.

@set-soft
Copy link
Member

More for @leoheck : The following patch 6a380e3 moves all the style stuff from the HTML to the CSS.
The main objective is to make the HTML easier to maintain. It could potentially allow some degree of customization using the CSS.

@leoheck
Copy link

leoheck commented Dec 12, 2023

@set-soft what if you make a fork of kiri? You could make a fork of it, and then a PR to me even if it is work in progress. It would help me test and merge all your good stuff at once.

@set-soft
Copy link
Member

@set-soft what if you make a fork of kiri? You could make a fork of it, and then a PR to me even if it is work in progress. It would help me test and merge all your good stuff at once.

Is a good idea, I'll try it when the functionality is stabilized. I'm trying to better separate:

  1. Data (generated files)
  2. Layout (the HTML)
  3. Style (CSS)

@set-soft
Copy link
Member

Some news, I did some code clean-up and changed things so we scan the DOM only when necessary. Now we don't even need jQuery. I guess the new code could have some issues, but is simpler.

This removed the flickering I was experimenting.

I'll try to remove the mousetrap dependency.

@leoheck
Copy link

leoheck commented Dec 22, 2023

That is great, I was using mixed styles there and have wanted to convert it to a single style for a long time. Do you have a branch/fork or something where we could check how it works? I was playing with the SVG Filter Matrix, it is pretty hard, but ChatGPT helps a bit. However I could not find a way to generate something like Red/Blue/Gray yet, maybe there isn't a way to keep the gray when the colors are not the ones I am using right now.

@set-soft
Copy link
Member

That is great, I was using mixed styles there and have wanted to convert it to a single style for a long time. Do you have a branch/fork or something where we could check how it works?

Currently only integrated with KiBot in the kiri_integration branch. The relevant files are stored in the kibot/resources/kiri/ folder.

I was playing with the SVG Filter Matrix, it is pretty hard, but ChatGPT helps a bit. However I could not find a way to generate something like Red/Blue/Gray yet, maybe there isn't a way to keep the gray when the colors are not the ones I am using right now.

The problem is that the matrix is applied to an individual image and you just interact by overlapping the two images. So the "not modified color" is just a combination of the colors used for both images. Red and Green can't generate gray.

@leoheck
Copy link

leoheck commented Dec 26, 2023

Yeah, thats exactly what is happening. The matrix is not that complex in the end. Do you know if there is another mechanism to compare svgs?

@set-soft
Copy link
Member

Yeah, thats exactly what is happening. The matrix is not that complex in the end. Do you know if there is another mechanism to compare svgs?

Nope, I'll investigate more about the filters.

I pushed more changes,I managed to make the generated web pages usable when you are off-line. The cost is that we must include a copy of the core JS libs (368K) and the icons (52K). Big, but not much when compared to the SVGs.

I removed:

  • mouse-trap using a key events handler, really simple.
  • iconify, including the SVGs for the icons and adding some CSS styles for them
  • fontawesome ... not sure what needed it, just removed it and couldn't find a change

@leoheck
Copy link

leoheck commented Dec 26, 2023

GitHub has 3 approaches to compare svgs.

You can check this in this example I could find quickly while browsing on my smartphone.

https://git.arapiraca.ufal.br/carlisson/2status-ufalara/-/commit/82eee0711f2b6e0c44cf48826b0ef94c9502c36e?view=parallel

I wanted to integrate them in the past, but I still think the current diff on Kiri is a bit better.

@set-soft
Copy link
Member

I wanted to integrate them in the past, but I still think the current diff on Kiri is a bit better.

I agree, these side/side, overlapping, etc. aren't really good.

I experimented using filters and found that the browser applies different smoothing filters to the source images, preventing from doing a XOR. Here is a test feComposite.zip

image

You can clearly see the change, that could be colored, but the unchanged stuff is wrong, the borders are different.

I tried pixelmatch, and it works really nice, but is bitmap oriented, so you need a lot of work in order to implement the pan and zoom. Here is an example: pixelmatch.zip

It generates:

image

In order to run both locally I used google-chrome --allow-file-access-from-files test.html

Some viewBox manipulation of the SVGs could potentially solve the pan and zoom, but I'm not sure how to achieve it. When you load the image using an Image object the image is converted to a bitmap. In order to pan and zoom it we need to keep the SVG and then use something to render it as a bitmap to compute the diff. I don't think I'll investigate more, is tedious and is needed only to get better colors and performance, not a show stopper.

@set-soft
Copy link
Member

I did some more changes, now the index.html has the JavaScript, CSS and SVG icons in one file. Of course it needs the generated data, but is much more simple to copy/move around. Here is an example:

res7.zip

@set-soft
Copy link
Member

set-soft commented Jan 3, 2024

Hi @leoheck !

I created a PR to KiRi: leoheck/kiri#105 containing all the changes I did. The only missing detail is the CSS, JS and SVGs embedding in the index.html. This is just a search and replace, but is tedious and I'm not really good writing shell scripts. The result is usable, but a lot of files are copied, instead of just one (big) HTML. I tested it using a very simple case.

@set-soft
Copy link
Member

set-soft commented Jan 3, 2024

Hi @Nudelsalad !

The KiRi integration is now available in the dev branch, so dev images should contain it. Can you test it?

@set-soft
Copy link
Member

set-soft commented Jan 4, 2024

Wouldn't it be cool if the zones were dimmed or completely hidden?

The above patch allows removing the zones before comparison.

@Nudelsalad
Copy link

Nudelsalad commented Jan 16, 2024

Hi @set-soft! sry for the late response,
just tested out your integration and is working really well. This is just awesome! Thank you for your investigation. I can see not only a better kiri ouput/website but also nearly zero overhead in the fetched repos as with kiri. Here an example output from my ci with no zones being filled:
image

Just awesome thank you so much! I think you can close this issue now:)

@set-soft
Copy link
Member

Ok, I'm closing this FR.

Any glitch should be reported as a separated issue.

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

4 participants