Fix #10944: unused EMS actuator warning never fires#11525
Open
brianlball wants to merge 2 commits intodevelopfrom
Open
Fix #10944: unused EMS actuator warning never fires#11525brianlball wants to merge 2 commits intodevelopfrom
brianlball wants to merge 2 commits intodevelopfrom
Conversation
IDF with actuator A1 + program `Set Al = 2` (typo). Expect err stream contains "Unused EMS Actuator detected A1" after checkForUnusedActuatorsAtEnd; stream is empty on develop. Fails on current source - verifies the gap.
1179f5f to
2469735
Compare
Prior attempt flipped Null sentinel initialized=false, which broke right-hand-side reads of actuators that hadn't been SET yet but had subsystem-provided default values (regressed EMSUserDefined5ZoneAirCooled, PythonPluginUserDefined5ZoneAirCooled). Value.initialized has dual meaning - both "safe to read as right-hand-side operand" and "was SET by Erl" - so co-opting it for the warning check was wrong. Per Myoldmopar's suggestion in the issue, add a dedicated `wasActuated` bool on ActuatorUsedType. Set it in the EMSManager.cc post-ManageEMS actuator-update loop (else branch where Type != Null, i.e. user SET a concrete value). Check it in checkForUnusedActuatorsAtEnd instead of Value.initialized. Leaves Null sentinel and Value.initialized semantics untouched.
2469735 to
e91fc19
Compare
|
Contributor
Author
Contributor
Author
Regression diffs — all expected, warning working as intendedThe regression tool flagged 13 files with diffs. All 13 are Pattern breakdown
Sample hit (EMSDemandManager_LargeOffice)ConclusionKeeping the warnings as-is. Each flagged actuator is a genuine declared-but-unused case. If the corpus authors meant these as placeholders, the warning now documents that; if they're dead, it flags them for cleanup. Either way, this is the fix working on real-world data. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
checkForUnusedActuatorsAtEndcheckedErlVariable.Value.initialized, but that flag has dual meaning: "safe to read as right-hand-side operand" AND "was SET by Erl program". Actuators getinitialized=trueearly (inherited from the Null sentinel at registration) so the first meaning applies cleanly - but the guard read it as the second meaning and never tripped.wasActuatedbool onActuatorUsedType. Set it only when the user's Erl program assigns a concrete value to the actuator (Type != Null in the post-ManageEMS actuator-update loop atEMSManager.cc:345). Check that flag in the end-of-sim guard. LeavesValue.initializedand the Null sentinel alone.Why this approach (history)
First attempt flipped the Null sentinel's
initializedflag tofalse. That seemed surgical but broke right-hand-side reads of actuators that hadn't been SET yet but had subsystem-provided defaults - the expression evaluator atRuntimeLanguageProcessor.cc:1800reads the same flag to gate "is this variable safe to use in an expression." CI caught it via regressions inintegration.EMSUserDefined5ZoneAirCooledandintegration.PythonPluginUserDefined5ZoneAirCooled. The current fix matches Myoldmopar's original suggestion in the issue ("a simple flag on each actuator").Call graph - issue & fix
Files changed
src/EnergyPlus/DataRuntimeLanguage.hh- addwasActuatedfield onActuatorUsedTypesrc/EnergyPlus/EMSManager.cc- set the flag in the actuator-update else branch; check it in the end-of-sim guardtst/EnergyPlus/unit/EMSManager.unit.cc- newUnusedActuatorWarningtestTest plan
EnergyPlusFixture.UnusedActuatorWarningasserts warning fires for unused actuatorintegration.EMSUserDefined5ZoneAirCooled: passes locally (was the CI regression in the prior attempt)integration.EMS*tests: passscripts/check_gcc_warnings.py src/EnergyPlus/EMSManager.cc: clean