wireviz / WireViz

Easily document cables and wiring harnesses.
GNU General Public License v3.0
4.41k stars 225 forks source link

[feature] connector pin attributes #291

Open T0jan opened 2 years ago

T0jan commented 2 years ago

Hello,

I usually use additional parameters in my harness plans like the signal direction or the voltage levels as additional layers of security. One could add them to the pinlabels ofc but I would like to see them in an additional column for each connector. If it is not already implemented and I missed it somewhere I would suggest a solution with optional attributes for the pinout definition:

connectors:
  J101:
    pinlabels: [+5V, +5V, GND, GND, RX+, RX-, TX+, TX-, SDA, SCL]
    pinattributes1: [PWR, PWR, PWR-RET, PWR-RET, IN, IN, OUT, OUT, IO, OUT]
    pinattributes2: [5V, 5V, GND, GND, RS422, RS422, RS422, RS422, 3V3, 3V3]

I think giving the option to add two or three individual attribute fields should cover the most cases in which this could be useful.

kvid commented 1 year ago

Thank you @T0jan for this feature suggestion. I'm sorry you had to wait so long without any feed-back. I'm not against your suggestion, but I just want to present a possible/optional alternative syntax to specify the same attributes:

connectors:
  J101:
    pinlabels:
      - [+5V, PWR,     5V]
      - [+5V, PWR,     5V]
      - [GND, PWR-RET, GND]
      - [GND, PWR-RET, GND]
      - [RX+, IN,      RS422]
      - [RX-, IN,      RS422]
      - [TX+, OUT,     RS422]
      - [TX-, OUT,     RS422]
      - [SDA, IO,      3V3]
      - [SCL, OUT,     3V3]
T0jan commented 1 year ago

@kvid All good, I am glad too see this is not dead. I like your suggestion much more then my initial idea as it is much better readable. I think my idea was based on how it was done for the other properties like colors or pin numbers so I wanted to keep it similar but I think your solution is better to work with for the user.

Would there be a limit on how many custom columns a user can add like this?

kvid commented 1 year ago

@T0jan wrote:

@kvid All good, I am glad too see this is not dead.

Don't worry! Open issues are not dead, but sometimes it might take a while before someone reply, and even after a reply with suggestions, it doesn't always mean that someone will implement such suggestions soon. However, if you (or someone else) are motivated to try some Python coding yourself, feel free to make a note of it here and then submit a PR. It's often useful to discuss alternative functionalities and visual appearances in this open issue first, before diving into the coding job, but problems or design choices that appear during coding might also be discussed here (or in the PR when that is created).

I like your suggestion much more then my initial idea as it is much better readable. I think my idea was based on how it was done for the other properties like colors or pin numbers so I wanted to keep it similar but I think your solution is better to work with for the user.

The same attribute can be able to take simple values for simple use cases, and more complex values when needed for complex use cases. I think this is a convenient way to expand the functionality without breaking compatibility with the original simple use case.

Would there be a limit on how many custom columns a user can add like this?

I haven't tried coding any of this, so I don't know for sure, but in principle, it doesn't need to be such a limit.

martinrieder commented 4 months ago

@kvid wrote:

connectors:
  J101:
    pinlabels:
      - [+5V, PWR,     5V]
      - [+5V, PWR,     5V]
      - [GND, PWR-RET, GND]
      - [GND, PWR-RET, GND]
      - [RX+, IN,      RS422]
      - [RX-, IN,      RS422]
      - [TX+, OUT,     RS422]
      - [TX-, OUT,     RS422]
      - [SDA, IO,      3V3]
      - [SCL, OUT,     3V3]

I've been thinking about a similar approach lately. I had some issues that my pinlabels did not match with the alphanumeric designators defined in pins. In order to make them match, I wished there was a way to define them together in a table similar to yours. I find that transposed definition much better to handle, also for copying it from my spreadsheet application.

PROPOSAL

connectors:
  J101:
    pintable:
      - [pins, pinlabels, pincolors]
      - [A, +5V, RD]
      - [B, 0V, BK]
      - [C, CANH, YE] 
      - [D, CANL, GN]

Note that the first line is a header that would define a reference to any possible attribute that accepts a list. Being able to load these as a table i.e. from CSV provides a big usability improvement. Defining the keys of each column is important, so they would not need to be reordered before inserting.

PS: the same principle applies to wires, which could then be defined through wiretable.

kvid commented 4 months ago

@martinrieder - I'm not against your approach. The syntax is very similar to my suggestion from last year above, but the functionality of your approach is perhaps closer to the suggestion in #279 that also is an optional alternative syntax for existing list attributes. The different approaches don't exclude each other, though. Any conflicts between specifying different values for the same attribute via the different approaches should raise an exception.

Comments about your syntax:

martinrieder commented 4 months ago

The syntax is very similar to my suggestion from last year above, but the functionality of your approach is perhaps closer to the suggestion in #279 that also is an optional alternative syntax for existing list attributes. The different approaches don't exclude each other, though.

I agree that my suggestion is not exclusive to yours in #279. It contains reference to independent approaches though, where each would work, though I suggest some slight modifications below.

What seems totally evident to me is the fact that it handles sub-components:

The table would only describe them at this level. Separate components require separate tables.

While the issue description suggests some automatic ordering of wires, @Halfwalker suggests an ordered list for pins. This effectively represents a sparse table that is compatible with my proposal:

Connectors:
  A4:
    pinlist:  # or simply keep 'pins' ? 
      - name: A 
        label: AC lock sensor
        color: GNYE
      - name: B  
        color: VTWH
      - name: C
        label: NC
      - # auto-assign to name: 4
        label: AC magnetic clutch
        color: BU

@Tyler-Ward suggested a wire definition, for which the key-value assignment represents an unordered list. In effect, this proposes the same as #275. It is different to a table with defined order, which might need some smart algorithm to do the sorting.

templates:
  - &blueWire
    part_number: 12345
    color: BU
cables:
  W1:
    wireinfo:
      A: # name could also be numbered 
        part_number: 12344
        color: BK
      B: <<: &blueWire
      C: <<: &blueWire
      4: # no sorting here
        part_number: 12343
        color: WH

@formatc1702 suggested a transposed version of the table. There is no real difference to my suggestion. Both should be allowed.

W1:
  colors: [BK,    BU,    BU,    RD   ]
  gauge:  [0.25,  0.25,  0.5,   0.5  ]
  mpn:    [12344, 12345, 12346, 12343]

Coming back to the suggested table syntax, which makes definitions a bit more compact than the previous example:

Comments about your syntax:

  • To support the original request of this issue, would you suggest pinattributes1 and pinattributes2 as headers, or would you like to support any custom header (or any with e.g. a userattr. prefix to avoid conflicts with future attribute names) as user defined labels?

Yes, this should accept any attribute that is presented as a list, even future custom attributes.

  • Instead of pintable and wiretable, I suggest a common name, e.g. attr_table or attributes_table.

Maybe simply name it attributes or table. I am open for suggestions, though I prefer short names without abbreviation.

  • Do you also consider optionally accepting something like attr_table: X1_attributes.tsv (or from a CSV file) as a future expansion of your approach?

Not sure about this. It seems to be a separate issue to me. This file would definitely require the same header in the first row... It could possibly be included through Jinja as an advanced feature:

Any conflicts between specifying different values for the same attribute via the different approaches should raise an exception.

I prefer warnings or error messages over exceptions. The implementation of additional components needs to be discussed with respect to #224.

martinrieder commented 4 months ago

@kvid wrote

  • To support the original request of this issue, would you suggest pinattributes1 and pinattributes2 as headers, or would you like to support any custom header (or any with e.g. a userattr. prefix to avoid conflicts with future attribute names) as user defined labels?

Some additional thought that might lead to a separate feature request: Would passing these user attributes to Jinja be an option, so users could use them in templates?

If so, it would need to be added to the output stage, similar to PR #382 by @liambeguin.

kvid commented 4 months ago

@martinrieder wrote:

[...]

  • Instead of pintable and wiretable, I suggest a common name, e.g. attr_table or attributes_table.

Maybe simply name it attributes or table. I am open for suggestions, though I prefer short names without abbreviation.

Of these two alternatives, I prefer attributes because table doesn't say anything about what to specify.

  • Do you also consider optionally accepting something like attr_table: X1_attributes.tsv (or from a CSV file) as a future expansion of your approach?

Not sure about this. It seems to be a separate issue to me. This file would definitely require the same header in the first row... It could possibly be included through Jinja as an advanced feature:

Then we forget about this. It's not a feature I want. I just asked if you considered something like this because you wrote "Being able to load these as a table i.e. from CSV provides a big usability improvement."

Any conflicts between specifying different values for the same attribute via the different approaches should raise an exception.

I prefer warnings or error messages over exceptions.

I agree about preferring warnings when it makes sense to ignore the unexpected event and continue, but what do you mean by preferring "error messages over exceptions"?

All exceptions provide an error message describing the problem. In some cases it might make sense to catch some low level exceptions and raise a higher level exception with more context information, but in most such cases it's useful to also include the low level error message as the root cause.

The implementation of additional components needs to be discussed with respect to #224.

How does this issue affects anything about additional components?

martinrieder commented 4 months ago
  • Do you also consider optionally accepting something like attr_table: X1_attributes.tsv (or from a CSV file) as a future expansion of your approach?

Not sure about this. It seems to be a separate issue to me. This file would definitely require the same header in the first row... It could possibly be included through Jinja as an advanced feature:

Then we forget about this. It's not a feature I want. I just asked if you considered something like this because you wrote "Being able to load these as a table i.e. from CSV provides a big usability improvement."

I meant to say that copying data from spreadsheets is a common task for most users. It usually requires some manual processing to assign the right columns.

I consider this nice to have, but I also see that it adds some degree of complexity. We should keep it in mind for the future, maybe only processing CSV strings as a PoC, before adding the file handling.

The proposal is a separate issue to me, but related and seems absolutely feasible.

I prefer warnings or error messages over exceptions.

I agree about preferring warnings when it makes sense to ignore the unexpected event and continue, but what do you mean by preferring "error messages over exceptions"?

By error message, I mean printing only a single line. You might call it a handled exception that limits the output to the highest level only. FileNotFoundError might be a good example that does not require any traceback to the lowest level.

The implementation of additional components needs to be discussed with respect to #224.

How does this issue affects anything about additional components?

These components would usually be related to the ones defined in the table. They might either be added to the same table or defined in a separate table. Think of a connector having different gauges for individual pins. It might have additional wire seals for each pin, but only a single connector housing.

I see the following options for those additional components on a connector (and similar ones for cables and wires):

Having separate tables seems to be less complex. Both could be linked using reference designators as discussed in #224. The additional component quantity could either be defined explicitly or implicitly determined by counting the number of references.

Jachimo commented 3 weeks ago

Not sure what the current status of this feature is, but I'd just like to note that the ability to explicitly specify the name/label for each pin explicitly (as a dictionary or some other similar structure) would be great when working on large connectors such as MIL-38999s.

The 38999 series connectors do not use pin numbers; they use pin letters, and then each pin typically has a name associated with it as well. And they can have a lot of pins, to the point where specifying the pin name / pin label positionally via two separate lists is... a bit unwieldy.

E.g. imagine something like this (which is not that complex as these things go): image

It would be nice to be able to write something like:

connectors:
  J24:
    pins:
      - name: A
        label: GPS
        type: M39029/103-559
      - name: Z
        label: GPS_VCC
        type: M39029/1-100
      - name: a
        label: GPS_GND
        type: M39029/1-100

IMO that is a lot easier to read and much less error-prone than having two positional lists that need to be kept in sync if a pin is added / removed.

kvid commented 3 weeks ago

@Jachimo wrote:

Not sure what the current status of this feature is,

It's one of many feature requests that are still open for discussions, but not yet have any suggested implementation.

but I'd just like to note that the ability to explicitly specify the name/label for each pin explicitly (as a dictionary or some other similar structure) would be great when working on large connectors such as MIL-38999s.

I agree, and personally, I argue for accepting alternative ways to specify such information, because different use cases might have different needs.

[...] It would be nice to be able to write something like:

connectors:
  J24:
    pins:
      - name: A
        label: GPS
        type: M39029/103-559
      - name: Z
        label: GPS_VCC
        type: M39029/1-100
      - name: a
        label: GPS_GND
        type: M39029/1-100

I agree this might be useful, but it seems like a duplicate of what was already suggested in #275 and then split out to #279, so please add your comments to that issue. The only detail that seems to differ, is your name attribute instead of pin - and that can be your first contribution/suggestion to #279.