Fix infinite loop risk: Counter never incremented in a do-while loop

[dareumnam]

Pull request overview

• Fixes Counter never incremented in an do-while loop — infinite loop risk (HVACVariableRefrigerantFlow.cc) #11556
• In HVACVariableRefrigerantFlow.cc, the do-while loop to calculate Ncomp has a max iteration guard Counter >= 30, but Counter is initialized to 1 and never incremented inside the loop here.
  The adjacent similar block already does this correctly with Counter = Counter + 1, this block is just missing this kind of code.
• Used || (Counter++ >= 30) in both blocks.
• This also fixes a CppCheck knownConditionTrueFalse warning at HVACVariableRefrigerantFlow.cc:11383, where Counter >= 30 is flagged as always false.

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
• If adding/removing any output files (e.g., eplustbl.*)
  • Update ..\scripts\Epl-run.bat
  • Update ..\scripts\RunEPlus.bat
  • Update ..\src\EPLaunch\ MainModule.bas, epl-ui.frm, and epl.vbp (VersionComments)
  • Update ...github\workflows\energyplus.py

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

[rraustad]

That looks so nice.

[mitchute]

Never let the chance for a simple one-line change to turn into a full refactor 🙈 ... sorry @dareumnam my bad

I made a couple changes to these convergence loops to keep the convergence criteria and iteration counter separate. I also added a warning message in case they ever don't converge, though, I suspect that's likely a very low possibility given how old this is.

@dareumnam pointing it back to you for a review.

[github-actions]

⚠️ Regressions detected on ubuntu-24.04 for commit 9000498

Regression Summary

• ERR: 1

• View Results
• Download Regressions

[mitchute]

Interesting. It appears we do have a case where max iterations is being hit.

    ** Warning ** Check input. Pump nominal power or motor efficiency is set to 0, for pump=MAINS PRESSURE_UNIT1
+   ** Warning ** AirConditioner:VariableRefrigerantFlow "VRF HEAT PUMP":
+   **   ~~~   ** ...CalcVRFCondenser_FluidTCtrl: Iteration limit exceeded calculating heating mode compressor power, maximum iterations = 30
    ************* Testing Individual Branch Integrity

[github-actions]

⚠️ Regressions detected on macos-14 for commit 9000498

Regression Summary

• ERR: 1

• View Results
• Download Regressions

[dareumnam]

@mitchute Thanks for the refactor! Really appreciate you made the same pattern in both the cooling and heating modes.
I think the warning message you added is a nice touch. It's great that we can now actually see when a case hits the iteration limit instead of having it silently fall through.
I'm curious about the warnings whether this case was already hitting the 30-iteration limit on develop and just being handled silently, or whether something in the refactor changed the convergence behavior. If it would help, I can apply just the warning message on top of unmodified develop and check whether the limit was already being reached before.

[dareumnam]

Looking at develop more closely, the heating mode's loop (the one already increments Counter and checks Counter >= 30) just silently exits when the limit is reached. So this looks like the warning is not from this refactor. It's just surfacing a silent iteration-limit exit that was already happening on develop. Happy to patch just the warning onto unmodified develop and re-run the test to confirm.

[mitchute]

Thanks @dareumnam. Sure, feel free to confirm. It may be only happening once, or during something like warmup, too. It would be good to check to see whether there's anything special going on that would cause it.

[rraustad]

I am not sure this internal loop check is necessary (line 11375, I am having trouble commenting with my flaky internet connection)? Just update Ncomp_new and proceed. The loop will either iterate again or exit.

[mitchute]

I am not sure this internal loop check is necessary (line 11375, I am having trouble commenting with my flaky internet connection)? Just update Ncomp_new and proceed. The loop will either iterate again or exit.

Fair point

[nrel-bot-2c]

@dareumnam it has been 28 days since this pull request was last updated.

[nrel-bot-2c]

@dareumnam it has been 28 days since this pull request was last updated.