Remove unconnected net cues for netname connected nets

[Peter Clifton]

This came up after a presentation with potential users.

The red cues on the end of unconnected net was perceeved as a nice warning that something was unconnected.

Their expectation was that a net connected at one end should lose the second cue (on its unconnected end) if the net is named. This could be implemented either to mean "has a netname=" attribute, or that it is connected to something which has one.

This helps to preserve the semantic that "red splodge" == something to fix.

We also noted that the pin cues were not nearly as visible as they ought to be when projected!

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

Making sure that net has a "netname=" is not enough - there might be no other net with same name.
Potentially the whole hierarchy has to be traversed to check for that.

Related bug #706552 has a patch to make unconnected pin ends stand out better.

[KaiMartin (kmk-familieknaak)]

This would receive a warm welcome by me.

---<)kaimartin(>---

[Peter Clifton (pcjc2)]

@Krzysztof,

It sounds like getting the red cues right is becoming more and more like a back-annotated DRC overlay from the design scope!

For now, the simple case of removing the cue when the net is named would be a help (so designers can see the bad red-dots). An electrical DRC is the right place to detect lack of connectivity - perhaps in the future we can back-annotate this as a live overlay on the problem areas.

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

I have something in the works for this - please see the top two patches from the attached series.
Remaining points:
* the "full connectivity" of each net should be cached for speed and recomputed when needed.
* screen redraw needs to be triggered when connectivity or net attributes change.

[KaiMartin (kmk-familieknaak)]

I tested the patch. And it works great!
The red marks really stick out. But they don't get into the way, even when zoomed way in.

Looking forward to see this in head.

---<)kaimartin(>----

[Peter TB Brett (peter-b)]

I have a few concerns with these patches.

1. is_net_fully_connected() is recursive, not iterative. Since C doesn't optimise tail calls, it means that on designs using very complex nets we might see stack exhaustion.

2. is_net_fully_connected() treats a netname attribute as a connection, but that's not reflected in the doxygen comments.

3. We allocate and build a hashtable for each and every net object in the entire design, every time we draw, which is rather expensive. Is there any way of optimising things so that the "fully connected" flag is only updated when a user modifies the design?

Thanks!

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

> 1. is_net_fully_connected() is recursive, not iterative. Since C doesn't
> optimise tail calls, it means that on designs using very complex nets we
> might see stack exhaustion.

Even if rewritten to be iterative, we would have to maintain a stack
of visited net segments ourselves - in addition to a hash the current
version uses. This might give us better control over memory usage and we
could fail gracefully when allocation fails - but will likely complicate
the function. Is gschem even run on systems where stack does not grow
automatically?

> 2. is_net_fully_connected() treats a netname attribute as a connection,
> but that's not reflected in the doxygen comments.

The Doxygen comment in is_net_fully_connected() says:

    Net is fully connected when it connects at least two separate
    entities. "netname" attribute being attached to one of the net
    segments counts as an connected entity as well.

Any suggestions how to reword this to make it more clear?

> 3. We allocate and build a hashtable for each and every net object in
> the entire design, every time we draw, which is rather expensive. Is
> there any way of optimising things so that the "fully connected" flag is
> only updated when a user modifies the design?

That is on a TODO list - we would have to keep "fully connected" cached
flag in the net objects, or maintain a separate cache of precomputed
values per net segment. The latter would not require making changes to
libgeda, as this is a feature only gschem would make use of. It seems
the most difficult part is to catch all cases where the cache should be
updated.

Any suggestions on the caching approach?

[Martin Kupec (martin-kupec)]

Krzysztof Kościuszkiewicz:
>Even if rewritten to be iterative, we would have to maintain a stack
>of visited net segments ourselves - in addition to a hash the current
>version uses. This might give us better control over memory usage and we
>could fail gracefully when allocation fails - but will likely complicate
>the function. Is gschem even run on systems where stack does not grow
>automatically?

The problem here is, that the size of the stack is usually set to some potentially "low" maximum.
It is usually enough, but I just recently came to the limitation. The program was just segfaulting at strange different locations. It is a pain to debug when you have no idea what are you dealing with.

Having own "stack" for this purpose is a bit more work, but can save a lot of work later when you encounter problems.

This maximum exist at least on Linux and Windows.

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

I have added caching in OBJECT structure, rewritten main function to eliminate recursion and triggered connectivity cache refresh in connection subsystem.

The whole approach still seems like a hack to me, to support everything properly I need to hook up cache refresh for some specific objects (nets) and attribute names (netname) in both connection (s_conn.c)
and attribute (o_attrib.c) subsystems.

And the icing on the cake is to trigger redraw of affected nets when connectivity potentially changes...

[Peter Clifton (pcjc2)]

See (not 100% complete) "attribute hook" work here:

http://repo.or.cz/w/geda-gaf/pcjc2.git/commitdiff/2156a7294a1aeaf4b97f6d6d5e7da3d584acd296
and
http://repo.or.cz/w/geda-gaf/pcjc2.git/commitdiff/308016b93bba3882f67c228cfd42acf6a7e9740a

NB: These URLs will be out of date if / when I push any changes to my stgit branch, sorry.

They live in this branch:

http://repo.or.cz/w/geda-gaf/pcjc2.git/shortlog/refs/heads/local_customisation

And are titled:

"Attribute change hooks"
and
"Perform slot updates based on attribute changed hooks"

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

Yes, hooks look like a good implementation strategy here.
I can make use of your "attribute" hooks and also create "conn" hooks (CONN* being added or removed).

[Peter Clifton (pcjc2)]

Cool,

The only reason I've not pushed that code so far is that it seems to cause some g_warnings to get fired as you shut down gschem in some conditions. I thinl it is related to hooks firing and causing attributes to be updated whilst gschem is being shut down, but I'm not 100% sure. If i recall correctly, it was pango which was complaining.

Other than the g_warnings, the code works fine (and has the benefit of splitting out the slotting implementation away from lots of special-cased calls). Perhaps I should push the code as-is and we can fix any warnings during the developmen cycle?

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

I think you can push it as is and later it can be reworked - maybe to a more general hook system in libgeda.

[Martin Kupec (martin-kupec)]

Hi, I just tested the patch series and I have a little problem with it.

It removes the red cues even form unnamed nets. So when I create a net with 2 components and than
make another line from it ending nowhere it has no cues.

I think it would be better to remove the cue just from "named nets with at least 2 different nets with the same name".

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

> Hi, I just tested the patch series and I have a little problem with it.
>
> It removes the red cues even form unnamed nets. So when I create a net
> with 2 components and than make another line from it ending nowhere it
> has no cues.

That is expected. I assumed red cue should mark an incomplete net. If
you connect at least two different nets/buses/elements with a net, then
it is "complete" from connectivity point of view. A netname attribute
attached to a segment counts as a connection in the above sense.

The short stub you attached to the complete net looks weird from
schematic point of view, but from connectivity point of view its
existence/absence is indifferent.

So the question is if we want to mark:
 * any endpoint that is not connected (netname attribute = connection)
 * all endpoints of the net whose connectivity is not complete

> I think it would be better to remove the cue just from "named nets with
> at least 2 different nets with the same name".

This could work this way, but has deficiencies - full blown solution would
have to dig into hierarchy (which might be costly) and search across all files
within a "project" - but currently we do not have anything that resembles a
"project" in gschem and libgeda - just a loose collection of schematic files.

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

In the absence of further comments I have pulled the attribute hooks from Peter C, added connectivity hooks and rewritten the code to use these.

Right now the only missing functionality is redrawing the objects when the when the connectivity changes.

The branch pin-cues can be cloned from here:
http://repo.or.cz/w/geda-gaf/kokr.git

[Peter TB Brett (peter-b)]

Hi Krzysztof,

Any chance of an update on how close this branch is to fully-cooked and ready to merge yet, please?

Thanks!

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

Hi Peter,

I did rebase the branch to git HEAD, but a few things in unrelated code blow up gschem for me right now.
I'm currently too busy with things unrelated to geda but I'll try to get the branch finished & merged once HEAD is stable.

Take care!

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

I got gschem working again. Still two problems to solve:
1) net connectivity status is not initialised properly on schematic load
2) not all connectivity is updated on deletion of net segment

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

I have fixed 1) and I believe the branch can be now merged.
Problem 2) is not blocking, as screen redraw will fix all stale pin cues.
To fix 2) properly I would have to create a generic function for traversal of connections for libgeda - this needs more discussion and is more work than I can devote to at the moment.

Note: as stated before the unconnected pins/nets now show up as red squars in both print and screen output (bug #706552). If there's no agreement to change PS output, this change should be removed before the merge.

[Peter TB Brett (peter-b)]

I'm not a big fan of the TOPLEVEL creation hooks: I don't think we should be adding non-reentrant stuff to libgeda at this stage. For now, is it possible to do that setup in the normal TOPLEVEL initialisation function?

Failing that, I suggest moving those hooks to private API for the time being.

Otherwise, this all looks pretty fun. :-)

[Peter TB Brett (peter-b)]

Actually, Krzysztof, maybe you should just go ahead and push this. We can always back the extra hooks out at a later stage, if necessary.

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

Pushed to git HEAD:
http://git.gpleda.org/?p=gaf.git;a=commit;h=8342bddce4487edf4a7214d5d6ab83cb73a066d4

(btw - commit robot still chokes on non-ascii bytes in git output...)