MitjaNemec / ReplicateLayout

GNU General Public License v2.0
90 stars 12 forks source link

V5 to V6 Conversion #34

Closed myamigo closed 1 year ago

myamigo commented 1 year ago

I have a circular PCB developed with V5 that I would like to port to V6 but I am having problems using the replicator. I maintain my design in a partially completed state by only working on a single circuit and then I replicate that circuit about the center of the PCB as 2 copies spaced 120 deg. This has always worked flawlessly with V5 using the legacy replicator included with the Kicad_action_plugins plugins.

I can convert the project to V6 by following the normal process detailed in a response to forum post Guide for migrating projects from 5 to 6

I installed the new ReplicateLayout plugin using the Plugin and Content Manager but when I try to replicate the circuit by selecting an anchor component the script fails with a complaint about the anchor component being on the root schematic, which is does not. I tried deleting the PCB and recreating it from scratch in V6 with the same result. I also tried deleting all but a single replicable sheet with no difference.

Thinking simpler is better I created a dead-simple V5 project which consists of 2 instances of a sheet that each have 2 resistors connected in series... image

... and connected the sheets in parallel at the top-level... image

That's attached as V5_V6_Replicator_Test.zip.

The PCB layout in V5 is fine... image

... and the legacy replicator moves R4 after I adjust the position of R3. image All good there!

Then I open the project with V6 and perform the conversion following the proper steps but I get a fatal error when I try to replicate it... image The error message indicates that there should be a save_restore_layout.log file, which references the other plugin so this must be a copy-paste error. replicate_layout.log is generated and I've attached that as replicatelayout#1.log. I note the log indicates that the resistors have no Sheetfile property, which I recognize from issue #10 Unhelpful error message: "Property not found: Sheetfile"

I then deleted the V6 generated PCB and recreated it from the schematic with the same result but a different log file that I've attached as replicate_layout#2.log. This time I note the final message is AttributeError: module 'pcbnew' has no attribute 'BOX2I'

I really need the replicator to work with the PCB file generated automatically when V6 imports and saves the original V5 file. I have many text adjustments and tweaks that are not practical to recreate from scratch. I need to migrate to V6 to use new features but I am unable to do so until this issue is resolved.

I love this plugin and hope the fix is straight forward.

Thank you!

MitjaNemec commented 1 year ago

Thanks for reporting this.

  1. When you open project that was created in V5 in V6, did you update layout from schematics? I think this is the main culprit.
  2. The reference to 'save_restore_layout.log` is a typo. Thanks for spotting and reporting it.
  3. You are running the 2.0.0 version of the plugin. This version is intended for KiCad V6.99 or latter. You can not run this version on KiCad 6.0.10. You should run plugin version 1.2.7
myamigo commented 1 year ago

It was the plugin version. I installed 1.2.7 and after updating from schematic it worked for my simple project. I'll try the main one now.

Thank you!

myamigo commented 1 year ago

I followed the same steps to import my project and update pcb from sch but I still get the original problem I reported (anchor component on root schematic but it isn't)... image

I also tried deleting all components before doing the update PCB from SCH with the same result.

Log... replicate_layout.log

What do you suggest I try to narrow this down?

myamigo commented 1 year ago

I think I determined that nested sheets are a problem. This project has 2 nested levels of sheets...

TOP: image

Sheet_A: image

Sheet_B: image

When I run the replicator I get the anchor on root error... image

You can change properties of the sheets on the top-level to use Sheet_B.sch instead of Sheet_A.sch and after reannotating the schematic and updating the PCB again you will be able to replicate as before. 2 or more hierarchal levels seems to be the problem.

MitjaNemec commented 1 year ago

Nesting should not be a problem as the plugin supports nested sheets.

The plugin fails at the test at action_replicate_layout.py#L381.

It looks like the anchor footprint is either missing the Filename property (most likely). Or there might be a bug in the plugin code.

Can you open a design in pcb editor and open a python console and type in (line by line) the following code:

import pcbnew
brd = pcbnew.GetBoard()
fp = brd.FindFootprintByReference('R201')
fp.GetPath().AsString()

Also, can you share your project?

MitjaNemec commented 1 year ago

Obviously change the reference to the reference of your anchor footprint

myamigo commented 1 year ago

I included a link to my 2-layer test project in my previous post but here it is... https://github.com/MitjaNemec/ReplicateLayout/files/10346227/V5_V6_Replicator_Test_V2.zip

I'll complete your request next.

myamigo commented 1 year ago

Console results...

import pcbnew brd = pcbnew.GetBoard() fp = brd.FindFootprintByReference('R1') fp.GetPath().AsString() '/00000000-0000-0000-0000-000063b58b46/00000000-0000-0000-0000-000063b5b6e9/00000000-0000-0000-0000-000063b58c44'

I'll try those steps with with the single level project.

myamigo commented 1 year ago

Results with single level project...

import pcbnew brd = pcbnew.GetBoard() fp = brd.FindFootprintByReference('R1') fp.GetPath().AsString() '/00000000-0000-0000-0000-000063b58b46/00000000-0000-0000-0000-000063b58c44'

myamigo commented 1 year ago

I added logger.info(f'src_anchor_fp = {src_anchor_fp}') right after the src_anchor_fp = replicator.get_fp_by_ref(src_anchor_fp_reference) statement and get the following in the log when running the script with single and then two level sheets...

SINGLE LEVEL 01-04 16:17:24 com_github_MitjaNemec_ReplicateLayout.action_replicate_layout 379:src_anchor_fp = Footprint(ref='R1', fp=<pcbnew.FOOTPRINT; proxy of <Swig Object of type 'std::deque< FOOTPRINT * >::value_type' at 0x0000022DC4387210> >, fp_id='63B58C44', sheet_id=['SheetA0'], filename=['Sheet_B.kicad_sch'])

TWO LEVEL 01-04 16:21:21 com_github_MitjaNemec_ReplicateLayout.action_replicate_layout 379:src_anchor_fp = Footprint(ref='R1', fp=<pcbnew.FOOTPRINT; proxy of <Swig Object of type 'std::deque< FOOTPRINT * >::value_type' at 0x00000217D4B12EA0> >, fp_id='63B58C44', sheet_id=[], filename=[])

sheet_id & filename are empty in the 2nd case.

Let me know what to try next and I'll do it if that will help.

MitjaNemec commented 1 year ago

Thanks for the test project. I've managed to find a bug with it. I've failed to remember to add V5 porting code to a patch for a different bug (#27)

But it'll be a while before I publish a new release with the bugfix

So you might want to change the code in replicate_layout.py aroun line 190 from:

                    if "(uuid " in contents[j]:
                        sheet_id = contents[j].replace("(uuid ", '').rstrip(")").upper().strip()
                    if "(property \"Sheet name\"" in contents[j]:
                        sheetname = contents[j].replace("(property \"Sheet name\"", '').split("(")[0].replace("\"", "").strip()
                    if "(property \"Sheet file\"" in contents[j]:
                        sheetfile = contents[j].replace("(property \"Sheet file\"", '').split("(")[0].replace("\"", "").strip()

to

                    if "(uuid " in contents[j]:
                        path = contents[j].replace("(uuid ", '').rstrip(")").upper().strip()
                        sheet_id = path.replace('00000000-0000-0000-0000-0000', '')
                    if "(property \"Sheet name\"" in contents[j]:
                        sheetname = contents[j].replace("(property \"Sheet name\"", '').split("(")[0].replace("\"", "").strip()
                    if "(property \"Sheet file\"" in contents[j]:
                        sheetfile = contents[j].replace("(property \"Sheet file\"", '').split("(")[0].replace("\"", "").strip()
myamigo commented 1 year ago

That was a good fix! My test project replicated properly and then I tried it on my main one and it seemed to work as well. I am going to perform a thorough gerber diff against the V5 outputs to confirm integrity but I did notice something right away.

V5 would replicate all tracks connected to replicated components up to the end of any stubs that I created that connect replicated circuits. V6 with the new replicator does not include all of the stubbed nets and it looks like it is clipping segments that are outside of the region bounding the components being replicated.

This image shows a portion of F.Cu with replicated components and nets. All portions of the the 3 stubbed nets should be selected for replication but they are not. image As per the notes it seems the replicator is using a bounding rectangle to determine if a segment is included or not. This is different from the way this was done with V5.

Let me know what to do to help narrow this one down.

This is a great plugin. Thankyou for your effort.

MitjaNemec commented 1 year ago

There is a chekbox for this image image

myamigo commented 1 year ago

Ah yes! I had enabled that before but noticed it was now replicating zones that are not even associated with the replicated region so I turned it off. The zones that are being replicated are irregularly shaped but their bounding rectangles extend into the replicated region so are being included.

Options:

For now I can delete the duplicated zones.

Thanks for the tip.

myamigo commented 1 year ago

I noticed the new options to not replicate locked tracks, zones, etc. so I figured I could just lock the zones that are being errantly replicated but it seems the Replicate intersection tracks/zones/drawings option overrides the locked excludes setting. Should Don't replicate locked have a higher priority than Replicate intersection tracks/zones/drawings?

MitjaNemec commented 1 year ago

Options:

  • Could the replicator test each point in the zone's polygon to see if any intersect with the bounding region and if so then include that zone?
  • Optionally could the checkbox be split into multiple options so intersecting tracks, zones & drawings could be individually selected. Splitting out just the tracks and leaving zones and drawings together would be fine.

I really don't want to make the UI more complex than it already is. In fact I'd really like to simplify it so if anybody has any good ideas I am open to them. So for corner cases as your you can either use locking or grouping.

it seems the Replicate intersection tracks/zones/drawings option overrides the locked excludes setting. Should Don't replicate locked have a higher priority than Replicate intersection tracks/zones/drawings?

Locked status should have higher priority from looking at the code.

I'd need a test case preferably a simple one (with only two zones, one locked one unlocked). The same code is also responsible to highlight elements when changing settings. So if the zone in question is not highlighted when locked it should not be replicated. Can you confirm if the zone is highlighted or not, when you start the plugin and adjust the settings properly?

MitjaNemec commented 1 year ago

It looks like there is a bug in the code. I've inverted the GUI checkbox meaning, but I did not invert the code logic.

Thanks for spotting this.

myamigo commented 1 year ago

Yes I noticed that reversed checkbox logic. Could just be a mater of removing the word Don't

The following settings get the result I expect... image

I can move forward now. Thank you!