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>
