Some issues with Threshold Alert

[domppn]

Hi Rich, I tried to use your ThresholdAlert rule template in openhab but I had some problem...my alert timer was not cancelling itself after my items were returning to their normal state (no alerting state). After reviewing your code I found three issues. Here is the first one (not related to my timer problem):

In this block, init AlertTimer is supposes to be cancel, but it's the alarm timer beeing cancel. It should be this line of code on line 475:

if(record.initAlertTimer !== null) {

https://github.com/rkoshak/openhab-rules-tools/blob/98a192d3edd269f90242940b1c6f978891118828/rule-templates/thresholdAlert/newThresholdAlert.yaml#L474-L479

The second one is the procEvent function:

https://github.com/rkoshak/openhab-rules-tools/blob/98a192d3edd269f90242940b1c6f978891118828/rule-templates/thresholdAlert/newThresholdAlert.yaml#L659-L668

On line 668, alerted is always set to false each time procEvent is fired. I changed this line to:

alerted: (records[name] === undefined) ? false: records[name].alerted};

The last one is also in the procEvent function:

The problem is if a user do not define a end alert rule, the notAlerting function never got called even if it's not in alerting state and alert timer never got canceled.

https://github.com/rkoshak/openhab-rules-tools/blob/98a192d3edd269f90242940b1c6f978891118828/rule-templates/thresholdAlert/newThresholdAlert.yaml#L674-L685

I fixed this issue by replacing this block to this:

  // Alerting state, set up an alert timer
  if(isAlertingState(value, record)) {
    alerting(value, record);
  }
   Not alerting, cancel the timer and alert if configured
 else {
  console.debug(name + ' is not in an alerting state');
   notAlerting(value, record);
  }

The logic of the original line 679 is already done is the notAlerting function.

I removed completly line 682 to 684.... if isAlertingState function can only return true or false then this block cover all possible condition.

Thank you very much for this rule template!!!.

[rkoshak]

This is a really complicated template and while I did my best, I'm really happy to have a second pair of eyes on the code. Thanks a ton for the fixes. If you can submit a PR that will probably be the most efficient way to make sure I get these fixes merged in correctly.

If not, I'll give it a go but it might take a day or two.

[rkoshak]

I had a few minutes to look at some things.

On line 668, alerted is always set to false each time procEvent is fired. I changed this line to:

That code should only run if there is no record, or there is no alertTimer, or there is no initAlertTimer, or alerted member of the record. This shouldn't be running on every event, only the first time any specific Item events.

I need to review whether it would ever be the case where alerted should be anything but false under any of those conditions. Clearly initializing it to false is correct if there is no record at all, or if alerted is undefined. Also, because when a timer is done or cancelled the record entry is set to null, there should never be a case where there is a alerted entry but not an alertTimer entry.

I'm not sure if it makes sense if one of the other timers is undefined though. I believe that those timers too get set to null when they are done so they wouldn't be undefined. If it is undefined, alerted should be false.

I do see that I am missing an = on the initAlertTimer.

We may need to break this up and handle the endAlertTimer and initAlertTimer separately. If there is an endAlertTimer that means that we have alerted so it might need to be changed to true in that case.

Ultimately, this is all about correcting cases that should not happen in the first place so maybe it is enough to just initialize it to false if it's undefined.

I removed completly line 682 to 684.... if isAlertingState function can only return true or false then this block cover all possible condition.

I'm not sure I agree with this explanation for this one. We really do have three conditions:

1. isAlertingState a. send the alert
2. Not Alerting and there is an endAlerting rule defined b. figure out if an end alert needs to be sent
3. Not alerting and there is not an endAlerting rule defined. c. we know an end alert doesn't need to be sent

However, case 3 is handled in the notAlerting function so it makes sense to simplify the if/else here.

The log statement in notAlerting under else if(!record.alertd) needs to be fixed though. If should read 'Exiting alerting state but an alert was never sent, not sending an end alert for " (bold added to show the changes).

And because notAlerting does the logging, the name + ' is not in an alerting state' log statement is redundant and can be removed.

---

Edit

I've done an analysis. The following are the values that alerted should be in if on of the other things in that if statement are undefined.

Undefined	alerted	reason
record[name]	false	If there is no record, this is the first event so we cannot have alerted yet
alertTimer	false	If this is undefined, we cannot have alerted.
endAlertTimer	?	If this is undefined we know nothing about whether or not we've alerted
initAlertTimer	?	if this is undefined we know nothing about whether or not we've alerted

Also, this is initialization/data correction. If the record exists at all, none of those other fields should ever be undefined. So I'm going to run with the following code which doesn't attempt to do data correction at all.

if(records[name] === undefined) {
  console.debug('Initializing record for ' + name);
  records[name] = { name: name,
                    alertTimer: null,
                    endAlertTimer: null,
                    initAlertTimer: null,
                    alerted: false };
}

We should never have a case where if the record exists at all, those fields should all be defined. If they ever are undefined that's an error that needs investigation.

---

Ive got a testing version with fixes in work. I'll create the PR once I'm convinced no other errors are introduced.

[domppn]

That's all makes sense. If you want me to test anything let me know.

Thank you again!

[rkoshak]

I've posted fixes for the above plus another one as the last reply to the fourm posting. You can copy what's there and replace everything after `~Functions` in the script action to test. I'm running with those changes for all my instances of this template and have seen no problems so far. A user on the thread discovered a problem with negative states and thresholds which I've fixed also.

I created the PR. The branch is taFixes: #106

Give that a go and if it addresses the above problems for you I'll post a PR.

[rkoshak]

I tested the changes for a few days and ran some special tests as well. I'm merged the code and this issue can be closed.