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

[bug] Unconnected components are ignored by v0.4-dev #328

Closed
kvid opened this issue Dec 29, 2023 · 6 comments
Closed

[bug] Unconnected components are ignored by v0.4-dev #328

kvid opened this issue Dec 29, 2023 · 6 comments

Comments

@kvid
Copy link
Collaborator

kvid commented Dec 29, 2023

The current dev branch (after merging in multiple PRs this summer) is no longer able to document unconnected connectors or cables - as has been possible with all releases (at least v0.2 and later).

This is a minimal example to document a single connector without any connections:

connectors:
  X1:
    pinlabels: [GND, VCC]

With the current dev branch, this results in an empty diagram - which is not very useful!

Why is the algorithm that generates the diagram rewritten to only add components that are referred to from connections? That makes it impossible to document e.g. a loop-back connector, which is not an unusual use case. See also the slightly related #217.

The PR #251 on top of dev does not seem to help with this issue.

@kvid
Copy link
Collaborator Author

kvid commented Dec 30, 2023

If this new fuctionality is intended as a feature, then it might be a solution to #294. However, in that case, it should not be the default option, IMHO, because I find it counterintuitive unless deliberately activating it as an optional feature, e.g. by setting:
options.hide_unconnected_nodes=true

@formatc1702
Copy link
Collaborator

formatc1702 commented Feb 25, 2024

Thanks for bringing this up.

The easy fix with the current code, is to define a connection set, with only that connector included:

connectors:
  X1:
    pinlabels: [GND, VCC]

connections:
  - - X1

The question is where harness.add_connector() is called, which currently only happens inside for connection_set in connection_sets: after resolving designators etc., instead of during initial YAML parsing.

I am OK with restoring the old behavior, feel free to submit a PR and adding the option as you suggest. Bear in mind the same would apply to unconnected cables as well. In the end, it is a matter of preference. [Edit: see comment below]
The alternative is to add the above fix (with a one-line connection set) to the documentation, which I would be fine with as well, since adding more and more options makes using the tool more unwieldy, IMHO.

@sb424dat
Copy link

I you want I can add this to the documentation and try some test cases on my machine!

@formatc1702
Copy link
Collaborator

formatc1702 commented Apr 15, 2024

I am coming back to this issue, and I can now offer a better explanation.
TL;DR: it's a feature, not a bug; but I agree it was an unexpected side effect of that feature.

Previously, connectors were immediately generated unless they were explicitly meant for autogeneration, then added to the harness, and thus rendered. Cables did not have autogeneration capabilities.

if sec == 'connectors':
if not attribs.get('autogenerate', False):
harness.add_connector(name=key, **attribs)
elif sec == 'cables':
harness.add_cable(name=key, **attribs)

However, after implementing the new autogeneration syntax proposed in #184 (comment), implemented in #186 and explained in the new syntax description* , the autogenerate property is no longer used, and the decision whether a connector (or cable!) should be instantiated as "itself" or as a new autogenerated instance, is only made by resolving the designators called in the connection set

# resolve all designators
for index, entry in enumerate(connection_set):
if isinstance(entry, list):
for subindex, item in enumerate(entry):
template, designator = resolve_designator(
item, template_separator_char
)
connection_set[index][subindex] = designator
elif isinstance(entry, dict):
key = list(entry.keys())[0]
template, designator = resolve_designator(key, template_separator_char)
value = entry[key]
connection_set[index] = {designator: value}
else:
pass # string entries have been expanded in previous step

and only afterwards is the harness actually populated populating with them.

if designator in harness.connectors: # existing connector instance
check_type(designator, template, "connector")
elif template in template_connectors.keys():
# generate new connector instance from template
check_type(designator, template, "connector")
harness.add_connector(
name=designator, **template_connectors[template]
)

Since I strongly believe the new autogeneration syntax is more robust and flexible**, I will preserve the current behavior and instead amend the syntax description with a note on how to force rendering components that are not connected by referencing them in a one-line connection set as described above.

connections:
  - - X1

* There are still some mentions of the autogenerate: true attribute in the dev branch's syntax description. I will reword these as well.

** For example, the new syntax allows to generate multiple complex but identical components by using autogeneration, without resorting to YAML templates, and while still allowing the user to assign designators for every instance.

connectors:
  X_template:
    # ... complex connector description

connections:
  -
    - X_template.X1: [...]
    - ...
    - X_template.X2: [...]
  -
    - X_template.X3: [...]
    ...

@kvid
Copy link
Collaborator Author

kvid commented Apr 16, 2024

@formatc1702 - thank you for the detailed explanation above. I now agree that the advantages with the new algorithm are probably greater than the disadvantage about breaking the old way to document an unconnected connector. The disadvantage can be reduced by describing such use cases in the documentation and maybe include a proper example. How can we best give the user an explanation about the reason when generating an empty diagram? There might also be other possible reasons for this than the use case described in this issue.

@formatc1702
Copy link
Collaborator

See the updated syntax description in the unreleased interim v0.4 for a (IMHO) concise and simple explanation and solution.

@kvid kvid closed this as completed May 12, 2024
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

No branches or pull requests

3 participants