Signal Bus Implementation by lukelowry · Pull Request #173 · ORNL/GridKit

lukelowry · 2025-07-04T17:42:12Z

Description

A preliminary BusSignal implementation that is a stepping point to a fully developed Bus Hierarchy. Supported signals can be specified by the Data structure input.

Closes Issue #154

An exciter model implementation and validated example (see branch IEEET1) are ready for PR once this is merged

Proposed changes

BusSignal Class inherits directly from BusBase, which still has virtual voltage/current functions. This is retained only because creating a seperate base grows the scope of this PR beyond what it was intended for. We can segment these models in the next PR.
Signal connections can be specified in (Model)Data struct.
SystemModel now auto implements governor.
Component class has helper functions that implement 'safe' versions of signal read/write/init to make model implementation easier.
Removes the GovernorBase class which was only added as a temporary solution.

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.

Further comments

The cmake of many filles had to be changed to include the governor program since I added it to the SystemModel. Is there a better way to have files that use SystemModel to not require all the same programs?

Future PR will need to create seperate bases: BusElectricalBase and BusSignalBase. This is a temporary solutions before these new bases are formed, because that task is large to undertake.

Forgive the large quantity of commits. I have a VM in addition to local WSL, so I am making frequent commits to the origin.

pelesh

In this first cut I suggest we implement signal bus as a standalone class inheriting directly from Model::Evaluator.

… [skip ci]

lukelowry · 2025-07-10T21:38:41Z

Okay, I have performed the following:

Rebased the branch
Refactor 'send' to 'write'
Use the new Port data structure to input the BusSignal index

After this merge, I will make the following Issues:

Electrical and Signal Bus Base Classes
Implement Governor with new Data Structure (it is currently using old struct)

Seeking further review @pelesh and @abirchfield . Hoping we unlock doors with this PR!

superwhiskers

some minor suggestions. i don't think i have any major comments.

superwhiskers · 2025-07-14T12:21:22Z

+        // if(is_initialized_)
+        //{
+        //   std::cout << "ERROR!";
+        // }
+        // else{
+        //   y_[0] = value;
+        //   is_initialized_ = true;
+        // }


why is this commented out? it appears to implement the intended behavior but instead we always set y_[0] to the value provided.

superwhiskers · 2025-07-14T12:27:00Z

+      /**
+       * @brief A one-time initialization function
+       * that can be called by any.
+       *
+       * @return state value of signal
+       */
+      void initial_value(ScalarT value) override


this method should be called initialValue to follow the naming conventions we have.

superwhiskers · 2025-07-14T12:33:02Z

+      /**
+       * @brief A helper function to easily init
+       * a signal value wiithout needing to check if initialized
+       *
+       * @param signal The signal bus object pointer
+       * @param value The value to send to the equality constraint
+       */
+      void safeInit(bus_type* signal, ScalarT value)
+      {
+        if (signal)
+        {
+          signal->initial_value(value);
+        }
+      }
+
+      /**
+       * @brief A helper function to easily init
+       * a signal value wiithout needing to check if initialized
+       *
+       * @param signal The signal bus object pointer
+       * @param value The value to send to the equality constraint
+       */
+      void safeWrite(bus_type* signal, ScalarT value)
+      {
+        if (signal)
+        {
+          signal->write(value);
+        }
+      }
+
+      /**
+       * @brief A helper function to easily read
+       * the state of the signal without checking nullptr
+       *
+       *
+       * @param signal The signal bus object pointer
+       * @param default_val The value to return if no signal
+       */
+      ScalarT safeRead(bus_type* signal, ScalarT default_val)
+      {
+        if (signal)
+        {
+          return signal->read();
+        }
+        else
+        {
+          return default_val;
+        }
+      }


i'm not sure these would belong on the Component class to be totally honest. they seem to exclusively be helper functions with no need to access any private data present on a Component; i think they would make more sense as standalone functions in a Utility.hpp file or something present within src/Model/PhasorDynamics.

superwhiskers · 2025-07-14T12:36:47Z

    {
      bus,             ///< Unique ID of the connecting bus
+      pmech_signal,    ///< Unique ID of the governor Pmech signal.
+      speed_signal,    ///< Unique ID of the speed signal of the machine.


we should also modify src/Model/PhasorDynamics/INPUT_FORMAT.md if we're adding new ports

superwhiskers · 2025-07-14T12:48:05Z

+          IdxT pmech_index = 0;
+          IdxT speed_index = 0;
+
+          bool has_bus = gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::bus);
+          bool has_pmech_signal = gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::pmech_signal);
+          bool has_speed_signal = gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::speed_signal);
+
+          if (has_bus && has_pmech_signal && has_speed_signal)
          {
+
            bus_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::bus);
+            pmech_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::pmech_signal);
+            speed_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::speed_signal);
+
+            auto* gen = new Genrou<ScalarT, IdxT>(
+              getBus(bus_index), 
+              getBus(pmech_index),
+              getBus(speed_index),
+              gendata);
+            addComponent(gen);
+          }
+          else{
+            bus_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::bus);
+
+            auto* gen = new Genrou<ScalarT, IdxT>(
+              getBus(bus_index), 
+              nullptr,
+              nullptr,
+              gendata);
+            addComponent(gen);
          }


Suggested change

IdxT pmech_index = 0;

IdxT speed_index = 0;

bool has_bus = gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::bus);

bool has_pmech_signal = gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::pmech_signal);

bool has_speed_signal = gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::speed_signal);

if (has_bus && has_pmech_signal && has_speed_signal)

{

bus_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::bus);

pmech_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::pmech_signal);

speed_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::speed_signal);

auto* gen = new Genrou<ScalarT, IdxT>(

getBus(bus_index),

getBus(pmech_index),

getBus(speed_index),

gendata);

addComponent(gen);

}

else{

bus_index = gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::bus);

auto* gen = new Genrou<ScalarT, IdxT>(

getBus(bus_index),

nullptr,

nullptr,

gendata);

addComponent(gen);

}

bus_type* bus = nullptr;

bus_type* pmech = nullptr;

bus_type* speed = nullptr;

if (gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::bus))

{

bus = getBus(gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::bus));

}

if (gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::pmech_signal))

{

pmech = getBus(gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::pmech_signal));

}

if (gendata.ports.contains(GenrouData<ScalarT, IdxT>::Ports::speed_signal))

{

speed = getBus(gendata.ports.at(GenrouData<ScalarT, IdxT>::Ports::speed_signal));

}

auto* gen = new Genrou<ScalarT, IdxT>(bus, pmech, speed, gendata);

addComponent(gen);

you can make this much cleaner

superwhiskers · 2025-07-14T12:50:59Z

+          /* Need to add signals to gendata class */
+          auto* gov = new Governor::Tgov1<ScalarT, IdxT>(
+              getBus(govdata.signal_pmech),
+              getBus(govdata.signal_speed),
+              govdata);


is it the case with Tgov1 that these signals are non-optional? (that sounds correct to me but i want to make sure as it is not documented in the input format)

pelesh · 2025-07-18T18:48:50Z

Closing this for now, but we can revisit this implementation approach later.

lukelowry added 30 commits July 3, 2025 07:19

init: test BusSignal compile [skip ci]

c2e857c

edit: source model class interaction with Signal [skip ci]

695154a

add: example TwoBusTgov1 new interface [skip ci]

5c16ba1

rem: old syntax [skip ci]

4ca42da

add: correct constructor [skip ci]

16cb12c

minor: type name [skip ci]

9e5311f

cmake: reflect changes in SystemModel [skip ci]

284b379

append: BusFactory [skip ci]

207d681

cmake: more gov link [skip ci]

52d22c3

edit: back support tengen [skip ci]

2090fdd

add: gov link to sys [skip ci]

4bcd715

edit: safe funcs pass by ref [skip ci]

533ecdb

syntax: readability [skip ci]

e707b12

read: const modifier preference [skip ci]

befccef

add: initial value function to BusBase [skip ci]

4264ea4

edit: default not init [skip ci]

8b06e21

rem: consnt, odd behavior [skip ci]

4388cd5

fix: resize bus vec [skip ci]

b4d1ef5

misc ref errors [skip ci]

db5283d

edit: init fixes [skip ci]

075608e

edit: constructor error [skip ci]

965fd6f

edit: type consistancny [skip ci]

f52f352

edit: busbase make change too [skip ci]

90ef74f

rem: initialization check [skip ci]

4cce9bf

misc edit [skip ci]

1aab2f2

edit: type hell [skip ci]

c564e0e

edit: init omega and change index [skip ci]

c598da8

edit: no funcs [skip ci]

ee375ac

edit: syntax [skip ci]

fcc3978

fix: rookie allocation mistake [skip ci]

f4d316c

superwhiskers mentioned this pull request Jul 9, 2025

Extend the json parser to add governor components Tgov1 as needed #176

Closed

pelesh requested changes Jul 10, 2025

View reviewed changes

Comment thread src/Model/PhasorDynamics/Bus/BusSignal.cpp

Comment thread src/Model/PhasorDynamics/Bus/BusSignal.hpp Outdated

Comment thread src/Model/PhasorDynamics/Bus/BusSignal.hpp Outdated

lukelowry and others added 19 commits July 10, 2025 14:50

Merge branch 'develop' into lukel/signal_bus_complete_dev

1f1aff2

Apply pre-commmit fixes

4429415

misc rebase fixes [skip ci]

9588581

genrou fixing after rebase [skip ci]

b67c6b6

change implementation to GenrouPort style [skip ci]

fabe772

correct genrou sys model [skip ci]

ae33c2b

correct the Tgov1 example [skip ci]

89163c2

update cmake from gov sys impl [skip ci]

ab4f7cc

readd: legacy support TenGen example [skip ci]

3a82364

comment out known magic_enum bug [skip ci]

2c63f68

Sys level handle genrou without gov [skip ci]

73b1001

edit: temporary sys model safe implementation [skip ci]

3b40ee6

minor: syntax error [skip ci]

ac4d8d0

incorrect boolean logic in sys model [skip ci]

298b545

edit: system model bug [skip ci]

c5495c2

debug: output of genrou [skip ci]

1213a9b

bug fixed: bus was not set in any example for new data struct [skip ci]

25b2c42

clean: pre rebase with modified implementation before further changes…

5afcb8e

… [skip ci]

rename: bussignal send to write and safeWrite [skip ci]

8e95f1a

lukelowry requested a review from pelesh July 10, 2025 21:42

pelesh requested a review from nkoukpaizan July 10, 2025 21:43

superwhiskers requested changes Jul 14, 2025

View reviewed changes

lukelowry mentioned this pull request Jul 15, 2025

Introduce signal node object to abstract connections with control devices #181

Merged

6 tasks

pelesh mentioned this pull request Jul 15, 2025

Add signal bus model to phasor dynamics family #154

Closed

pelesh closed this Jul 18, 2025

lukelowry deleted the lukel/signal_bus_complete_dev branch August 23, 2025 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signal Bus Implementation#173

Signal Bus Implementation#173
lukelowry wants to merge 91 commits intodevelopfrom
lukel/signal_bus_complete_dev

lukelowry commented Jul 4, 2025 •

edited

Loading

Uh oh!

pelesh left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukelowry commented Jul 10, 2025

Uh oh!

superwhiskers left a comment

Uh oh!

superwhiskers Jul 14, 2025

Uh oh!

superwhiskers Jul 14, 2025

Uh oh!

superwhiskers Jul 14, 2025

Uh oh!

superwhiskers Jul 14, 2025

Uh oh!

superwhiskers Jul 14, 2025

Uh oh!

superwhiskers Jul 14, 2025

Uh oh!

pelesh commented Jul 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lukelowry commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed changes

Checklist

Further comments

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukelowry commented Jul 10, 2025

Uh oh!

superwhiskers left a comment

Choose a reason for hiding this comment

Uh oh!

superwhiskers Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

superwhiskers Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

superwhiskers Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

superwhiskers Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

superwhiskers Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

superwhiskers Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

pelesh commented Jul 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukelowry commented Jul 4, 2025 •

edited

Loading