Skip to content

Conversation

kbenne
Copy link
Contributor

@kbenne kbenne commented Sep 30, 2025

Pull request overview

Description of the purpose of this PR

Pull Request Author

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@kbenne
Copy link
Contributor Author

kbenne commented Sep 30, 2025

Release the wolves! I'm prepared to be eaten alive for this PR. :)

@Myoldmopar
Copy link
Member

You'd better be ready to talk this over on Wednesday's call.

:)

  • I think some of these should be made into first-class API citizens so that they can be used in more general workflows
  • I see in at least one case you are throwing a runtime error. Why not in all the other places where there is an invalid index, for example?
  • SetZoneTemperature must be kept hidden away out of sight, that's all 🚨 energy-balance-breaking-danger 🚨 in the wrong hands

Copy link
Contributor

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

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

The main question I have about this is about the difference between this SpawnHelper and the public API (C, and Python) we already provide.

  • It seems that some functions are just duplicated here. I flagged a couple.
  • Some of these functions could potentially benefit the public API and should be moved there

I guess one argument one could make would be that the public API is C, not C++, but this is a non-argument. We could just have the functionality implemented in a C++ method, and the C one would just reinterpret_cast/cast and forward to the C++ one.

If you look at runtime.h, we have a C++ and C function for the callbacks

C++

ENERGYPLUSLIB_API void callbackBeginNewEnvironment(EnergyPlusState state, std::function<void(EnergyPlusState)> f);

C

extern "C" {
ENERGYPLUSLIB_API void callbackBeginNewEnvironment(EnergyPlusState state, void (*f)(EnergyPlusState));
}

Here is a dumb demo: https://compiler-explorer.com/z/8951PYh6o

Comment on lines +159 to +162
[[nodiscard]] int ActuatorHandle(EnergyPlus::EnergyPlusData &state,
const std::string &componentType,
const std::string &controlType,
const std::string &componentName);
Copy link
Contributor

Choose a reason for hiding this comment

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

ENERGYPLUSLIB_API int getActuatorHandle(EnergyPlusState state, const char *componentType, const char *controlType, const char *uniqueKey);

const std::string &controlType,
const std::string &componentName);

void SetActuatorValue(EnergyPlus::EnergyPlusData &state, int handle, Real64 value);
Copy link
Contributor

Choose a reason for hiding this comment

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

ENERGYPLUSLIB_API void setActuatorValue(EnergyPlusState state, int handle, Real64 value);


void SetActuatorValue(EnergyPlus::EnergyPlusData &state, int handle, Real64 value);

void ResetActuator(EnergyPlus::EnergyPlusData &state, int handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

ENERGYPLUSLIB_API void resetActuator(EnergyPlusState state, int handle);

@JasonGlazer
Copy link
Contributor

I would like to see just an extension of the API to cover the additional functions that Spawn needs, rather than creating a special interface.

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

Successfully merging this pull request may close these issues.

5 participants