kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #35203
Re: [PATCH] - File format shim for clearance data
Hi Jeff-
Can we split this patch? I think the addition of strict checking for the
pads should be in 5.
-S
2018-03-27 10:34 GMT-07:00 Jeff Young <jeff@xxxxxxxxx>:
> 5.0, 6.0 or abandon?
>
>
> On 20 Mar 2018, at 16:47, Jeff Young <jeff@xxxxxxxxx> wrote:
>
> 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 <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 <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 <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 <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 <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 <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
> <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 <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
> <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
> <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
> <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
> <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
> <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
> <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
> <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
>
>
> _______________________________________________
> 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
>
>
Follow ups
References
-
[PATCH] - File format shim for clearance data
From: hauptmech, 2018-03-07
-
Re: [PATCH] - File format shim for clearance data
From: Tomasz Wlostowski, 2018-03-07
-
Re: [PATCH] - File format shim for clearance data
From: hauptmech, 2018-03-07
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-16
-
Re: [PATCH] - File format shim for clearance data
From: Wayne Stambaugh, 2018-03-16
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-16
-
Re: [PATCH] - File format shim for clearance data
From: hauptmech, 2018-03-16
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-18
-
Re: [PATCH] - File format shim for clearance data
From: Seth Hillbrand, 2018-03-19
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-20
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-20
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-20
-
Re: [PATCH] - File format shim for clearance data
From: Wayne Stambaugh, 2018-03-20
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-20
-
Re: [PATCH] - File format shim for clearance data
From: Jeff Young, 2018-03-27