← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] - File format shim for clearance data

 

Oh, and I don’t think this materially alters the support equation: we already have to deal with hand-edited boards, so what’s in the file is never guaranteed to be something Kicad put there.

> On 20 Mar 2018, at 10:19, Jeff Young <jeff@xxxxxxxxx> wrote:
> 
> Hi Seth,
> 
> The original version spit out log entries for skipped elements.  This version follows the XML/browser convention of silently ignoring.  Even though this isn’t an XML format, I think we need to recognise that we live in an XML world and XML processing conventions set expectations.
> 
> The patch strictly checks everything for round-tripping so that there is no data loss.  The pad stuff is really a separate issue: it was meant to be loose only during development and then tightened up, but the tightening step was forgotten.  Since we don’t store pad stuff we don’t understand, it has to be tightened.  In short: if you can round trip stuff you don’t understand then do so; otherwise throw.
> 
> Certainly one use case is opening boards from future versions.  If you edit them, then you’re at your own peril.  This behaviour is common enough that I believe it is well understood (although we should obviously mention it in a version-check dialog).
> 
> Another use case is 3rd-party tools (which might even be written as Python plugins) that want to carry their own stuff around in the board.  These might even be processing/manufacturing instructions that don’t have any visual expression in Kicad anyway.
> 
> Cheers,
> Jeff.
> 
> 
>> On 19 Mar 2018, at 22:51, Seth Hillbrand <seth.hillbrand@xxxxxxxxx <mailto:seth.hillbrand@xxxxxxxxx>> wrote:
>> 
>> Hi Jeff-
>> 
>> A few questions on how you are implementing this:
>> 
>> 1) How does the user know what was skipped?  I can imagine team members with different versions getting into difficulty, especially if the features being skipped change the board layout.
>> 
>> 2) You are removing strict checking for most of the board but you are adding strict checking for pad options. What's the difference?
>> 
>> 3) If we keep these options around but don't allow editing/removing, don't we run into a risk of generating a really wonk-y board that only looks wonky in a newer version of KiCad but looks fine in an older version?  For example, imagine we implement rounded corners as a parameter and then a user with an older version of KiCad edits and saves the board.  The rounded corner is only visible in KiCad 6 but the user in KiCad 5 can edit the board to look right to her only to have her colleague use KiCad 6 and see the track violating DRC.  I think that might be counter-intuitive for users but maybe there's a way around it.
>> 
>> In general, if I understand the use-case, this is to allow users to open newer boards in older KiCad versions?  Is there another use case?
>> 
>> I think I can see clear to this for options like the 3d-model offset where it could be either inches or mm but there isn't a difference in the actual layout.  Allowing general unrecognized options would seem to open up a worm can in terms of support as in "Please post the KiCad version and the file version in order to figure out the problem." 
>> 
>> -S
>> 
>> 
>> 
>> 2018-03-18 9:46 GMT-07:00 Jeff Young <jeff@xxxxxxxxx <mailto:jeff@xxxxxxxxx>>:
>> OK, for your guys’ (re)viewing pleasure, a patch which losslessly round-trips stuff it doesn’t understand.
>> 
>> 
>> 
>> 
>>> On 16 Mar 2018, at 19:15, hauptmech <hauptmech@xxxxxxxxx <mailto:hauptmech@xxxxxxxxx>> wrote:
>>> 
>>> While i would still like to see this (my) shim go in, I agree with wayne. Until/unless a less brittle approach is used for the parser, it's better to signal a problem painfully and maintain the integrity of the file.
>>> 
>>> I have to admit that when i first heard the s-expressions idea I assumed that the intention was to use a lisp like virtual machine for loading and maintaining design data. I've used vm's for data storage before (lua and python), and it's great. Parsing is free and you can jam in functions and other data when needed. 
>>> 
>>> 
>>> 
>>> On 17 Mar 2018 07:17, "Jeff Young" <jeff@xxxxxxxxx <mailto:jeff@xxxxxxxxx>> wrote:
>>> Hi Wayne,
>>> 
>>> Patch reverted.
>>> 
>>> However, I think your concern is misplaced.  If we want to prevent dataloss (ie: from reading a 6.0 file into 5.0), then we should warn the user based on the version string (and give them a chance to say “I still want to open”).
>>> 
>>> But either way, actually failing to read the file leaves the user in a pickle (especially when it’s easy enough for them to try out a nightly that may very well be ahead of their stable).
>>> 
>>> (And for that reason I think it’s important to put it into 5.0, as otherwise it won’t help until we start making 7.0 file format changes.)
>>> 
>>> Cheers,
>>> Jeff.
>>> 
>>> > On 16 Mar 2018, at 18:00, Wayne Stambaugh <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>> wrote:
>>> >
>>> > Jeff,
>>> >
>>> > Please revert this patch.  Any changes to the board file parser and/or
>>> > formatter need to be discussed before the changes are committed.  It has
>>> > been the intention that the board parser be pendantic by design to
>>> > prevent data loss by ignoring unknown formatting.  Allowing anything
>>> > outside of the expected formatting in the board file is not something
>>> > that I want to introduce without some discussion.  Even should we decide
>>> > to accept this patch, I would prefer we put it off until v6.
>>> >
>>> > That being said, the patch fails to build on windows with following error:
>>> >
>>> > C:/msys64/home/wstambaugh/src/kicad-trunk/pcbnew/pcb_parser.cpp: In
>>> > member function 'void PCB_PARSER::parseUnknown()':
>>> > C:/msys64/home/wstambaugh/src/kicad-trunk/pcbnew/pcb_parser.cpp:1269:12:
>>> > error: request for member 'LogText' in '__acrt_iob_func(2)', which is of
>>> > pointer type  FILE* {aka _iobuf*}' (maybe you meant to use '->' ?)
>>> >     stderr.LogText( msg );
>>> >            ^~~~~~~
>>> >
>>> > Cheers,
>>> >
>>> > Wayne
>>> >
>>> > On 3/16/2018 1:08 PM, Jeff Young wrote:
>>> >> Perhaps somewhat germane to this discussion I have removed the strict-format nags from the PCB parser.  It now logs warnings to stderr but otherwise ignores structures it doesn’t understand.
>>> >>
>>> >> I’m not sure that helps hauptmech much as if the file is subsequently written the unknown markup will be lost, but I thought I’d mention it.
>>> >>
>>> >> Cheers,
>>> >> Jeff.
>>> >>
>>> >>> On 7 Mar 2018, at 20:12, hauptmech <hauptmech@xxxxxxxxx <mailto:hauptmech@xxxxxxxxx>> wrote:
>>> >>>
>>> >>> Hi Thomasz,
>>> >>>
>>> >>> I hope I'm able to address you concerns. I'm stuck using kicad stable in many situations. I brought clearances up for discussion last July but no one moved on it, nor are they required to. Clearance management is a major pain point for me so I wrote a fix. This patch will let us (me and the people I collaborate with) work using version 5, but open and close files written with a version patched with clearance handling code.
>>> >>>
>>> >>> I believe that code exactly like this will go into version 6. Getting it in earlier makes a huge difference to me, so I'm submitting it.
>>> >>>
>>> >>> On 07/03/18 23:30, Tomasz Wlostowski wrote:
>>> >>>> Hi hauptmech,
>>> >>>>
>>> >>>> I'm sorry but IMHO we can't accept your patch:
>>> >>>> - it changes the file format while we are already in feature freeze.
>>> >>>> This is a way too big change to accept for the V5.
>>> >>>
>>> >>> This patch simply adds tokens to the file format. No clearance data is saved for files that use the netclass only. Files without clearance tokens continue to remain without them.
>>> >>>
>>> >>> Yes it is a backward compatible file format change, but it does no harm to V5 files already in the wild.
>>> >>>
>>> >>>> - we are planning to overhaul the clearance/design rules system in V6.
>>> >>>> Storing the clearance *DIRECTLY* for each track segment/via will
>>> >>>> conflict with any more sophisticated design rule management system.
>>> >>>>
>>> >>> I'm glad you are planning this. I am sure that regardless of the sophistication of the rule system, you will store clearance directly for exactly the same reason that track width is stored directly now. There are always exceptions to the rules.
>>> >>>
>>> >>> If kicad choose a direction that does not store clearances per item, then it is easy to rip these few lines back out.
>>> >>>
>>> >>> Did this answer your existing concerns about this patch? Are there any other concerns you have about this patch?
>>> >>>
>>> >>>
>>> >>>
>>> >>> _______________________________________________
>>> >>> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> >>> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> >>> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> >> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> >> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>>> >>
>>> >
>>> > _______________________________________________
>>> > Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> > Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> > More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>>> 
>>> 
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>> 
>> 
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>> 
>> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp


Follow ups

References