Pause

[readeral]

Implements pause feature, which required solving the problem of constant running Timeout identified in line 531-532:

        // Once the node is instantiated this keeps running
        // I'd like to change this to run when only needed

[linuxha]

Sadly, I never wrote down all the use cases, looks like it might be a good idea to do that now. Also I unfortunately allowed msg.payload = { ...} code, which complicated this greatly. I didn't understand the change node at the time.

First let me say, yes we'll add the pause code, in general I like your code. I think you'll start to see why it gets a bump in major revision (this is not a huge issue).

I'm not rejecting the PR yet. I might have a use case where it fails, I'm checking it again in case it's the testing code. The current testing is a manual check of the results and it's hideous to look at but it's covered my butt so far. I also have a few new test cases to add.

I'd prefer that we use play rather than on as the restart command. This does make things more complicate. I'd also prefer that we use state=pause. I might want to send out a

{ "payload": ticks, "state": 3, "flag": "pause"}

Every second, while in pause (I have an IR devices that tracks the time remaining).

In general I like your code. I do like the setInterval put in the on() function. and the clearInterval in the stop(). And I now know how to use the ${var} (oh, backticks!). I'll take a look at everything in about 6 hours as I have hose work to do now (I'm EDT, I think you're AEST. so 14+ Hours different).

[readeral]

So the specific use case I'm trying to cover is your node is the basis for one of my examples on this node I've been working on as a visual count-down timer.
Examples where this is helpful:

• As a Pomodoro timer
• As a way of catching anomalies (for example someone might choose to put the timer into a pause state, setting their own shorter timeout to send a 'stop' command if a condition isn't met)

In reality 'Pause' is a bit of a hack. A timer has only 3 real states: running, stopped, init. The kicker is the 'tickler' functionality, which expecting a certain behaviour from the start function, derives some intermediate states. To 'restart' a timer (any timer, not this one in particular) is actually just to send a 'on' command with new parameters. And in a real sense pause must be treated as a stop command. but, having stowed away the time at which the timer was stopped.

If I were float a suggestion that I have no expectation you'd take up, it would be to throw out my 'pause' state and actually redesign the 'stop' state to work how I've programmed pause, as that more closely resembles other countdown timers (like the one on my iPhone). But I realise you have this in play in the real world, and that would be too much of a change in functionality.

So to your comments:

1. State 'pause' - that was actually an oversight on my part, see no. 3
2. Writing a Play() function goes against DRY principles, and I don't see it offering any additional functionality over On() - given that 'restarting' is simply 'start' with a revised msg.timeout there is nothing new here. If you 'restart' from a paused state, and then want to tickle the timer, the initial parameters are used, not the paused ones.
3. I didn't really understand what "state": 2/"state": 1/"state": 3 meant because you haven't documented it, but I suspect it's specific to your use case where:

• 1 equals 'normal unwarned state'
• 2 equals 'warning has been activated but still running'
• 0 equals 'timer is stopped'
  It seems to me that a 'paused' should by default have a state of '0', and if you want the ticks to continue, it should send either state 1 or state 2 as per normal.

4. Rather than coding play() we should instead allow a parameter be passed with the 'pause' command (or allow a 'suspend' command) to determine whether the ticks continue (I assume to output like '60, 59, 58, 57 (paused), 57, 57, 57, 57 (on), 56, 55...) or not. This could then be passed as a boolean to pause() to determine if clearInterval() should be called or not, and will be passed to TIX listener to skip decrementing the timer. That way, your "state": 0/1/2 is preserved without the need for a "state": 3, because those values should be related to the progress of the timer value, not to the state of the overall timer itself.

I'll revise my pull request accordingly, if I can work out how to do that...

[linuxha]

First I'm not too stuborn with the architecture that we can't adjust it,

My use case
I have 2 specific use cases, the first is a simple timer to keep a light on. A sensor, such as an IR sensor will send a tickle (on) to node-red. Node-red seeing that message will turn the light on. After x seconds of no on messages the light will be turned off. One problem is that some sensors can send junk (hence anything not an specific command is an on).

The second use case is more complicated and is still being worked out. Its a kind of sleep timer. There are devices that can be turned on or off and a TV (IR) that toggles with the power command. Often you don't know the state of the TV but if the sleep timer is started you can assume it's on. The commands for turning on/off and adjusting the timer can come from a web page (node-red) or a TV remote (IR). The remote can send a timer on, a tickle (another on), an off or a cancel or stop (I haven't figured out which yet). The modules in charge of the TV is an ESP-8266 with an IR Transceiver. It receives the commands from the remote, sends to node-red. It also receives commands from node-red and translates those to the TV. Also it keeps track of the timer state via the second output (Red, Yellow, Green). It doesn't do any timer functionality itself.

[linuxha]

I didn't really understand what "state": 2/"state": 1/"state": 3 meant because you haven't documented it

Yes, that's some code left over from an earlier attempt before I attempted to understand a state engine (I've not used a state engine before). Originally it was just 0/stop and 1/run. Then my wife decided she might some kind of warning so I tossed in 2/warn. This is for the ESP (coded in C/C++)

[readeral]

Ok, well I think you'll like the changes I've made (once I've worked out how to rebase my pull request) because they don't require reprogramming your ESP-8266 for a 0/1/2 scenario.

I've rethought changing the on() behaviour, even in a paused state it will now tickle the timer. To restart from a paused state now requires payload to be either pause or suspend, which means whoever is programming node red needs to put that logic into their own program for restarting after a pause. I have thrown them a bone however, as sending 'pause' as the payload while the state is 'off' will start the timer.

I've put in a 'suspend' command which is almost the same as pause (and uses the same pause code) except it continues to send ticks to output 2. Otherwise 'pause' will send a '0' state with the flag 'pause', and the messages to output 2 will stop.

An additional use case for the Suspend option could be a solution for false positives for your light sensor - it would mean a slight delay in a light turning on (one tick) but using suspend for one tick to test whether or not there has been continual movement over that period, and if not, to send 'suspend' again (to continue the countdown) would mean the light would turn off after the designated time, rather than restarting just because a flower drooped over or something (I don't really know...) - and if there was continual movement, sending an 'on' msg would reset the timer from a suspend state.

[readeral]

Turns out I didn't have to rebase given there's been no other commits

[linuxha]

My view, which may not be correct, of the state engine was as follows:

1. Init - done on instantiation of the node-red module (and every reload)
2. stop - default state (after instantiation and after a run)
3. run - timer counting down

I kind of view pause as another state as it's between stop and run and I need to monitor that the timer is paused (see below).

The pause is interesting in that I don't have a way for my wife to make that occur so I shouldn't see it. But it does cause an inconsistency in that if it should occur, I can't see it on output 2. I've usually assumed that if there was no message on output 2 the time is no longer running.

Now here's what I found that's a problem: If we use on, then a pause, then an on we're okay (timer runs, paused and restarts with remaining time). I'll explain why not good in a moment. Now, with the timer running if I send a plain on I'd expect the timer to just go on back to it's default timeout from when the timer was initial turned on but it doesn't (test script output):

home/test/countdown ### on, pause, on & on
home/test/countdown-in-b on
home/test/countdown-out-b on
home/test/countdown-out-b-txt 10 <- default timeout (normal)
home/test/countdown-out-b-txt 9
home/test/countdown-in-b pause
home/test/countdown-out-b-txt 8
home/test/countdown-out-b pause
home/test/countdown-out-b-txt 7 <- 7 seconds remaining

... (script slept for 4 seconds as expected) ...

home/test/countdown-in-b on
home/test/countdown-out-b on
home/test/countdown-out-b-txt 7 <- Good, we've running again with 7 seconds
home/test/countdown-out-b-txt 6
home/test/countdown-out-b-txt 5
home/test/countdown-in-b on <- Sent an on (a tickle)
home/test/countdown-out-b on
home/test/countdown-out-b-txt 7 <- Oops, should be 10, not 7
home/test/countdown-out-b-txt 6
home/test/countdown-out-b-txt 5
home/test/countdown-out-b-txt 4
home/test/countdown-out-b-txt 3
home/test/countdown-out-b-txt 2
home/test/countdown-out-b-txt 1
home/test/countdown-out-b off
home/test/countdown-out-b-txt 0

I haven't looked at you code to suggest a good way to fix the above, but it should be too hard to fix.

Now my explanation for why not good with the Pause/On. I have documented that msg.payload = "on" means use the defaults (setup in the config node). I also have documented that you can override the timer with msg.payload = { "payload": "on", "timeout": 20, "warning": 10 }. I haven't given this much thought yet.

Hmm, not sure my safe/unsafe input has been considered (or coded properly on my part). By changing the safew/unsafe string you should be able to define the on (safe) and off (unsafe) commands. Dang more growing pains. Also need to add a test case for that.

[readeral]

Yeah so I think you'll see I've addressed your concerns with Pause/On by requiring a Pause/Pause. Your documentation remains accurate.

Another possible problem-solver could be to have in the node edit config an 'allow pause' flag which means for that particular timer, pause isn't even available. Set it as 'off' by default, and backwards compatibility for any and every situation is preserved.

Regarding your test, I'm not sure why it's doing that, given what I'm seeing in the output on node-red is a reset to the original time. However, try running your test with my new commit, and I think that will be resolved now.

[linuxha]

Okay let me setup a feature branch, I have a bug-fix branch but I hasn't had a chance to add the test suite for it. :(

I have no idea how to automate the testing yet. I know how to test it but setting up the node needs to be manual. I'll include the test script in the t directory. I wan't actual able to get the code in the test directory to behave (another thing to learn).

I have a docker container built for testing my code. Can't test on my production network, wife would kill me. ;-)

[readeral]

To have access to your test script would definitely help to determine why it's outputting what it is, because at the moment I don't have visibility on the variables it's drawing upon.

[linuxha]

In the z directory you will find, the manual-test.sh (a bash script). Unfortunately it's still pretty sloppy (error messages that can be ignored). I'm not usually this sloppy but this was really thrown together quick and it has grown into it's own monster. The errors can be ignored as they are for processes that get hung (when things really break).

• z - directory with the test files
• z/loop - a sample command I used to test
• z/manual-test.sh - the test script
• z/flow.json - the flow I use in node-red

Required:

• node-red
• mqtt (I'm using mosquitto)
• mosquitto_sub
• mosquitto_pub

The script will create some junk files. I leave them around for when things go wrong. The results to look at are in the results.txt file (I'm very creative with my naming).

I'll do a merge into the new branch: feature-pause-mytimeout. This is based on the origin code. I haven't been able to test the float fix code yet other than the most basic of tests. I was planning on do that this weekend. Oh well. ;-)

[readeral]

Ah yeah, the FLOAT fix is just a comment. That should be a separate pull request after #14 is merged I reckon.

[readeral]

Thanks for the testing files, I'll have a look. TBH I have very little idea about testing, I haven't really committed the time to understanding it yet, so this will be a good opportunity.

[linuxha]

BTW, my normal job is QA, I break things. :-) I've been doing this for 40 years but in different parts of the technologies. So even with that background, modern development is new to me. So any mistakes I make, let me know. The more I learn, the less I know. ;-) and I have a lot to learn.

[linuxha]

This is not very good testing but it can be the basis of some automation if I get the time to think about. it.

I have no idea how to function testing on a node. I can only see doing end-to-end testing. Also I don't fully check the msg object on output 2, just the payload. I'll need to make that change a bit later. Hopefully I'll be able to automate it by then.

[readeral]

Ah now I see, end to end, so you were literally testing the output values, rather than internally testing the code. FWIW, testing your node functions, you'd probably use a library like Jest. I just haven't taken the time to learn how it works ;-) I have never worked professionally as a developer, or in IT at all, so generally don't have much motivation to learn these sorts of things that (if I honestly want to do open source stuff) I really should be learning.

I have run out of time today to load your testing up and stuff, so will have to leave it till tomorrow.

[readeral]

I'm entirely ignorant of MQTT - is there something additional I need to set up to get this testing suite to work? The nodes refer to a server named 'Mozart'

[linuxha]

The reference to mozart.uucp is a hostname of my mqtt server (internal dns). It can be replaced with the hostname of the mqtt server (if you have it registered in you dns) or an IP address. The mqtt server is a message broker. It's pretty simple to setup. Just install it with your package manager. Also install the mosquitto clients and that will give you the mosquitto_sub and mosquitto_pub commands. There are several good mqtt tutorials, google it (I can't find the old links I had, sorry).

[linuxha]

I've made a mess of the code/pull request but I hope I'll be able to work around that will new code with the pause feature. I think you mentioned using pause/suspend. I'll add that code but I'm going to document it as pause/cont with a note that pause/suspend will work.
I have the first run of the test suite for the pause feature built and it will be in the feature-pause-3.2.3 branch. I'll commit that after I clean up that test case as it fails now but that's because I wanted it to fail. I still need to add some more test cases but the initial check looks okay. Hope to have the first run pushed tomorrow and after the rest of the test cases are written and pass then I'll release it as 4.0.0.

[linuxha]

Latest code in the feature-pause-3.2.3 branch. Currently basic pause/suspend & pause/continue works. But pause/pause and suspend/suspend fail the test cases. I've not added testing with the stop state. I also don't think I comprehend the suspend requirement. I'll reread this thread as I've forgotten a lot of it.

[linuxha]

Okay I've determined that the pause to pause mytimeout and a second pause to unpause it won't happen. It's going to be pause/continue. The reason is that the anti-loop code needs to see a different message or a Tix increment. Since the timer has stopped, no Tix increment. I thought about using the last timestamp+offset but decided against it as different messages work better and this allows multiple ons (which may occurs with something like a occupancy sensor) to reset the timer to the freshly triggered state.

I'd also like to get rid of the suspend. Just leaving pause/continue to pause/unpause mytimeout. More later

[TailwindNL]

I really would like a pause function. Any news on when it will be available?

[linuxha]

I really would like a pause function. Any news on when it will be available?
Sorry, all hell broke loose around here (personal life) and I've been trying to catch my breath.
I've pushed the latest to the origin/feature-pause-3.2.3 branch. I'm not sure how far I got with testing. I think this branch has all the latest tests also.

[linuxha]

Remember I couldn't use your code as my code got too far out of sync. Take a look at the states. I'm not using suspend, just 0, on, off, pause, continue, stop, cancel. Junk is still treated as an on command.