kicad-developers team mailing list archive

[Maciej Suminski <maciej.suminski@xxxxxxx>]

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> 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> 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> 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
>>>>> 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
>