Skip to content

Groups support for entity and component ports, and architecture signal declarations #99

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

amb5l
Copy link

@amb5l amb5l commented Jun 14, 2025

As discussed in issue 97.

@Paebbels Paebbels changed the base branch from main to dev June 14, 2025 22:23
@Paebbels Paebbels self-requested a review as a code owner June 14, 2025 22:24
@Paebbels
Copy link
Member

Thanks for providing the PR, so I can review the proposed changes. So far I can see the solution is completely different from the first proposal.

I see some drawbacks... give me some time to write a detailed explanation and justification.

So far, I'm missing essential features of a PortGroup:

  • Name
  • Description / Documentation
  • Parent reference

Other findings:

  • A component is not a design unit, thus the mixin name needs to be different.
    The TODO in my code mentions 2 separate mixins for ports and generics, so it could be reused. Solving this TODO is not required for the port groups feature.
  • The proposed implementation covers ports and signals, but what about generics, parameters, variables, constants and files.
    Why should we care for signal groups?
    Isn't it more important what objects (constant, variable, signal file) are grouped?
    I see a meaning in groups for interfaces, but not for objects as these are interleaved e.g. also with types and subprograms.

@Paebbels Paebbels added the enhancement New feature or request label Jun 14, 2025
@amb5l
Copy link
Author

amb5l commented Jun 14, 2025

Thanks for providing the PR, so I can review the proposed changes. So far I can see the solution is completely different from the first proposal.

The current approach results from further study of your codebase but I'm happy to change it as much as needed to align with your preferences.

I see some drawbacks... give me some time to write a detailed explanation and justification.

So far, I'm missing essential features of a PortGroup:

  • Name
  • Description / Documentation
  • Parent reference

When parsing source code and creating pyVHDLModel objects, I expect groups to be inferred from source proximity, and the name to extracted from comments, or meta-comments. The name is the key of the PortGroups dict. I can see that it may make sense for a group to have a description, in which case we need a "PortGroup" class - subclassed from a "Group" class - a list with a description.

I assume that since the elements of the list will be references to the (flat) ports/signals/etc objects, and those objects have a parent, there is no need to have a parent attribute for a group, or a dict of groups? But it would be easy to add this.

Other findings:

  • A component is not a design unit, thus the mixin name needs to be different.

No problem. Do you have a suggestion?

The TODO in my code mentions 2 separate mixins for ports and generics, so it could be reused. Solving this TODO is not required for the port groups feature.

I'm happy to change this as required.

  • The proposed implementation covers ports and signals, but what about generics, parameters, variables, constants and files.

I expect that the grouping functionality will expand to include these, if we get the foundations right.

Why should we care for signal groups?

My code often has groups of signals associated with different areas of functionality in the architecture body. For example, different clock domains, or different pipeline stages.

Isn't it more important what objects (constant, variable, signal file) are grouped?

If so then let's create grouping functionality for these also.

I see a meaning in groups for interfaces, but not for objects as these are interleaved e.g. also with types and subprograms.

See my comment about signal groups above.

Let me know how you would like me to proceed?

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

Successfully merging this pull request may close these issues.

2 participants