Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Processing Jumpers and Loops, Plus Processing Wires inside the Table as Edges #369

Draft
wants to merge 32 commits into
base: dev
Choose a base branch
from

Conversation

tobiasfalk
Copy link

@tobiasfalk tobiasfalk commented May 30, 2024

Here I started to implement Jumpers, and a new way of drawing Wires inside the Wire table.

Currently, only the Black circles are done and nothing else.
See ex15.png
Ps. And now even the right branch

Edit:
Ex15:
ex15

Ex16:
ex16

@tobiasfalk
Copy link
Author

Now everything from #350 is implemented.
ex15

@tobiasfalk
Copy link
Author

Syntax:

connectors:
  X1:
    type: Molex KK 254 # more information
    subtype: female
    pinlabels: [GND, VCC, RX, TX, GND, VCC, GND] # pincount is implicit in pinout
    internal_shorts: [[1, 5, 7], [2, 6]]
    internal_shorts_color: [PK, RD]
  X2:
    type: Molex KK 254
    subtype: female
    pinlabels: [GND, VCC, RX, TX, GND, VCC, GND]
    internal_shorts: [[1, 5, 7], [2, 6]]

@tobiasfalk tobiasfalk changed the title [WIP] Jumpers, A start to implement #350 Jumpers, implementaition of #350 May 30, 2024
@amotl amotl changed the title Jumpers, implementaition of #350 Processing Jumpers Jun 4, 2024
Changed the syntax and added options for the shorts, see ex15 for more
@tobiasfalk
Copy link
Author

New Syntax:

connectors:
  X1:
    type: Molex KK 254 # more information
    subtype: female
    pinlabels: [GND, VCC, RX, TX, GND, VCC, GND] # pincount is implicit in pinout
    shorts:
      SH1:
        pins: [1, 5, 7]
        color: PK
        manufacturer: WireViz
        mpn: 42XCD42A5
        description: shortPart
      SH2:
        pins: [2, 6]
        color: RD
        manufacturer: WireViz
        mpn: 42XCD42A5
        description: shortPart
        length: 42

@tobiasfalk
Copy link
Author

ex15

@tobiasfalk
Copy link
Author

ex16:
ex16

@kvid
Copy link
Collaborator

kvid commented Jun 9, 2024

I'm sorry that I've not yet had time to play with this PR, but it seems to work as proof of concept for the #350 discussions.

  • I suggest that you activate "convert to draft" while this is work-in-progress (I do forget that with my PRs also sometimes).
  • Be aware that your latest syntax expansion probably will need coordinaton with changes in Large scale refactoring #251 that is first in line for merge into dev.
  • The way you want to combine internal shorts and loops, might also need a discussion in Way to implement Jumpers/Interconnects #350 because I fear it will be too complicated for the simplest use cases.

@tobiasfalk tobiasfalk marked this pull request as draft June 9, 2024 10:00
@tobiasfalk
Copy link
Author

I mainly have problems with the bom and how I add the shorts to the bom

@kvid
Copy link
Collaborator

kvid commented Jun 9, 2024

I mainly have problems with the bom and how I add the shorts to the bom

Have you considered making the additional components list be a union of shorts and explicitly specified entries, instead of having a separate list for shorts? Then adding to BOM could be handled equally as well.

@tobiasfalk
Copy link
Author

@kvid @martinrieder could you look at the new syntax in ex15 & 16?

Copy link

@martinrieder martinrieder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a bit of rework. Comments added.

examples/ex15.yml Outdated Show resolved Hide resolved
manufacturer: WireViz
mpn: 42XCD42A5
type: shortPartA
qty: 42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find qty confusing for unit mm. How about length as an alias?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but Additional Components works with qty

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just leave it, as changing this is not in scope.

examples/ex16.yml Outdated Show resolved Hide resolved
examples/ex15.tmp Outdated Show resolved Hide resolved
Comment on lines 6 to 8
shorts:
- SH1: [1, 5, 7]
- SH2: [2, 6]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a list here is fine. Does not match the syntax description though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax description is yet to be updated.

src/wireviz/Harness.py Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
}
else:
common_args = {
"qty": part.qty * component.get_qty_multiplier(part.qty_multiplier),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numShorts could be the return value of the function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not completely understand what you mean

Copy link
Author

@tobiasfalk tobiasfalk Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had the qty_multiplier choice extended for shorted, so I thought that the function get_qty_multiplier might return numShorts as an integer in that case. The meaning has changed now with references.

else:
numShorts = len(part.shorts)

if numShorts > 0:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be avoided, see below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not completely understand what you mean

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the order of comments got mixed up and my statement was possibly misleading.

tutorial/tutorial01 Outdated Show resolved Hide resolved
@tobiasfalk
Copy link
Author

Overall question is if the syntax is acceptable?
If so Checks and other things can be done.

@martinrieder
Copy link

martinrieder commented Jun 15, 2024

@tobiasfalk wrote:

Overall question is if the syntax is acceptable?

I would prefer a solution that does not require using loops or shorts in two different locations. Alternatively, I suggest to use a reference key instead for both, since designators have to be unique anyways. It will also avoid the situation like in ex15 where loops and shorts got unintentionally mixed up.

Overall, I would say that this implementation is quite close to what was discussed in #224 (comment). The only difference is that reference was implied, but implementing that is a little more tricky. Still, I think that we should clarify the outcome of #224 before merging this PR.

Please also have a look at #288, which also implements some loop handling and might cause some merge issues. What I need for my daily work is having alphanumeric pin names and pinlabels matching to work also on loops. I am currently using a workaround for this.

ex15

connectors:
  X1:
    #[...] 
    pinlabels: [GND, VCC, RX, TX, GND, VCC, GND]
    shorts:
      - SH1: [1, 5, 7]  
      - SH2: [2, 6]
    additional_components:
      - reference: SH1 # discussed in #224
        color: PK
        #[...] 
        
      - reference: SH2
        color: RD
        #[...] 

  X2:
    #[...] 
    pinlabels: [GND, VCC, RX, TX, GND, VCC, GND]
    shorts:
      - SH1: [1, 5, 7] # same designator as above... 
      - SH2: [2, 6]   # does this imply the same attributes? 

EDIT: Cross-referring to #224 (comment) for additional details.

@martinrieder
Copy link

martinrieder commented Jun 16, 2024

Just adding lables will make it ugly, maybe some text in the connector table?

Yes, I was thinking the same. You would also need to name the color, similar to how it is done for wires. For the colorblind it seems impossible to recognize the loops otherwise.

What about choosing a connector side for loops?

@tobiasfalk
Copy link
Author

tobiasfalk commented Jun 16, 2024

@martinrieder
I have the Idea to add additional_cable to the connector and add references like with AddComp. Would also "solve" your complaint about the unit/qty attribute.

@martinrieder
Copy link

martinrieder commented Jun 17, 2024

@tobiasfalk please explain your reasoning... The way I understand this, it could be solved by #376, which also allows defining a connector side for loops. It would not be displayed in the additional components table then.

I would call the two approaches:

@tobiasfalk
Copy link
Author

@martinrieder my reasoning behind it was to group it with the same type of wire/cabel in the overall bom.
But this can also be added later.

@tobiasfalk
Copy link
Author

If the base is OK, I will add a tutorial and then wait for #251 to then move/reimplement this. Since this will create a bunch of conflicts.

@tobiasfalk
Copy link
Author

@martinrieder
About the Problems with the Black Circle, I really do have no Idea to have a fix for all systems, since fonts often have rendering problems.
I asked for help in the GraphViz Forum: https://forum.graphviz.org/t/way-of-drawing-a-black-circle-inside-a-table-field/2273

I thought about that maybe replacing it with an X but then the centering Problem of SVG would be more prominent.

@martinrieder
Copy link

martinrieder commented Jun 18, 2024

@tobiasfalk how does the × multiplication sign (×) perform instead of the letter X?
It is part of ISO/IEC 8859-1 that can be explicitly defined through the charset attribute.

@tobiasfalk
Copy link
Author

You will have the same Problem as with X

@tobiasfalk
Copy link
Author

tobiasfalk commented Jun 18, 2024

Also, it is kinda small:
PNG:
ex15

SVG:
ex15

@martinrieder
Copy link

@tobiasfalk I finally got around to test your PR on my dev machine. It fails to run gvpr due to a path issue or missing environment variable:

Error: gvwrite_no_z problem 57
gvpr: Could not find file "pin2pin.gvpr" in GVPRPATH

You may need to use an absolute path for calling this script. I tried to set the environment variable to point to the right path and also moved pin2pin.gvpr` into the same folder, but it fails to run.

@tobiasfalk
Copy link
Author

@tobiasfalk I finally got around to test your PR on my dev machine. It fails to run gvpr due to a path issue or missing environment variable:

Error: gvwrite_no_z problem 57
gvpr: Could not find file "pin2pin.gvpr" in GVPRPATH

You may need to use an absolute path for calling this script. I tried to set the environment variable to point to the right path and also moved pin2pin.gvpr` into the same folder, but it fails to run.

Will look in to it

@martinrieder
Copy link

martinrieder commented Jun 18, 2024

@tobiasfalk I fixed it by adding the line

os.environ['GVPRPATH'] = str(Path(__file__).parent)

@tobiasfalk
Copy link
Author

@martinrieder to where exactly?

@martinrieder
Copy link

Just before the call to gvpr in the if branch with find_executable in harness.py.

@tobiasfalk
Copy link
Author

@martinrieder could you test it again?

@tobiasfalk tobiasfalk changed the title Processing Jumpers and Loops Processing Jumpers and Loops, Plus Processing Wires inside the Table as Edges Jun 18, 2024
@tobiasfalk
Copy link
Author

tobiasfalk commented Jun 18, 2024

Some commits back, do not know exactly which one, I implemented #352 and it works without a problem for me

@martinrieder
Copy link

martinrieder commented Jun 19, 2024

Then I propose you edit the description of the PR to include this and the other issues from #369 (comment) as well.

@tobiasfalk
Copy link
Author

If everything is now Ok, then I would now wait for #251
Is there a Time frame for when it will be merged? Since the last update on it was in April(2 months ago)

@tobiasfalk
Copy link
Author

Oh yes I need to clean up the code a bit, I know that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants