kicad-developers team mailing list archive

[miles mccoo <mail@xxxxxxxxxx>]

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