kicad-developers team mailing list archive

[Jeff Young <jeff@xxxxxxxxx>]

Hi Wayne,

This solution is pretty much “property” tokens except that:

1) it’s more flexible (properties can be arbitrary s-expressions)
2) it handles 3rd-party-needs and our own with a single infrastructure

I was going to add (3) it requires name-spacing, but “property” tokens will require name spacing anyway (to keep 2 different 3rd parties from colliding).  The only difference is that we get the default name-space to ourselves.

In fact, I think the “property” angle is a much better angle to look at it from.  Rather than think of it as a file-format issue, think of it as s-expression meta-data for BOARD_ITEMS.  If we later decide that "(hole-to-hole-clearance 0.25)" should define a distance used by DRC, then great.  Until then, it’s just an opaque s-expression.

I also think it demonstrates that the risk of wiping out stuff editing a future board with an older version isn’t really there.  Just like “properties”, we’ll round-trip whatever meta-data was in the board.  Sure, new BOARD_ITEMS that you add won’t have the future data, but why would they?  

(And if we remain concerned José’s force-save-as solution is an excellent idea.)

The code complexity it adds to the parser is tiny.  It turned out to be far easier to implement than anticipated.

Cheers,
Jeff.


> On 20 Mar 2018, at 14:48, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
> 
> Jeff,
> 
> I wanted some time to think about this.  It has been discussed before.
> While I'm not completely opposed to it, I still haven't found a more
> compelling argument to convince me that it is a better idea than using
> strict parsing.  As a user, it does have a certain appeal.  As a
> developer, it opens a Pandora's box of issues that once they are in
> play, could be extremely difficult to reverse.  Please see my responses
> below.
> 
> On 3/20/2018 9:40 AM, Jeff Young wrote:
>> @Wayne, did you have any thoughts on this iteration?
>> 
>>> On 20 Mar 2018, at 10:22, Jeff Young <jeff@xxxxxxxxx
>>> <mailto:jeff@xxxxxxxxx>> wrote:
>>> 
>>> 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.
> 
> True, but this is one of the reasons that the board parser is strict.
> It is immediately obvious what doesn't belong in the file.  Also, in the
> past users have tricked eeschema and pcbnew into a feature that didn't
> technically exist by taking advantage of the loose file parsing.  Then
> when something changed internally and their clever hacks were broken, we
> ended up with bug reports.  I do not want to repeat this again.
> 
>>> 
>>>> On 20 Mar 2018, at 10:19, Jeff Young <jeff@xxxxxxxxx
>>>> <mailto: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.
> 
> After reading this, I am even more thankful that we didn't choose XML
> for our file format.  Gleefully ignoring file syntax that the parser
> doesn't understand IMO is a bad idea.  This flies in the face of the
> basic premise that we control the format of our files not someone else.
> As soon as you allow others to dictate your file format, you are in trouble.
> 
>>>> 
>>>> 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.
> 
> The round tripping is fine and makes sense but it also adds to the code
> complexity of the parser.  The pad issue is different.  I don't know
> when that was slipped into the parser but it should not be there.  If
> there is an unexpected token, that should flag a file load error.
> 
>>>> 
>>>> 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).
> 
> Perhaps, but I just see this ending badly.  Maybe I'm being paranoid
> here but it's so easy to imagine a scenario where someone did a lot of
> editing in a newer version of kicad then makes the fatal mistake of
> opening the board in an older version of kicad and wipes out a lot of
> work.  Technically not kicad's fault but I'm not so sure the user will
> see it that way.
> 
>>>> 
>>>> 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.
> 
> This is one of the primary reasons that I do not like ignoring unknown
> file formatting.  It creates the potential for name space pollution that
> could cause issues down the road.  The eventual goal is to implement the
> "property" token to define key/value pairs for third party applications
> to add user specific information to any board object without polluting
> the controlled part of the file format.  The beauty of this is that we
> do not have to coordinate with 3rd party developers to ensure we are not
> clashing with anything the are working on.  They are free to add
> whatever properties they would like while we still maintain strict file
> parsing.  This was always part of the grand plan for the board file
> format that kind of got lost in the noise and me becoming project leader.
> 
> Cheers,
> 
> Wayne
> 
>>>> 
>>>> 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
>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>> More help   : https://help.launchpad.net/ListHelp
>>> 
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : 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
>> 
> 
> _______________________________________________
> 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