MitjaNemec / Kicad_action_plugins

Kicad action plugins
413 stars 62 forks source link

Replication plugin error on Nightly 5.99 #102

Open hildogjr opened 3 years ago

hildogjr commented 3 years ago

I am having the following error when executing the replication plugin:

Screenshot from 2020-09-09 09-10-54

replicate_layout.log

MitjaNemec commented 3 years ago

Thanks for reporting. I've seen this with some other plugin, so I'll have to look in previous issues. But it will take some time as I doubt I'll find the time in September.

How are you running the plugin with 5.99 at all? The schematics format changed so the plugin should fail to parse it and the timestamp format changed. How old is the 5.99 version you are running.

hildogjr commented 3 years ago

Just migrated the system now (I was in the older Ubuntu, stuck with older Nighties, without the timestamp change) but I have no current plans for huge layouts.

I was just testing and reporting all issues of my migration.

MisterHW commented 3 years ago

Same issue here - I need 5.99, but also need to replicate layout sections. Extremely unfortunate.

Any plans to use group functionality?

MitjaNemec commented 3 years ago

Can you elaborate a bit more on group functionality? Is it present within pcbnew? How would you use it within the replicate plugin?

MisterHW commented 3 years ago

In v6, schematic and board items have uuids, and groups have lists of member uuids. Copy&paste, rotation and translation of groups is already supported in the footprint editor as well as pcbnew.

When looking at groups, or copies of grouped items, one immediately thinks of duplicating groups and re-associating the copies with items of hierarchical sheets. In my opinion there is a bit of a gray zone there as one might have parts a group that change across the copies, like I²C address pull-up /pull-down configurations, but a common-denominator approach (e.g. leaving the address pin routing out of the group) could just work.

image

With groups, it could also be possible to attempt to propagate later changes without having to delete all other duplicates and starting over.

MitjaNemec commented 3 years ago

I still don't follow, so please bear with me. You've got a group. You lay it down. But how would the plugin find corresponding footprints from other groups?

As it is now, basing on hierarchical sheets, the plugin can find connections through footprint UUID's, I don't have a slightest idea how this could work.

One of my design decisions I try to uphold as much as I can is that the plugin has to be as foolproof and robust as it can. And at the moment I do not see how could I do this with groups. But I probably just need to get additional info regarding this approach. So if you have some, please share it.

MisterHW commented 3 years ago

I try to uphold as much as I can is that the plugin has to be as foolproof and robust as it can

Yes, please :+1:

I'm not an active developer, so I cannot comment on how the v6 version of replicate_layout could be implemented. But allow me to paraphrase my proposition from above.

Instead of replicating the entire layout contents of hierarchical sheets (same schematic file), one could

In my current project, hierarchical sheets are already a couple of levels deep. With replicate_layout only acting on entire hierarchical sheets, we'd have to further partition a part of the schematic across a boundary that doesn't seem to make a lot of sense from the point of schematic readability, but would be required to be able to handle the layout replication. Here, expressing the subdivision by grouping corresponding items of a reference section of the layout ought to be equivalent to the hierarchical sheet re-organization.

MitjaNemec commented 3 years ago

Aaaah, now I got it. Thanks.

You've got hierarchical schematics, but you'd only want to replicate part of the hierarchical sheet (for whatever reason, it does not really mater). The part would be selected via group feature.

This would be doable. Even more so if python API will have access to groups. But even without it, it would be doable. Come to think of it, it would be doable even in 5.1.x. But I am not starting on this. Anyhow even when V6 comes out, this feature will be quite low on the TODO list.

I've opened an issue for this (#109)

MisterHW commented 3 years ago

Yes :)

Also, thanks for logging the feature request.

MitjaNemec commented 3 years ago

@hildogjr There is a test branch which should work with 5.99 available.

Only replicate layout plugin is supported.

hildogjr commented 3 years ago

Thanks @MitjaNemec , I tryed both: master and 5.99-test branch. The first one still with the issue (of course), the second one doesn't load into the plugin list (maybe some import error?).

MitjaNemec commented 3 years ago

Can you attach Replicate_layout_error.log file that should be in the same folder as the plugin. And you can also paste the output when you run

import pcbnew
pcbnew.NOT_LOADED_WIZARDS
pcbnew.FULL_BACK_TRACE

in the pcbnew scripting console.

Thanks

hildogjr commented 3 years ago

I got the first error looking into the tool log.

Both variables pcbnew.NOT_LOADED_WIZARDS and pcbnew.FULL_BACK_TRACE return ''.

Now I have a long-long execution log error (attached the first lines). replicate_layout.log

MitjaNemec commented 3 years ago

Hmmm, the big part of the .log is the sexp module debug logging which should be disabled .

Can you attach only the last part of the file?

MitjaNemec commented 3 years ago

Closed by mistake

hildogjr commented 3 years ago

Just the remove the log initialization didn't stopped it so, I strip out each logger mention on sexp_parser.py.

Follow the log replicate_layout.log

MitjaNemec commented 3 years ago

Still no info within the .log file what the error is. I don't know why is it clipped. Did Kicad hang and become unresponsive or did it crash and you got any kind of error message?

As I see it is a multiple channel board. Can you replicate only one or two channels and we'll see where this will get us?

hildogjr commented 3 years ago

After strip out the logging, KiCad didn't hanged.

I reproduced with the test project on the plugin.

replicate_layout.log

hildogjr commented 3 years ago

For simplicity sake I remove some element of your test bench. Follow the changes and the log in this case (replicating "Leg"):

replicate_layout.log

image image image

MitjaNemec commented 3 years ago

Thanks, I've pushed a fix, can you try it again

hildogjr commented 3 years ago

Sure.

The error log: replicate_layout.log

MitjaNemec commented 3 years ago

Another API change caught. Wash, rinse and repeate.

This is getting a bit tedious, but at the moment I don't plan to setup my local 5.99 environment,

MisterHW commented 3 years ago

Hi, just dropping by to say I appreciate the work :) Have a nice weekend

MitjaNemec commented 3 years ago

@MisterHW Thanks. It is always nice to be appreciated. @hildogjr did you manage to find the time and test new version?

hildogjr commented 3 years ago

New log replicate_layout.log

Be worried that there is a new API capability into pcbnew the footprint.GetProperty('NAME') that for the example of "J201" connector bellow may allow you to get "Sheetfile" and "Sheetname".

  (footprint "Connector:Pusa_4mm" (layer "F.Cu")
    (tedit 5A141F73) (tstamp 00000000-0000-0000-0000-00005b7740f9)
    (at 93.345 28.575)
    (property "Sheetfile" "Leg.kicad_sch")
    (property "Sheetname" "Leg1")
    (path "/00000000-0000-0000-0000-00005b767fe4/00000000-0000-0000-0000-00005b772913")
    (attr through_hole)
    (fp_text reference "J201" (at 10 -1) (layer "F.SilkS")
      (effects (font (size 1 1) (thickness 0.15)))
      (tstamp a5282bba-0b15-4c2d-92e2-9ef7a45afb49)
    )
    (fp_text value "Pusa_4mm" (at 12 1) (layer "F.Fab") hide
      (effects (font (size 1 1) (thickness 0.15)))
      (tstamp 37dc53c7-f361-4bdc-b45e-fc850c3c58b5)
    )
    (fp_circle (center 0 0) (end 7 0) (layer "F.CrtYd") (width 0.1) (fill none) (tstamp 65aa1bff-f7b2-4785-a16f-c4f4a3baaed2))
    (fp_circle (center 0 0) (end 2 0) (layer "F.Fab") (width 0.1) (fill none) (tstamp 917763ff-f275-4da9-a2f1-1e993b3117a3))
    (fp_circle (center 0 0) (end 7 0) (layer "F.Fab") (width 0.1) (fill none) (tstamp fcf144b5-0513-4c94-a196-6f4e09b5a555))
    (pad "1" thru_hole rect (at 0 0) (locked) (size 10 10) (drill 4) (layers *.Cu *.Mask)
      (net 8 "/Leg1/Sensor/OUT") (pinfunction "Pin_1") (pintype "passive") (tstamp 78df72b1-d724-4413-b5d8-a869b8f80033))
    (model "${KISYS3DMOD}/Connector.3dshapes/Pusa_4mm.wrl"
      (offset (xyz 0 0 0))
      (scale (xyz 1 1 1))
      (rotate (xyz 0 0 0))
    )
  )
MitjaNemec commented 3 years ago

Whoa, thanks for the info. I don't get your reference, why I should be worried. From where I look this might come handy and some of my plugins might be able to work without parsing the schematics. Anyhow thanks for the log the issue is known and I'll get on it. When solved, I'll notify you as I'll need your help again

hildogjr commented 3 years ago

Maybe not the best word to express but it could be used to simplify some logic.

MitjaNemec commented 3 years ago

@hildogjr you might want to try it again. It should at least run. But I am skeptical how the zones, text items and drawings will be replicated

EeliK commented 3 years ago

Be worried that there is a new API capability into pcbnew the footprint.GetProperty('NAME') that for the example of "J201" connector bellow may allow you to get "Sheetfile" and "Sheetname".

"Be aware" would indeed be semantically better. But here's something from the scripting console:


>>> fp1.GetPath().AsString()
'/2017ec01-526a-42d9-86f1-5dc7f621ac58/614a9b3b-1788-4ed3-af54-f7db90bc1603'
>>> fp1.GetProperties()
{'Sheetfile': 'untitled.kicad_sch', 'Sheetname': 'Untitled Sheet'}
>>> fp2 = fplist.pop()
>>> fp3 = fplist.pop()
>>> fp3.GetPath().AsString()
'/d0baa0b1-de3a-4974-b633-395f4f70d9ff/614a9b3b-1788-4ed3-af54-f7db90bc1603'
>>> fp3.GetProperties()
{'Sheetfile': 'untitled.kicad_sch', 'Sheetname': 'Untitled Sheet1'}
>>> 
EeliK commented 3 years ago

But I am skeptical how the zones, text items and drawings will be replicated

Zones, tracks, rule areas and text seem to be replicated. Graphic items and dimensions not.

The view isn't refreshed properly or there's something else wrong: the locations of footprints change but replicated items aren't visible until I save, close and reopen pcbnew.

MitjaNemec commented 3 years ago

Thanks for the report.

Drawings replication is currently commented out (lines 436-445) so that explains that.

As for the view refreshing, I seem to recall this mentioned on GitLab discussions. I'll have to look it up, but it seems as a regression from the KiCad side.

Thanks for the report, it is appreciated

EeliK commented 3 years ago

https://gitlab.com/kicad/code/kicad/-/issues/7065

EeliK commented 3 years ago

Drawings replication is currently commented out (lines 436-445) so that explains that.

With my latest PR these work, too. Does the plugin now do everything the 5.1 version does without other issues than adding items to the board problem (i.e. visibility & lacking undo problem)?

MitjaNemec commented 3 years ago

Yeah, the plugin should now have feature parity. But I am going to leave it in 5.99 branch as it is. I expect that before KiCad V6 is released, there will be some changes (support for content/plugin managers, usage of eeschema python API, ...)

hildogjr commented 3 years ago

Appear almost fine now. We just have to disable the sex_parser log. It will create a huge log file.

EeliK commented 3 years ago

Lest it be forgotten: Sheetfile and Sheetname properties are added to the pcb file only when the pcb has been updated from schematic at least once with version 5.99. If the design has been made with 5.1, migrated to 5.99 but the layout hasn't been updated at least once, these properties aren't there.

I'm currently looking into getting rid of schematic file handling, using these properties instead.

MitjaNemec commented 3 years ago

I'd get rid of extract_subsheets and find_all_sch_files and derive the self.self.dict_of_sheets they gather, from all of the board modules bmod just before I construct list of all modules self.modules

MitjaNemec commented 3 years ago

Btw, thanks for pointing out the corner case

MitjaNemec commented 3 years ago

@EeliK , @hildogjr . I've torn out parsing of the schematics file and instead use the Sheetfile and Sheetname footprint preferences. If you use the plugin, I'd ask you if you would be so kind to update it and report back any issues.

hildogjr commented 3 years ago

It is not working with the embedded example:

replicate_layout.log

https://user-images.githubusercontent.com/2211474/111627687-188f7680-87ce-11eb-97ab-8621d61cd86d.mp4

EeliK commented 3 years ago

Did you update from schematic first? As I said in my previous comment, if that's not done, the pcb file footprints don't have the sheet data. The example project is for 5.1.

Maybe the plugin should refuse to run if the anchor footprint doesn't have the sheet data? It's the same for non-schematic footprints which have been added directly to the board, they don't have sheet data. The plugin works only on footprints from hierarchical sheets so it would make sense to check it in the same place than having exactly one footprint selected before the board is analyzed further.

hildogjr commented 3 years ago

Yes, first I migrate the files to v5.99 and realize that was necessary to sync Eeschema->Pcbnew.

This result is after synchronization.

MitjaNemec commented 3 years ago

@EeliK, I've added a check if footprints have all the required property fields

@hildogjr, the phenomena you observe is a regresion of KiCad where any action(s) from plugins is not shown in the viewport (canvas). You literally had to close and open pcbnew. This should be solved with recent merge request. As it was merged a day ago we might need to wait a day or two so that we get this in the nightlies

EeliK commented 3 years ago

I haven't been able to test the latest changes yet, the board which I have used for replication now gives a cryptic error message when I try to run the plugin:

image

But by looking at the code I would say that checking the existence of sheet data doesn't work as intended. It now loops through all the footprints on the board and if any one of them lacks sheet information it throws an error. This prevents having footprints added directly to the board without the schematic, for example mounting holes or logo which some people don't add to the schematic. They are totally irrelevant for the plugin. Only those footprints which are replicated are relevant. It should be enough to check this only for the anchor footprint because if it has the sheet data then all replicated footprints have it, too, and all other footprints can be ignored. Theoretically it may be possible that one footprint of a sheet has the sheet data and another doesn't, but I think it's highly unlikely.

By the way, even though I'm not sure about how the algorithms work, to me it looks like some needless data is formed and saved to data structures. For sheet data (dict_of_sheets) only the sheet of the anchor footprint, its siblings and their children should be needed. For footprints (mod_named_tuple) only the footprints in those sheets are ever needed. I don't know if this has any practical efficiency effects, though, even when all footprint data is later looped through a few times.

MitjaNemec commented 3 years ago

Thanks for the comment in issue 7065. I was aware of the issue, and I wanted to point you towards it, but I can not find the list of issues I am subscribed to in GitLab.

As for the plugin, thanks for the testing. I am testing it on my obviously limited test layout (included with the plugin) and I did not have any footprints without their representations in schematics. I'll fix this and include this corner case to my test layout.

As for the code optimizations, I am following the first rule of optimizations: "Thou shall not do any code optimizations".

To be specific, I need to get the full schematics hierarchy to offer the user the list of possible destination sheets. So I currently don't see how this could be optimized. But I agree there are probably a lot of optimizations that can be done.

MitjaNemec commented 3 years ago

You should be aware of issue 7982