Skip to content

replace ComponentSignals with a "port" abstraction - #570

Merged
superwhiskers merged 1 commit into
developfrom
sw/scratchpad
Sep 11, 2026
Merged

superwhiskers merged 1 commit into
developfrom
sw/scratchpad

Conversation

@superwhiskers

@superwhiskers superwhiskers commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Description

this pull request takes #481 and reworks it to have stronger compile-time checks, in addition to addressing review comments present on it.

i anticipate this pull request needing a lot of discussion before it may be merged. any comments are welcome.

Proposed changes

  • replace ComponentSignals with a "port" abstraction
    • maintain input "ports" separately from output "ports"
    • strengthen compile-time checks
  • rework enum handling
    • remove the need for enums to have a MAXIMUM or SIZE variant to have their size known
    • add a bunch of helpers abstracting over magic_enum functionality in anticipation of it being removable once we use c++26
  • standardize most, if not all enums so that they don't have unnecessary NONE variants and that their backing type is size_t

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.
  • The CHANGELOG.md has been updated to reflect the changes. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further comments

this pull request takes us further in the direction of compile-time enforcement. most of the changes relative to #481 were made to achieve this.

@lukelowry

lukelowry commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

On my end, this is amazing and helps with readability and clarity. The current implementation on develop is confusing and a bit restrictive because there are some models like BusFault which need to have signals/ports that do not correspond to an InternalVariable. I think this resolves, or at least partially resolves, that issue!

@nkoukpaizan nkoukpaizan added the enhancement New feature or request label Sep 7, 2026
@superwhiskers
superwhiskers force-pushed the sw/scratchpad branch 12 times, most recently from dfa7a95 to 7753126 Compare September 9, 2026 17:07
@superwhiskers
superwhiskers marked this pull request as ready for review September 9, 2026 17:25

@nkoukpaizan nkoukpaizan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few initial comments.
I like the changes to enum and the behavior of ports.

Comment thread examples/Consumer/GridKitConsumer/UtilitiesEnum.cpp Outdated
Comment thread examples/Consumer/GridKitConsumer/UtilitiesEnum.cpp Outdated
Comment thread GridKit/Model/PhasorDynamics/Exciter/SEXS-PTI/SexsPti.hpp Outdated
Comment thread GridKit/Model/PhasorDynamics/Controller/REECB/ReecbImpl.hpp
Comment thread GridKit/Solver/Dynamic/Ida.cpp Outdated
Comment thread GridKit/Model/PhasorDynamics/PortGroup.hpp
@superwhiskers
superwhiskers force-pushed the sw/scratchpad branch 2 times, most recently from ff761d9 to db8937a Compare September 10, 2026 17:46

@lukelowry lukelowry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Amazing, thank you for this work!

@nkoukpaizan nkoukpaizan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work!

@superwhiskers
superwhiskers merged commit 4846a70 into develop Sep 11, 2026
6 checks passed
@superwhiskers
superwhiskers deleted the sw/scratchpad branch September 11, 2026 19:12
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.

4 participants