MitjaNemec / PlaceFootprints

GNU General Public License v2.0
27 stars 6 forks source link

Duplicate Ref. when sheets share name prefix #21

Open mdavidsaver opened 8 months ago

mdavidsaver commented 8 months ago

I think I have an interesting corner case. As odd as it seems. I think sheet names are somewhere being compared by string prefix.

We have a circuit with 32 identical channels. These were initially named Channel1 -> Channel9, Channel10 -> Channel19, ...

This produces the odd result that "Channel1" seems to be confused with "Channel11", "Channel2" with "Channel21", and "Channel3" with "Channel31".

I am able to reproduce this with the place_footprints.kicad_pro example project by adding "Sheet21", which is confused with "Sheet2".

place-dialog

R1001 is not actually duplicated, and R301 is actually present.

I can rename "Sheet2" -> "Sheet02" as a workaround.

place-dialog2

I am running kicad 7.0.10 (Debian 12) with PlaceFootprints 2.1.2.

mdavidsaver commented 8 months ago

Here is a sub-string match. "Sheet2" in "Sheet21" evaluates as True.

https://github.com/MitjaNemec/PlaceFootprints/blob/6f5ab45d3e435e38c416aed03576af7f75483e3a/action_place_footprints.py#L176

Changing in to == fixes the error described above. However, I suspect this change would break for a nested hierarchy?

mdavidsaver commented 8 months ago

Another note, perhaps unrelated. The string returned by footprint.GetPath() is different for Sheet21 vs. the others. eg.

Which passes through the .replace('00000000-0000-0000-0000-0000', '') with a much longer sheet_id. However, it isn't obvious to me that this replace() is actually necessary.

MitjaNemec commented 8 months ago

Hi,

thanks for reporting the issue and especially for looking in the code and locating the source of the issue. I'll have to refactor the test to support your case with hierarchical sheets.

Can you share your project? Or can you prepare a test project where I could test possible solutions. This would reduce time required from my side.

As for your observation on the sheet paths:

  1. Which KiCad version dod you use when creating the project originally? V5 had completely different sheet path encoding scheme.
  2. The replace was required for plugin to work in V6 when project was created in V5. It would maybe be possible to simplify the code now, but as I don't have the time do solve more important issues, this will stay as it is.
MitjaNemec commented 8 months ago

So this took way longer than expected. As I am currently porting the plugins to V8, I managed to test the code only in V8 and I've backported the fix to V7 compatible branch of plugin. It is available as 2.1.4. If you can find the time to test it, I'd appreciate it

mdavidsaver commented 8 months ago

... It is available as 2.1.4. If you can find the time to test it, I'd appreciate it

Unfortunately, something still goes wrong. The effect is the same, although the dialog is now partially reversed.

In the project place_footprints.zip the dialog does not show a duplicate. However, the order of the refdes is somehow reversed from the corresponding sheet name. R301 is actually on the Channel2 sheet, R1001 is on the Channel21 sheet, etc.

place

If I continue, then as before R301 (on Channel2 sheet) is not placed.

Also as before, renaming the Channel2 sheet as Channel02 allows R302 to be placed. The dialog ordering issue appears after a rename as well.

mdavidsaver commented 8 months ago

Changing sheet comparison to exact equality fixes the issue with a single level of hierarchy.

diff --git a/action_place_footprints.py b/action_place_footprints.py
index 083a2ae..e0c929f 100755
--- a/action_place_footprints.py
+++ b/action_place_footprints.py
@@ -300,7 +300,7 @@ class PlaceBySheetDialog(PlaceBySheetGUI):
         for fp in footprints_with_same_id:
             self.logger.info(f'fp.sheet_id: {repr(fp.sheet_id)}')
             for sheet in self.list_sheetsChoices:
-                if "/".join(sheet) in "/".join(fp.sheet_id):
+                if sheet==fp.sheet_id:
                     self.ref_list.append(fp.ref)
                     break
         self.logger.info(f'self.ref_list: {repr(self.ref_list)}')
@@ -729,7 +729,7 @@ class PlaceFootprints(pcbnew.ActionPlugin):
             fp_references = [ref_fp_ref]
             for sheet in sheets_to_place:
                 for fp in footprints_with_same_id:
-                    if "/".join(sheet) in "/".join(fp.sheet_id):
+                    if sheet==fp.sheet_id:
                         fp_references.append(fp.ref)
                         break

I tried to experiment with a pathological situation (place_hier.zip ) with two levels of hierarchy (child and grandchild), with some of the child and grandchild sharing a sheet name, but some unique. Placement of a component at the first level (R201) works the same as with the single level project.

At the second level, things do not behave as I expect. Although I do not trust my expectations...

mdavidsaver commented 8 months ago
  1. Which KiCad version dod you use when creating the project originally? V5 had completely different sheet path encoding scheme.

I am not certain how to distinguish a V5 project from V7 on disk. The place_footprints.zip linked above originates from your place_footprints_test_projects/. The place_hier.zip project is newly created with kicad 7.