MitjaNemec / SaveRestoreLayout

GNU General Public License v2.0
22 stars 5 forks source link

Would it make sense to get rid of the schematic hash check? #3

Open ducky64 opened 1 year ago

ducky64 commented 1 year ago

It's a check that's brittle, it's sensitive to even nonfunctional changes in the schematic. It also needs its own dedicated section in the readme. Does it make sense to just get rid of it?

I assume the goal is to guard against functional changes in the schematic, but it seems redundant. Wouldn't DRC catch cases where the schematic connectivity changed? And if there were differences in the component list, then it would be detected on a restore anyways, at which point it could restore the components that do match and warn the user of the nonmatching components?

MitjaNemec commented 1 year ago

Yeah, I agree V6 schematics format is much more likely to change hash value while maintaining same connectivity/netlist than V5 schematics did.

I also have to say that I practically did not use the V6 version of the plugin, so I don't really have the same experience using the plugin as I did with V5 version of the plugin. And I did not get much feedback besides yours. If you would be more verbose on your experiences (preferably with schematics file(s) examples) I'd greatly appreciate it.

As for the hash check, it is possible that plugin may run correctly even when source and destination schematics differ. Emphasis on "may". But I try to make my plugins as foolproof as possible. It reduces number of issues raised significantly. That is one reason why there is the hash check. Another reason is that it simplifies the plugin code. When hashes match there are significantly less chances for plugin to fail. So if hashes differ I only need to report one message back to the user. If the plugin would continue chances for plugin fail somewhere increase. And it seems fair that in case of error, the plugin should catch it, and report a sensible message to user. So I'd have to research all the possible failure points, catch them and report back sensible messages.

So I am not really inclined to remove the hash check. But I am open to adding an option to continue even when hashes don't match. With proper warning to the user obviously.

ducky64 commented 1 year ago

I have not used V6 much either, I'm only starting to migrate to it - so sadly not much feedback there yet šŸ˜”. I've used the V5 version, though, it's a real convenience!

There are two use cases I'm looking at:

Anyways, I think reducing this to a warning instead of a fatal error would address both cases. That being said, for the latter, I haven't tested the current behavior for what happens if there are components missing in the source or destination.

MitjaNemec commented 1 year ago

Previously it would be necessary to generate a dummy schematic Yeah, this should be solved now, though, there might be a bug in edge cases, but it'll be a while before I build test project, or I get a proper issue reported.

it would be interesting to be able to directly export a layout from an older board and drop it into a newer board with a newer (but mostly similar) schematic. That is really a nice goal to strive to.

But That being said, for the latter, I haven't tested the current behavior for what happens if there are components missing in the source or destination. this is an issue that I think is not solvable.

For cases where schematics changed a bit currently (and I think also in the long run) the only sensible solution is to use old schematics, restore old layout and then update the schematics. It is a bit of work, but I don't see a better way.

I'll add the option to force restore even when hashes don't match. With big disclaimer.

NickKnatterton commented 1 year ago

I think removing the check would also fix problems associated with the new schematic format keeping all footprint/value informations in the root schematic. When copying schematics between projects, this leads to changes in the schematic. I guess that is because KiCAD is refactoring the schematic to have all footprint/value information in the root sheet after some work on the project. (see https://gitlab.com/kicad/code/kicad/-/issues/11712 or https://forum.kicad.info/t/problems-update-symbols-and-value-in-subsheet-v6-0/35391) That behaviour basically breaks the work flow for using your plugin to copy layouts from one project to another. I havenĀ“t found a conclusive description on how the part information in hierarchical designs is going to be handled in the future, so itĀ“s probably to early to think about a final solution for it.

MitjaNemec commented 1 year ago

Yeah V6 schematics behavior is problematic when using this plugin. But if you (@NickKnatterton) have any examples where plugin fails due to hash mismatch, please report them to me. I can expand what should be ignored when calculating the hashes https://github.com/MitjaNemec/SaveRestoreLayout/blob/main/save_restore_layout.py#L73

This should make plugin a bit more robust

ducky64 commented 1 year ago

Previously it would be necessary to generate a dummy schematic Yeah, this should be solved now, though, there might be a bug in edge cases, but it'll be a while before I build test project, or I get a proper issue reported.

I can verify that's solved in ReplicateLayout, which doesn't need schematic files at all, but SaveRestoreLayout still hard-gates on the schematic as part of the hash check. If the schematic doesn't exist, it errors out. If the hash check is commented out, it works fine.

But That being said, for the latter, I haven't tested the current behavior for what happens if there are components missing in the source or destination. this is an issue that I think is not solvable.

I haven't looked in depth into the existing implementation to see if this is feasible, but I'd imagine one intuitive behavior would be to calculate the overlapping set of components (those present in both source and destination), then restore only those. If we wanted to be fancy, similar behavior could apply to traces - drop traces that connect to components which no longer exist in the destination. Though even if it restored all traces, it's still probably better than failing and not doing anything at all, and in the end DRC will still catch any errors. The user can be warned about component mismatches.

NickKnatterton commented 1 year ago

@MitjaNemec Unfortunately I can not really reproduce the issue reliably... But when comparing the schematic files, I found that besides reference and value also the footprint line ist changing. So adding exclusion for the footprint line should fix this. For now I have removed the hash check to see if I run into any unexpected problems and it seems to work fine for the import of multiple layouts. Maybe it would be useful to discuss an alternative to checking schematic similarity? As @ducky64 described it is not really a "minimal" set of information and many changes do not affect the PCB. Probably the application of the plugin is rather diverse and requirements vary widely with use-case. For me itĀ“s reusing tested pieces of schematic + layout in different projects like building blocks. Thus for me focus is on stability of the import process. As I only copy/paste the schematics, changes are nearly always due to KiCADs internal workings (e.g. reassigning UUID/timestamps when forking one subsheet from another in the same project). Based on that application I propose the following approach:

MitjaNemec commented 1 year ago

I am on vaccation so I didn't read through this with proper concentration. I can get the gist of your proposal. The plugin can be made to work but (a big but):

And for me that is a lot of work

MitjaNemec commented 1 year ago

For anyone interested, there is a (minor) change in schematics file encoding in place https://gitlab.com/kicad/code/kicad/-/merge_requests/1328

SimonMerrett commented 1 year ago

If this is unrelated, I'm happy to open a new issue. I get this failure which looks like it is at the hash check stage: image

From log file

10-21 15:07:07 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 149:Plugin executed on: 'win32'
10-21 15:07:07 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 150:Plugin executed with python version: '3.9.14 (main, Sep 19 2022, 05:11:57) [MSC v.1932 64 bit (AMD64)]'
10-21 15:07:07 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 151:KiCad build version: (6.0.8)
10-21 15:07:07 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 152:Plugin version: 1.1.3
10-21 15:07:07 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 153:Frame repr: <wx._core.Frame object at 0x000002331921A550>
10-21 15:07:07 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 171:Anchor footprint reference is 'U201'
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 180:Save layout chosen
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 287:Working on C:/Users/local/Repos/_/hardware/TC_Radio_standalone/TC_Radio_standalone.kicad_pcb
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 132:getting a list of all footprints on board
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 144:Footprint REF** does not have Sheetfile property, it will not be considered for placement. Most likely it is only in layout
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 292:Saving the current board temporary in order to leave current layout intact
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 298:Saving board as tempfile: C:\Users\local\AppData\Local\Temp\temp_board_file_for_save.kicad_pcb
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 303:Loaded temp boardfile: C:\Users\local\AppData\Local\Temp\temp_board_file_for_save.kicad_pcb
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 304:Get project schematics and layout data
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 132:getting a list of all footprints on board
10-21 15:07:09 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 144:Footprint REF** does not have Sheetfile property, it will not be considered for placement. Most likely it is only in layout
10-21 15:07:17 com_github_MitjaNemec_SaveRestoreLayout.action_save_restore_layout 219:Saving the layout in 'C:\\Users\\local\\Repos\\_\\hardware\\TC_Radio_standalone\\TC_Radio_standalone.pckl' for level 0
10-21 15:07:17 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 315:Saving layout for level: ['Radio']
10-21 15:07:17 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 316:Calculating hash of the layout schematics
10-21 15:07:17 com_github_MitjaNemec_SaveRestoreLayout.save_restore_layout 320:Saving hash for files: ['File: Radio_.kicad_sch']

I'm afraid I can't share the project but grateful if any workaround is known. The sub-sheet I'm trying to save layout for is sheet 2 in the hierarchy (root is sheet 1).

ducky64 commented 1 year ago

It's been a while, but I'm interested in making this happen - schematic-less support for Save Restore Layout (and also Replicate Layout - I think a recent version now needs schematics as well). I'm open to doing some of the engineering work if there's agreement on an acceptable user-facing workflow for board files without schematics.

NickKnatterton commented 1 year ago

For me this would boil down to more of a usability improvement. So far I havenĀ“t gotten around to using any of the "code your netlist" tools, but I expect the plugin to become far less susceptible to non-functional schematic changes as well. And those have reall are a pain; cost of ownership for a collection of circuits to reuse quickly goes up with new KiCad versions, library changes etc. Regarding the workflow: where do you see changes from the current approach of using an anchor footprint and restoring from there? I think removing the need for a schematic could be something most users would not notice when using the plugin. (and would also not impede their work with "regular" schematic based layouts) My head locked on some kind of crosscheck on the netlist saved in the layout file. WhatĀ“s your current idea?

ducky64 commented 1 year ago

For where the circuit is the same between saved and restored, there should be no workflow changes. In Kicad 6.0 the layout contains the hierarchical path for each footprint, so the information needed for an anchor footprint based flow should be available in the layout file.

There might be an edge case if there's a sheet with no footprints (eg, only sub-sheets), since the footprint path only contains the unique ID for each object and the (immediate) containing sheetfile - so in this case there might not be a way to identify which sheetfile the unique ID corresponds to. I'm less sure how to handle this - it would probably need a schematic at this point, but a fallback in the absence of a schematic could be something that tells the user that some hierarchical parts did not match a sheetfile. Also, to be completely thorough, there isn't any guarantee that the layout is up to date with the schematic, which would be another reason to try to do as much as possible with the layout only.

Without gating on equivalence, the other edge case would be if the saved layout is missing a component, or if it has an additional component. While I guess it isn't unreasonable for restoring to fail entirely on a mismatch, I'd think (and feel free to disagree) that a partial best effort makes more sense. If the saved layout is missing a component, the component in the target layout could be untouched - maybe with some kind of highlight to indicate that it's missing. If the saved layout has an extra component, it could instead display an error message that some component wasn't restored because it no longer exists.

When components are missing in the target, it might also make sense to delete connected traces. Though practically I think it's fine if it restored the layout with dangling traces - I'd also say it's better than refusing to restore at all. And if there's a connectivity mismatch in the restored layout, DRC should catch it after the fact.

In practice, by commenting out the schematic checks, both here and in later version of replicate-layout that does such checks, the plugin mostly works as expected even with a non-Kicad-schematic flow. But it'd be nice to not need to maintain my own hacked version with stuff commented out.

MitjaNemec commented 1 year ago

while I am open of adding such a feature the main sentiment (also stated in #12) that I have is that there should be additional checks to let the user know if we detected anything funny (different nr. of footprints, same number of footprints bu some have different UUID, ...).

Architecturally it is probably likely that restore functionality would have to be split in order to let the user know via UI what the plugin has found out regarding the source layout and destination board. I don't mind restoring partially, we just have to let the user know.

Then we'd need to prioritize whether to implement #3 first or #12 first. Obviously any development of new features should go only on V7 compatible version.

And finally I have to let you know that I am quite short on time and V6/V7 migration took some energy. In the short term I'd like to close the https://github.com/MitjaNemec/ReplicateLayout/issues/38. I currently have the replicate layout plugin architecture/code in my head and I am really not keen to context switching. Thanks for understanding