Newalarm #340
Newalarm #340
Conversation
…e api documentation; add test for the case ringTime is not specified.
Resolved Conflicts: src/Infrastructure/TimeMgr/src/ESMCI_Alarm.C
| // A negative ringInterval indicates a one shot alarm at ringTime only | ||
| TimeInterval zeroTimeInterval(0,0,1,0,0,0); | ||
| if(clock->timeStep < zeroTimeInterval) | ||
| alarm->ringInterval = clock->timeStep; | ||
| alarm->ringInterval = -clock->timeStep; | ||
| } |
There was a problem hiding this comment.
- This seems problematic, particularly given the code in
ringerOffthat decrementsringTimebyringInterval. (See also Newalarm #340 (comment).) I think it would be better and safer to have a boolean flag that says whether this alarm rings on an interval or is a one-shot alarm.
| } | ||
|
|
||
| if (ringTime != ESMC_NULL_POINTER) { | ||
| alarm->ringTime = alarm->prevRingTime = alarm->firstRingTime = *ringTime; |
There was a problem hiding this comment.
- I think it's a problem that
firstRingTimeis no longer set: it's still referenced inringerOff(see Newalarm #340 (comment))
| void Alarm::enableSticky(void){ | ||
| // 1/100th of a second granuality to ensure integer return when being divided by | ||
| //this->ringInterval=TimeInterval(0, 1, 100, 0, 0, 0); | ||
| this->ringInterval=TimeInterval(1, 0, 1, 0, 0, 0); | ||
| } |
There was a problem hiding this comment.
- This warrants investigation: First, note that this is now using a 1 second granularity rather than the 1/100 second mentioned in the comment. But more importantly, it looks like this setting of
ringIntervalwill differ depending on whether an alarm is initially created as sticky (in which case I don't thinkenableStickyis executed, and so ringInterval is either kept at what is set or set to a negative value for a one-shot), or if it's later set as sticky (in which case thisenableStickyis called).
| if(old_direction == ESMF_DIRECTION_FORWARD) | ||
| ringTime = ringTime - ringInterval; // ringTime goes backward by 1 interval | ||
| else | ||
| ringTime = ringTime + ringInterval; // ringTime goes forward by 1 interval | ||
| } |
There was a problem hiding this comment.
- This warrants investigation: Although I haven't investigated carefully yet, it seems like this will result in different behavior in these two cases: (1) An alarm is set on a forward-running clock and then the clock is later set to be running in the reverse direction (so this function is called); (2) An alarm is set on a clock that is already running in reverse (in which case I don't think this function is called).
- Is it right that the behavior differs in these two cases?
- If so, is this intended?
I also wonder more generally about the purpose of adjusting ringTime like this when changing the clock direction. I think I understand why this is done: so that the alarm is ringing for the duration of the same Nth time step in reverse as it was forwards. But I want to confirm that this is what we want to do, because it could be confusing to a user if the ringTime is adjusted when reversing the clock's direction: what if they really intended for the ring time to be what they set it as? At the very least, we should make sure this is clearly documented – e.g., that ringTime is the time at the start of the time step in the forward direction, or the end of the time step in the reverse direction if the clock is later reversed. (I think that's the current behavior.)
There was a problem hiding this comment.
Note, though, that this is at least somewhat similar to what's done on develop if userChangedDirection is true.
There was a problem hiding this comment.
Actually, I think my initial understanding of why this is done was wrong:
I think I was thinking about this in terms of a one-shot alarm, and I actually think we probably don't want to apply this in the case of a one-shot alarm (this is currently problematic because of the way ringInterval is set for a one-shot alarm; it should probably either be 0 or we should have an explicit flag saying if this is a one-shot alarm, and if so, should avoid doing this adjustment).
But I actually see the rationale for why this is done for a repeating alarm: If ringTime gives the next time the alarm will ring, then we reverse directions in between two rings, then we need to adjust ringTime by one ringInterval so that it gives the "next time" in this new direction.
So maybe we want this in place, but we want to avoid triggering it in some cases: for one-shot alarms and maybe some other cases:
- e.g., what if
ringTimeis already exactly at the current time? (I'm not sure about this case.) - e.g., what if we haven't reached the first
ringTimeyet (i.e., this alarm hasn't rung yet)? It seems like we shouldn't adjustringTimein this case.
This seemed like an incomplete test that wasn't testing anything new (at least at this point, with the test being incomplete). Note that a reverse sticky alarm is currently covered with Test_AlarmHang, particularly after the extra testing I added in that test.
More rework is still needed for this test so that it actually tests what it says it's testing.
It looks like this has been effectively replaced by canRingAtTime
Go for an extra couple of time steps to make sure the alarm isn't ringing at those additional time steps. (But don't bother with this for the test that runs both forward & reverse, because that would be messier.)
Demonstrate that, currently, a sticky alarm operates the same way as a non-sticky alarm - though I feel this is unintuitive behavior.
| !call ESMF_AlarmDebug(alarm,'test3',rc=status) | ||
| end subroutine | ||
| !------------------------------------------------------------------------ | ||
| subroutine ForwardAlarm_Test4(rc) |
There was a problem hiding this comment.
- The current behavior as demonstrated in this test seems problematic and unintuitive, especially for sticky alarms: As demonstrated here, when the alarm dt doesn't align with the clock dt, the alarm will only ring when the two come into alignment, even for a sticky alarm. This is (I think) in contrast to the previous behavior, when the alarm would ring when we first pass the ring time.
|
Need to review this PR against the new proposal from the NASA alarm branch. |
Replaces #121 .
Relevant issues:
See also: