search

SergiuToporjinschi / node-red-contrib-heater-controller

Heater controller for node-red dashboard
GNU General Public License v3.0
18 stars 17 forks source link

Non-optimal check for missing current temperature #51

Closed holgerpieta closed 2 years ago

holgerpieta commented 4 years ago

The node surprised me by stating that current temperature is not set (which also means that the slider is not movable), though I have been sure that it should have gotten one. Turns out, the way "missing current temperature" is checked in frontEndSlider.html isn't optimal. It's done by "!msg.currentTemp", which will also be false when currentTemp is 0. Which is the reasons why it didn't work for me: I was sending 0 as current temperature.

Anyway, I don't think that's intended behavior, because 0 degC is a totally valid temperature.

I'm not sure how to improve it, but something like "msg.currentTemp != undefined" might work.

SergiuToporjinschi commented 3 years ago

Hi, in read.me file:

This controller accepts one main input which has to have topic as "currentTemp" and payload needs to be a float The entire control is not functional until this message is received The heater status is recalculated when this message received, or when the user is changing the target temperature.

Message example:


{
    "topic" : "currentTemp",
    "payload" : "22.5"
}

the  currentTemp is the current value that you have, then this component will turn on and off the heater when the current temp will rich the target value that you are setting in front-end. This node does not have a thread, to execute the logic, is not checking all the time if the target is met. Is doing that only when a new message with a new currentTemp arrives.
Is this helping you?
holgerpieta commented 3 years ago

Oh, sorry, didn't notice that you answered.

I haven't encountered the problem again, because I limited my temperature values to >= 1, so the =0 case doesn't happen anymore. But given that the NPM version hasn't been updated since more than a year, I do not think the problem has been fixed. Maybe it has been fixed here in the repository, but I haven't tested that.

The problem is that the check for missing temperature is done by either !status.currentTemp or by status.currentTemp, which both will give wrong/unexpected results if currentTemp=0, because JS treats 0 as false. Better would be to check it for undefined, because that's not confused by the value 0.

SergiuToporjinschi commented 3 years ago

Oh, sorry. Is summer now and I did not used it for quite a while now. But, I had a complete refactoring of this but is not published yet. Is quite finished at least I know that is fully functional but I just not published in NPM. In Git is on branch Refactoring, if you want you can give it a try.

And this branch is 99% covered with unit testing so it should be very stable.

SergiuToporjinschi commented 2 years ago

The new version has been released, please check on latest version and reopen if is the case