kicad-developers team mailing list archive

[Maciej Suminski <maciej.suminski@xxxxxxx>]

Hi Miles,

On 11/17/2017 02:14 PM, miles mccoo wrote:
> I see. Let me summarize to see if I understand.
> 
> The python wrapper around wx was rewritten. A move to SIP is one of (the
> main) differences that the new version brings with it. I imagine this
> involves more than a simple recompile. At least Kicad doesn't have a ton of
> python code needed to have a basic working kicad.
> 
> I further imagine that SIP and SWIG can't really or perhaps shouldn't
> coexist. Kicad doesn't have a lot of typemaps so while this is work, maybe
> it's not huge.

Yes, I think you got it right. I am not a SWIG or SIP expert to tell
whether they easily cooperate, I am just pointing out that there might
be a few bumps ahead of us. If there are people willing to work on the
scripting interface, then trying out Phoenix might be beneficial to the
project. Obviously, I neither want nor will enforce anyone to take on
the task, but perhaps someone becomes interested in the topic.

> There are a bunch of things on the python side that could be fixed at the
> same time. For one, I'd want to see wx stuff more seamlessly integrated.[1]
> Why do I have to allocate a wxPoint object instead of an (x,y) tuple. stuff
> like that.
> 
> 
> 
> 
> *I am left with one question though*: Has my patch been denied? I could try
> to respond to points raised so for, but perhaps those arguments are already
> irrelevant. This thread has touched on many orthogonal points.

IMHO they make Python fans life easier, which is obviously a desired
thing. The only thing I am afraid of is we may end up exposing too much
C++ interface and it will turn against us at one point. I wish we have
had an abstraction layer to prevent potential problems. If that was the
case - I would not hesitate a second to commit your patch.

In any case, I think the remark about using (x, y) tuples stays valid
regardless of whether we have a Python abstraction layer or not. The
same for the typemap that removes the requirement for EOT sentinel in
types list.

I do not deny the patch, I would like to get some additional input
either from the developers or the users.

Regards,
Orson

> Miles
> 
> 
> [1] I realize that I'm just a random contributor to kicad project.
> Hopefully my kicad scripting efforts give me some credibility
> https://kicad.mmccoo.com/
> I also understand that the kicad team doesn't really put a lot of value in
> the python interface. Based on my career experience, I think it can be
> kicad's most important feature.
> 
> 
> 
> 
> On Fri, Nov 17, 2017 at 9:25 AM, Maciej Suminski <maciej.suminski@xxxxxxx>
> wrote:
> 
>> On 17/11/2017 9:08 AM, miles mccoo wrote:
>>
>>>
>>>     Another thing, there will be no more wxPython releases. The project
>>> has
>>>     been renamed to wxPython and AFAIR nobody has tried to use it with
>>>     KiCad. It may turn out that our SWIG work might be dropped/converted
>>> or
>>>     we will stay stuck with wx3.0 forever.
>>>
>>>
>>> So Kicad will no longer have a python interface? I don't really
>>> understand this part of your mail.
>>>
>>> I googled for Kicad wxPython and the only thing I found is the
>>> description of the build option needed to build kicad with python
>>> support. This is not new.
>>>
>>> I haven't tried to use this new project because I hadn't heard of it
>>> before this mail. Again, maybe I'm just not understanding your mail.
>>>
>>> Can I have a link to more information about the renamed stuff?
>>>
>>
>> Right, it was not very clear due to my mistake. Obviously wxPython has not
>> been renamed to wxPython, but turned into another project called Phoenix
>> [1]. One of the major changes is switch to SIP instead of SWIG and I am not
>> sure how well does it play with our SWIGged interface. I am not saying
>> there will be no KiCad scripting enabled builds, but we may need some extra
>> work to keep them in the future.
>>
>> Regards,
>> Orson
>>
>> 1. https://github.com/wxWidgets/Phoenix
>>
>> Miles
>>>
>>>
>>>
>>>
>>>
>>> On Nov 16, 2017 11:54 PM, "Maciej Suminski" <maciej.suminski@xxxxxxx
>>> <mailto:maciej.suminski@xxxxxxx>> wrote:
>>>
>>>     Hi Miles,
>>>
>>>     I think the silence you get is not caused by the message length, but
>>> two
>>>     other factors:
>>>     - We lack (at least I do) deep SWIG knowledge required to assess the
>>>     patch. More or less, I understand what does your patch change, but I
>>>     cannot tell if this is the right way to do it. The typemap trick and
>>>     removal of the obligatory EOT sentinel look neat though.
>>>     - Exposing more bare C++ interfaces in Python is not what we exactly
>>>     need nor desire. The code is continuously evolving and the more
>>>     interfaces are SWIGged, the more often we face the choice of either
>>>     breaking the existing Python scripts or refraining from code refactor.
>>>
>>>     IIRC it has been agreed a few times that we need an abstraction layer
>>> to
>>>     decouple C++ and Python worlds. I know that there are some other
>>> people
>>>     with deeper knowledge of KiCad scripting who perhaps could lend a hand
>>>     defining the layer. Ideally the interface would arrive with unit tests
>>>     to assure the C++ devs fix the abstraction layer if it becomes
>>> invalid.
>>>
>>>     Another thing, there will be no more wxPython releases. The project
>>> has
>>>     been renamed to wxPython and AFAIR nobody has tried to use it with
>>>     KiCad. It may turn out that our SWIG work might be dropped/converted
>>> or
>>>     we will stay stuck with wx3.0 forever.
>>>
>>>     Regards,
>>>     Orson
>>>
>>>     On 11/16/2017 08:43 PM, miles mccoo wrote:
>>>     > Actually, forget everything I wrote in my previous mail. Too long,
>>>     no one
>>>     > wants to read all that and most of it's not relevant to my patch.
>>>     >
>>>     >
>>>     > My patch doesn't change core kicad in any way. It only exposes the
>>>     existing
>>>     > connectivity class to python. The only deviation from the existing
>>> c++
>>>     > interface is that I protect python users from the mistake of
>>>     forgetting the
>>>     > EOT on the GetNetItems.
>>>     >
>>>     > Otherwise, it's simply the addition of some more %includes in the
>>>     .i files.
>>>     >
>>>     > Not sure what happens next. My other patches were merged pretty
>>>     quickly
>>>     >
>>>     > Miles
>>>     >
>>>     >
>>>     > On Thu, Nov 16, 2017 at 11:02 AM, miles mccoo <mail@xxxxxxxxxx
>>>     <mailto:mail@xxxxxxxxxx>> wrote:
>>>     >
>>>     >>
>>>     >> Interesting thread. Lots of stuff that I hadn't stumbled across
>>>     before.
>>>     >>
>>>     >> There's a lot in my answer to cover the ground raised by
>>>     everyone. For
>>>     >> those that don't make it to the end, I'll say that there are no
>>>     clear next
>>>     >> steps for me. I'd like to be able to get from a net to that net's
>>>     pads. I'd
>>>     >> like to do that in a way that's better than just filtering
>>>     through all of
>>>     >> the board's pads. Guidance please, on what I need to do to make
>>>     progress in
>>>     >> that direction.
>>>     >>
>>>     >>
>>>     >>
>>>     >>
>>>     >> First, it's probably worth repeating that I want to go from a
>>>     specific net
>>>     >> to the pads that that net connects to. I see that as a fundamental
>>>     >> operation, but perhaps there's disagreement on that. I'm not
>>>     trying to get
>>>     >> to all Pads on the board or on a module. It is already possible
>>>     to iterate
>>>     >> through all pads and filter through them for the net [1]. Six
>>>     months ago,
>>>     >> it was possible to as a net for its pads via the Pads() method.
>>>     >>
>>>     >>
>>>     >>
>>>     >> On 15/11/17 18:38, Wayne Stambaugh wrote:
>>>     >>> It looks like the PCB_COLLECTOR objects are not swigged yet but
>>> that
>>>     >>> would be the way to do it.  Properly swigged, there would be
>>> nothing
>>>     >>> preventing you from collecting all objects connected to an net
>>>     or any
>>>     >>> other collection criteria for that matter.  That's what the
>>>     collector
>>>     >>> object is for.  Let's not reinvent the wheel.
>>>     >>
>>>     >> To use something like this a script writer would instantiate a
>>>     >> PCB_COLLECTOR (or one of its child classes GENERAL_COLL...) for a
>>>     net and
>>>     >> then ask for the pads?
>>>     >>
>>>     >> No sure if the wheel part is to me. I wasn't aware of the wheel.
>>>     Is there
>>>     >> another document repository somewhere that maybe I missed?
>>>     >>
>>>     >>
>>>     >>
>>>     >>
>>>     >> On Wed, Nov 15, 2017 at 6:49 PM, Tomasz Wlostowski
>>>     <tomasz.wlostowski@
>>>     >> cern.ch <http://cern.ch>> wrote:
>>>     >>
>>>     >>> I'm fine with swigging the COLLECTORS, but they aren't in my
>>>     opinion the
>>>     >>> simplest way to execute complex queries on a board. I would use
>>>     >>> iterators instead. The DLIST_ITERS in the BOARD class that hide
>>> the
>>>     >>> DLISTs behind a C++-style iterator can be extended with filter
>>>     methods,
>>>     >>> e.g.:
>>>     >>>
>>>     >>> // Iterates all tracks in net 1 and on Top layer.
>>>     >>>
>>>     >>> for ( auto track : board->Tracks().InNet(1).OnLayer( LAYER_TOP )
>>>     ) {...}
>>>     >>>
>>>     >>
>>>     >>
>>>     >> I will take an analogy to an extreme. To get the name of a net:
>>>     >>
>>>     >> for (auto names : board->Names.InNet(1))
>>>     >>
>>>     >> I think of net->pad mappings as fundamental to the data model.[2]
>>>     >>
>>>     >>
>>>     >> Also, it seems like both PCB_COLLECTOR and DLIST_ITERS solutions
>>>     still end
>>>     >> up just iterating over all pads in the design. Perhaps I shouldn't
>>> be
>>>     >> avoiding that. Premature optimization? A browse through the
>>>     connectivity
>>>     >> stuff made me think it's got efficient ways for this kind of thing.
>>>     >>
>>>     >>
>>>     >>
>>>     >>>>>  1. The new connectivity stuff is in connectivity.h. Perhaps
>>>     it should
>>>     >>>>>     be called class_connectivity.h to match the naming of
>>>     other files?
>>>     >>>>>     better yet class_connectivity_data.h
>>>     >>>> Nope. Ideally, we should have one class-one file mapping, we're
>>> not
>>>     >>>> there yet. Even the files that are prefixed with class_* don't
>>>     follow
>>>     >>>> the rule strictly (e.g. CLASS_BOARD). I'd rather rename it to
>>>     >>>> connectivity_data.h to match the inner class name.
>>>     >>
>>>     >> I agree that the CLASS_ prefix is not really helpful. Still, it
>>>     appeared
>>>     >> to be the standard for this project.
>>>     >>
>>>     >> One class per file is not what I was advocating. That kind of rule
>>> is
>>>     >> silly in real design. connectivity_data is the only class in that
>>>     file
>>>     >> (other than two minor structs) so I thought the filename should
>>> match
>>>     >>
>>>     >>
>>>     >>
>>>     >>>>>  3. The API that I'm exposing in python is this one:
>>>     >>>>>     const std::list<BOARD_CONNECTED_ITEM*> GetNetItems( int
>>>     aNetCode,
>>>     >>>>>                                            const KICAD_T
>>>     aTypes[] )
>>>     >> const;
>>>     >>>>>
>>>     >>>>>     And is used like this:
>>>     >>>>>     const KICAD_T types[] = { PCB_PAD_T, PCB_ZONE_AREA_T, EOT };
>>>     >>>>>     auto netItems = m_connectivity->GetNetItems( i, types );
>>>     >>>>>
>>>     >>>>>     why not just use a std::vector? Adding the EOT is another
>>>     thing
>>>     >>>>>     folks have to remember to include.
>>>     >>>> I'm OK with that. Feel free to add convenience wrappers for
>>>     Python (e.g.
>>>     >>>> BOARD::GetNetItems, etc.) in the swig interface files.
>>>     >>
>>>     >> A wrapper is not needed given how my patch exposes GetNetItems,
>>>     but you
>>>     >> didn't know that because it's buried in one of the comments in
>>>     the patch:
>>>     >>
>>>     >> this typemap allows any of the following:
>>>     >>    conn = board.GetConnectivity()
>>>     >>    conn.GetNetItems(net.GetNet(), (pcbnew.PCB_PAD_T,
>>>     pcbnew.PCB_TRACE_T))
>>>     >>
>>>     >>
>>>     >>
>>>     >>>>
>>>     >>>> Concerning the use of std::list - it's there probably because the
>>>     >>>> previous ratsnest code (RN_DATA) returned a std::list. I don't
>>> mind
>>>     >>>> changing the function to return std::vector. Could you update
>>>     your patch
>>>     >>>> when it's done?
>>>     >>
>>>     >> I wasn't referring to the return value. I meant the filter
>>> criteria.
>>>     >>
>>>     >>  GetNetItems( int aNetCode,  const KICAD_T aTypes[] ) const;
>>>     >>
>>>     >> is called like this:
>>>     >>  const KICAD_T types[] = { PCB_PAD_T, PCB_ZONE_AREA_T, EOT };
>>>     >>  m_connectivity->GetNetItems( i, types );
>>>     >>
>>>     >>
>>>     >> I suggest something like this test code (which will compile,
>>>     given c++
>>>     >> -std=gnu++11):
>>>     >> #include <vector>
>>>     >>
>>>     >> enum blah { Blah1, Blah2, Blah3 };
>>>     >>
>>>     >> void GetNetItems(std::vector<blah> &b)
>>>     >> {
>>>     >>   // safely get the filter types from b.
>>>     >> }
>>>     >>
>>>     >> int main() {
>>>     >>   GetNetItems({Blah1, Blah3});
>>>     >> }
>>>     >>
>>>     >>
>>>     >>>>>  4. m_connectivity is a public member of connectivity_data and
>>>     used all
>>>     >>>>>     over the place. My head is ringing with the voices of my
>>>     college
>>>     >> TAs
>>>     >>>>>     about access methods.
>>>     >>>> Nope, it's not. I don't know what version of Kicad code you
>>>     looked at,
>>>     >>>> but certainly in the current master m_connectivity is private.
>>>     >>
>>>     >> Not sure where I got this one. Even looking at the history it's
>>>     private.
>>>     >>
>>>     >>
>>>     >>
>>>     >> [1] I suspect that at the end of this thread, this is what I'll
>>>     have to do.
>>>     >>
>>>     >> [2] In my previous life, I worked on a VLSI tool with a very
>>>     uniform data
>>>     >> model. Every object knew about its most basic relationships and
>>>     getting to
>>>     >> those was possible in a uniform way.
>>>     >> Every object had a getName() method. Every object had a
>>>     get<classname>s,
>>>     >> where classname was the name of the class the relationship was
>>>     with. So
>>>     >> cell has getNets, getInstances. Net has getCell, getInstancePins.
>>>     Instance
>>>     >> has getCell, getInstancePins. InstancePin has getNet, getPin (the
>>>     pin on
>>>     >> the master)
>>>     >>
>>>     >>
>>>     >> Thanks, if you made it this far.
>>>     >> Miles
>>>     >>
>>>     >>
>>>     >>
>>>     >> On Wed, Nov 15, 2017 at 6:49 PM, Tomasz Wlostowski <
>>>     >> tomasz.wlostowski@xxxxxxx <mailto:tomasz.wlostowski@xxxxxxx>>
>>> wrote:
>>>     >>
>>>     >>> On 15/11/17 18:38, Wayne Stambaugh wrote:
>>>     >>>> It looks like the PCB_COLLECTOR objects are not swigged yet but
>>>     that
>>>     >>>> would be the way to do it.  Properly swigged, there would be
>>>     nothing
>>>     >>>> preventing you from collecting all objects connected to an net
>>>     or any
>>>     >>>> other collection criteria for that matter.  That's what the
>>>     collector
>>>     >>>> object is for.  Let's not reinvent the wheel.
>>>     >>>
>>>     >>> Hi Wayne,
>>>     >>>
>>>     >>> I'm fine with swigging the COLLECTORS, but they aren't in my
>>>     opinion the
>>>     >>> simplest way to execute complex queries on a board. I would use
>>>     >>> iterators instead. The DLIST_ITERS in the BOARD class that hide
>>> the
>>>     >>> DLISTs behind a C++-style iterator can be extended with filter
>>>     methods,
>>>     >>> e.g.:
>>>     >>>
>>>     >>> // Iterates all tracks in net 1 and on Top layer.
>>>     >>>
>>>     >>> for ( auto track : board->Tracks().InNet(1).OnLayer( LAYER_TOP )
>>>     ) {...}
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>> Cheers,
>>>     >>> Tom
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>>>
>>>     >>>> On 11/15/2017 10:59 AM, Tomasz Wlostowski wrote:
>>>     >>>>> On 15/11/17 15:37, miles mccoo wrote:
>>>     >>>>>> Hello,
>>>     >>>>>>
>>>     >>>>> Hello Miles,
>>>     >>>>>
>>>     >>>>> Thanks for the patch. My comments below:
>>>     >>>>>
>>>     >>>>>>
>>>     >>>>>> A couple code review type observations on the existing code.
>>>     >>>>>>
>>>     >>>>>>  1. The new connectivity stuff is in connectivity.h. Perhaps
>>>     it should
>>>     >>>>>>     be called class_connectivity.h to match the naming of
>>>     other files?
>>>     >>>>>>     better yet class_connectivity_data.h
>>>     >>>>> Nope. Ideally, we should have one class-one file mapping,
>>>     we're not
>>>     >>>>> there yet. Even the files that are prefixed with class_* don't
>>>     follow
>>>     >>>>> the rule strictly (e.g. CLASS_BOARD). I'd rather rename it to
>>>     >>>>> connectivity_data.h to match the inner class name.
>>>     >>>>>
>>>     >>>>>
>>>     >>>>>>  2. The initial commit comment of connectivity.h should says
>>>     this:
>>>     >>>>>>     "New connectivity algorithm."
>>>     >>>>>>     It would have been helpful to have more. Presumably,
>>>     there's a
>>>     >>>>>>     design document somewhere describing the change. Or
>>>     perhaps and
>>>     >>>>>>     email thread? Could have saved some hunting.
>>>     >>>>>
>>>     >>>>>>  3. The API that I'm exposing in python is this one:
>>>     >>>>>>     const std::list<BOARD_CONNECTED_ITEM*> GetNetItems( int
>>>     aNetCode,
>>>     >>>>>>                                            const KICAD_T
>>>     aTypes[] )
>>>     >>> const;
>>>     >>>>>>
>>>     >>>>>>     And is used like this:
>>>     >>>>>>     const KICAD_T types[] = { PCB_PAD_T, PCB_ZONE_AREA_T, EOT
>>> };
>>>     >>>>>>     auto netItems = m_connectivity->GetNetItems( i, types );
>>>     >>>>>>
>>>     >>>>>>     why not just use a std::vector? Adding the EOT is another
>>>     thing
>>>     >>>>>>     folks have to remember to include.
>>>     >>>>> I'm OK with that. Feel free to add convenience wrappers for
>>> Python
>>>     >>> (e.g.
>>>     >>>>> BOARD::GetNetItems, etc.) in the swig interface files.
>>>     >>>>>
>>>     >>>>> Concerning the use of std::list - it's there probably because
>>> the
>>>     >>>>> previous ratsnest code (RN_DATA) returned a std::list. I don't
>>>     mind
>>>     >>>>> changing the function to return std::vector. Could you update
>>> your
>>>     >>> patch
>>>     >>>>> when it's done?
>>>     >>>>>
>>>     >>>>>>  4. m_connectivity is a public member of connectivity_data
>>>     and used
>>>     >>> all
>>>     >>>>>>     over the place. My head is ringing with the voices of my
>>>     college
>>>     >>> TAs
>>>     >>>>>>     about access methods.
>>>     >>>>> Nope, it's not. I don't know what version of Kicad code you
>>>     looked at,
>>>     >>>>> but certainly in the current master m_connectivity is private.
>>>     >>>>>
>>>     >>>>> Tom
>>>     >>>>>
>>>     >>>>> _______________________________________________
>>>     >>>>> 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>
>>>
>>>
>>>
>