I believe CurrentTime is the time at the end of the current time step. So CurrentTime - TimeStepZone would be the beginning. What I am not sure of in code is if SysTimeElapsed = TimeStepZone when the simulation has not downshifted. I seem to recall in the watch window that they are NOT and SysTimeElapsed only gets updated when the simulation downshifts. I'm not exactly sure when the time stamp is incorrect but I do not recall seeing 24:00 - 24:10, well, ever. I would look at this again with this in mind. Maybe this is strictly an EMS thing?

@rraustad I had wondered initially if it was just an EMS thing. Then I had vague recollections of times when the error message said one thing but in the debugger the problem happened before when the error message said. I also considered a solution where I just created a new timestamp routine for the EMS only. I could certainly still do that and it wouldn't impact any of the error messages that already use this timestamp. That would fix the issue that is clearly in the EDD file for this file that uses EMS. I'll walk back the initial solution and implement a new EMS only routine instead.

I think I would add these variables to the watch window and step into EMS code where an EDD statement is posted and compare the time variables.

Ok, so I'm going through the debugger and here is what I am finding when I step through the EMS code. I have stopped at a point where the values of variables are as follows:

HourOfDay = 8
TimeStep = 3
TimeStepZone = 1/6
CurrentTime = 7.5
SysTimeElapsed = 1/6
TimeStepSys = 1/6

These values agree with the values of these parameters that have been set by SimulationManager (looked at these in ManageHeatBalance), so they have not changed from the main simulation moving into the EMS.

In CreateSysTimeIntervalString, here are the values that I get using my revised code:

ActualTimeE = 7.5
ActualTimeS = 7.333...

I believe that those values calculated are correct. With six time steps per hour (10 minutes each) and in the 8th hour of the day, the third zone time step should be from 7:20-7:30.

Going back to your original question about what happens when there is no downshifting, the file that I am working on seems not to be downshifting much if at all. So, when things get to whatever point that EMS is actually reporting, SysTimeElapsed has been updated from the reinitialized value of zero (HVACManager.cc: ManageHVAC line 205) to the value at the end of system time step (HVACManager.cc: ManagerHVAC line 564). The downshifting happens after the first predict but before there is actually any progress forward.

Given all of this (if any of that made sense or actually answered anything?), here is what I think I can say:

• I agree that "CurrentTime is the time at the end of the current time step".
• I also agree that "CurrentTime - TimeStepZone would be the beginning" when there is no downshifting or when discussing the zone time step.
• From what I have seen SysTimeElapsed = TimeStepZone ONLY at the end of all of the system time steps (whether one or more than one because of downshifting).
• SysTimeElapsed is updated at the end of a system time step.

Because SysTimeElapsed is updated at the end of the system time step, the code in develop is correct for error messages that use CreateSysTimeIntervalString because SysTimeElapsed has not been updated. So, I agree that my initial solution would "break" things for the error messages and that is not what we want for everything.

For EMS, the situation is a bit more nuanced. When SysTimeElapsed is not equal to TimeStepZone, this means that either we are still stepping through system time steps or something is reporting before HVAC simulates. If SysTimeElapsed (see data earlier in this post) equals TimeStepZone, there is reporting happening after the system is done with the last time step (or the only time step in case of downshifting). Because SysTimeElapsed has already been updated, the CreateSysTimeIntervalString routine has to switch to what I proposed as a solution--just only for this case.

I think that this would solve the issue. Does any of this make sense or am I just making stuff up on the fly? In all honesty, just writing this out has been a bit confusing. However, it has been a good exercise and maybe gets to the real solution.

If you agree that this is a correct approach/analysis, I will create a new subroutine that checks the SysTimeElapsed in comparison with TimeStepZone. When they are not equal, CreateSysTimeIntervalString from develop will be used. When they are equal, it will follow logic from what I proposed as the solution in the previous commit. Thoughts?

Yes that does make sense. What doesn't make sense is a new subroutine. You can implement that logic right here in CreateSysTimeIntervalString. The odd thing is that I have looked at time stamps in the csv from detailed reporting and they looked correct. So I'm still not sure what is causing this issue.

Real64 ActualTimeS, ActualTimeE;
if (SysTimeElapsed == 0.0) {
    ActualTimeE = state.dataGlobal->CurrentTime;
    ActualTimeS = ActualTimeE - state.dataGlobal->TimeStepZone;
} else {
    ActualTimeE = state.dataGlobal->CurrentTime - state.dataGlobal->TimeStepZone + SysTimeElapsed;
    ActualTimeS = ActualTimeE - TimeStepSys;
}

Since "I have looked at time stamps in the csv from detailed reporting and they looked correct", maybe if you move HVACManager line 564 up above the call to ManageEMS that would fix the edd reporting issue? There is another call to ManageEMS at line 445 so not sure how that call would or should report the time stamp.

    EMSManager::ManageEMS(
        state, EMSManager::EMSCallFrom::EndSystemTimestepAfterHVACReporting, anyEMSRan, ObjexxFCL::Optional_int_const()); // EMS calling point
    // UPDATE SYSTEM CLOCKS
    state.dataHVACGlobal->SysTimeElapsed += state.dataHVACGlobal->TimeStepSys;

Yes, agree--a separate subroutine is definitely not needed. Just an if-then block in the existing subroutine--that makes much more sense. I'll work on that next.

The moving of the EMS call that you mentioned was definitely an intriguing idea. I went back to develop code and made a switch of those two lines you highlighted. Unfortunately, that did not resolve the EDD issue. I have concerns about shifting the SysTimeElapsed up any further because it will likely open up a can of worms that could lead to a lot of diffs.

@rraustad Ok, I have made changes very similar to what you suggested above (an additional condition on the if block). Updated the new unit test (added one more test) and made necessary changes a couple of existing unit tests where error messages changed as a result of the updated code. I'm guessing/hoping this will come back "green".

To be honest, I don't trust a unit test to get this right since the time variables are getting set differently than they would in code. Use a detailed input file and check both csv and rdd times to know when this is solid.

I see your point. In a unit test (though these are still very important), it doesn't necessarily have the time variables set as they would be while running through the code. While I'm not thrilled that I've ended up sinking more time than I anticipated into this, I feel like this is actually going to fix what the user saw and maybe something that has been a "hidden" mistake.

I did wonder if using only the == 0 check would do the trick (because of timing of when these time variables get set/reset) so I am not surprised something different was needed. I checked the first err diff and the time changes look good. Then I saw this on the 3rd diff where time changed by 1 minute and 2 minutes when I would expect both to change by 2 minutes. Maybe that's a rounding issue (I wonder what the csv shows when using detailed?). Also, as you described in your investigation, the only time TimeStepZone and SysTimeElapsed are equal is on the last system time step where I would expect SysTimeElapsed to be greater than 0 (because it's equal to TimeStepZone and therefore not = 0). OH, so this is wrong, on that last system time step (your || conditional) ActualTimeE would be the current time (correct) and ActualTimeS would be - TimeStepZone which is incorrect (it should be - TimeStepSys). I think you should critique the time stamps in the edd and err file diffs to see if anything stands out. It's possible that this needs and if, else if, else to cover all bases (or maybe my original suggest was correct?). With the calling order of setting/resetting the time variables and multiple calls to EMS this is turning out to be a little trickier than first thought. Set the first diff file to detailed and review the csv and rdd time stamps while you test different strategies. You'll get there.

-   info = WASHINGTON DC DULLES INTL AR ANN CLG 2% CONDNS WB=>MDB, 07/21 06:58 - 07:00                
+   info = WASHINGTON DC DULLES INTL AR ANN CLG 2% CONDNS WB=>MDB, 07/21 06:57 - 06:58              

Yeah, this whole thing has been pretty subtle/tricky--a lot more so than I thought. I've looked at this again and I'm wondering if the problem with my last commit is two-fold. First, I agree with the need for an else if condition with a correction to - TimeStepSys when TimeStepZone and SysTimeElapsed are equal. If the system downshifted, then it would be coming to the end with smaller system time steps and the time stamp needs to reflect the downshifting. Second, the else condition is not the same as in develop which I think caused the diffs in the err files. My theory (it's early and I haven't had coffee yet so don't hold me to it) is that if I fix both of these problems that the EDD files will have diffs (which is correct because they are currently clearly wrong--see the 24:10 times) and that the ERR diffs will go away.

Well, this is "exciting"! I made those two fixes and compared the error file that gets generated from _SmallOffice_Dulles.idf with develop and the one that gets generated with this branch. I was expecting no differences, but I saw that there actually was one that wasn't flagged in the last full test suite run. In develop, the error message had 20:00 - 20:10 while in this branch it is producing 19:50 - 20:00. To look at this further, in develop, I set a breakpoint where this psychrometric error was being produced and ran to that point. Here are the values of the time keeper variables:

CurrentTime = 20
TimeStepZone = 1/6
HourOfDay = 20
TimeStep = 6
SysTimeElapsed = 1/6
TimeStepSys = 1/6

I interpret this as meaning we are in the 20th hour of the day, in the last zone time step of the hour, and the system is running at the zone time step (no downshifting). Which means that we should be talking about 19:50 - 20:00 not 20:00 - 20:10. Thus, I think develop is actually wrong here, and this is the same issue that the EMS stuff is having. It's just that you wouldn't know it until you actually went into the debugger. Maybe my weird, vague recollection of "when the error message said one thing but in the debugger the problem happened before when the error message said" was actually a symptom of this problem (naturally I previously dismissed this as just a weird debugger thing...) and now we are on the right track? I'm going to make another commit and see what ci comes up with.

That's interesting, TimeStepSys = 1/6 is the same as TimeStepZone but SysTimeElapsed = 1/6 and not 0. I, frankly, don't know what that means. Carry on.

CurrentTime = 10.6 or 10:36 and TimeStepZone = 0.2 or 12 minutes, so at zone level this is time 10:24 - 10:36.
TimeStepSys = 0.05 or 3 minutes and SysTimeElapsed = 0.1 or 6 minutes. So I would expect this to be the 2nd of 4 system time steps. With those 4 time periods being: 10:24 - 10:27, 10:27 - 10:30, 10:30 - 10:33, and 10:33 - 10:36. So why is the unit test showing the 3rd of 4 time periods? Is SysTimeElapsed showing what has already been simulated (your answer would be right where start time = 10:30)? or what is being simulated (then something is wrong where start time would be 10:27)?

This relates to the else. It may be that ActualTimeS is really ActualTimeE and then ActualTimeS = ActualTimeE - TimeStepSys;

} else {
    ActualTimeS = state.dataGlobal->CurrentTime - state.dataGlobal->TimeStepZone + SysTimeElapsed;
    ActualTimeE = ActualTimeS + TimeStepSys;
}

I would think this would show up the the csv and edd so you could quickly find out which is correct.

Ok, I'm not sure why the comments I just made at the line are not showing up here...or why only half of your comment here shows up at the comment at Line 714. I'll paste in here and then respond to the second half of your comment about the else statement.

SysTimeElapsed gets updated at the end of the system time step. So, during a particular HVAC time step, while it is still simulating something, SysTimeElapsed is the time elapsed to the end of the last time step. So, in this case, because SysTimeElapsed is set to 0.1 and is not equal to the TimeStepZone, we are still simulating HVAC time steps, we have finished the time step that ends at SysTimeElapsed, and we are now in the NEXT HVAC time step. So, we have already simulated through 10:30 and are now in the 10:30-10:33 time frame. Does that make sense?

Now, what I'm not sure about is why when unix runs this that it thinks the answer should be 10:33-10:36. I think maybe because I used abs rather than std::abs, but that seems like a stretch. In any case, I did correct that and make another commit. Guess we'll see what comes back. So far, in the last hour+, I'm not getting any reports that the unit test is failing so I'm slightly hopeful.

Whew...unit test on unix is now coming back all green. Good thing because I had absolutely no other explanation as to why it would come back different on different operating systems. 😳😂

Ok, now about your else comment in the second half. The else condition is identical to what is being used in develop and I believe that is what we want. There weren't problems in develop with error messages so it seemed right to leave that as-is (this was a backtrack from the first version I committed after thinking this through again at your suggestion). And I believe that it is actually correct because SysTimeElapsed is not the end of the current HVAC time step but rather the previous one because it gets updated at the end of the HVAC time step. This hopefully gets rid of the .err file errors that showed up after the first commit where error messages that use this time stamp routine were shifted. That's not what we wanted, I think.

@RKStrand what an exciting PR! I appreciate your efforts and @rraustad scrutinizing this. I'm glad CI is coming out so clean with only a few unit test fixups.