openaps / oref0

oref0: The open reference implementation of the OpenAPS reference design.
http://www.OpenAPS.org
MIT License
431 stars 395 forks source link

DUTY_CYCLE counts from END of cycle, not start #1058

Closed N3FM closed 5 years ago

N3FM commented 6 years ago

When setting the duty cycle, in oref0-pump-loop.sh, it seems to be sampling every 60 seconds. However, the elapsed time for the existing cycle is shown at the start of the delay loop, and it exits only when the sampling exceeds the set point (not when it matches). For example, set the duty cycle to 240 seconds. I would expect the next cycle to start 4 minutes after the previous cycle started. However, here is what we get: (previous cycle 'Starting oref0-pump-loop at Sat Jul 14 17:37:02 EDT 2018', checked pump clock at 17:38:22) ` Completed oref0-pump-loop at Sat Jul 14 17:38:34 EDT 2018

28 (of 240) since last run --> stop now. 88 (of 240) since last run --> stop now. 148 (of 240) since last run --> stop now. 208 (of 240) since last run --> stop now. 268 (of 240) since last run --> start new cycle.

Starting oref0-pump-loop at Sat Jul 14 17:43:02 EDT 2018 with 9 second wait_for_silence: ` This exposes two issues: 1) 28 seconds before 17:38:34 is NOT the time shown by previous 'Starting oref0-pump-loop'; 2) The duty cycle delayed 240 seconds from the END of the cycle. It shows the 28 seconds (plus preflights) consumed by the previous cycle, and exits 240 seconds after THAT (268 seconds). If we are trying to match up with the 300 second cycle of the Medtronic pumps, this will drift later and later each cycle. (In the example, 'Starting oref0-pump-loop' cycles began 6 minutes (360 seconds) apart when duty cycle was set to 240 seconds.)

For consideration: If we are retrieving the 'actual' MDT sampling/averaging time, add 5 minutes plus a fixed 'processing / error budget' offset to that time, to set the next retrieval time. Hope this helps! N3FM (Bob Smallwood)

scottleibrand commented 6 years ago

PRs welcome. Also tagging @juehv as he's the only one I know of using the DUTY_CYCLE code.

juehv commented 6 years ago

Sorry for the late response, somehow I missed the tag ... This behavior is as it's implemented. The duty cycling time is a guarantee for waiting this long not for starting after this time. This is because the pump loop gets started every minute by a cron job and the duty cycling code checks at the beginning if it's necessary to run. If not it exits. So you could get a little more responsive behavior if you change the cron job to ever half minute or so. But I think it's easier just to choose a little lower value than you want. I usually use half the time of my bg gets updates.

Think we could improve this to work more intuitive, but I guess it's not worth the work. In the current version it does its job and we didn't need to change too much of the other code.

N3FM commented 6 years ago

@juehv Jens, thanks for the info. The target I'm hoping to achieve is to only run a loop every 5 minutes -- and I think success in that arena would be a BIG DEAL, IF the prior loop was successful -- I'm hoping to NOT pull in any 'No new CGM ... ' messages, as my little rig is prone to get into mischief if it doesn't have new BG to play with (blaming that on 0.6.2 for now! ) If we get onto a 5 minute loop, that is consistent whether SMB runs or not, then the rhythm is in place (and now I can start looking at 'sleeping' the Edison, or other rig that has a working sleep mode available). Just got my 2nd rig, so hopefully I can start contributing more to the process.

N3FM commented 6 years ago

If we could change the behavior to be '300' seconds from the START of the last loop... instead of starting the counter when the loop exits... your counter target would be loaded with (300-x (minus another second?) ) we could reuse the existing counter.
If loop ran 75 seconds, set your counter target to (300-75) 225 and the counter exit would behave just as it does now. Your counter target would have to become a stored variable...

N3FM commented 6 years ago

Changing the value does not meet the core issue that I see... a 'vanilla' run may be less than 60 seconds, one with autotune and SMB may run 75 to 140 seconds (on my Edison). So, a 120 second duty cycle would trigger a loop anywhere from 3 to 6 minutes... I understand, maybe a different solution is needed -- but your duty cycle is SO close to my goal!

juehv commented 6 years ago

Not a bad idea to take the starting time instead of finishing time. It'll make the execution more predictable. I'll take a look at that.

juehv commented 6 years ago

How about making it depending on last seen CGM value? Wouldn't that be the feature you really want?

N3FM commented 6 years ago

For some users, that may work well. In my situation, I'm just looking to grab MDT cgm values without seeing 'no new cgm data' . Looking for a static 5 minute cycle. And if a loop takes more than 5 minutes, the next cgm value is already waiting for us. Aren't temp basals assuming a certain repeat cycle?

Bob

On Tue, Aug 21, 2018, 1:10 PM Jens Heuschkel notifications@github.com wrote:

How about making it depending on last seen CGM value? Wouldn't that be the feature you really want?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/openaps/oref0/issues/1058#issuecomment-414750376, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMbS3E5r38z0mITao-7s9frzlArLxTVks5uTD8GgaJpZM4VQByn .

juehv commented 6 years ago

have a prototype if you want to try:

DUTY_CYCLE=${DUTY_CYCLE:-540}   #0=off, other = delay in seconds

function check_duty_cycle { 
    DUTY_CYCLE_FILE="/tmp/pump_loop_start"
    LOOP_SUCCESS_FILE="/tmp/pump_loop_success"
    if [ -e "$DUTY_CYCLE_FILE" ]; then
        DIFF_SECONDS=$(expr $(date +%s) - $(stat -c %Y $DUTY_CYCLE_FILE))
        DIFF_NEXT_SECONDS=$(expr $DIFF_SECONDS + 30)
        DIFF_SUCCESS=$(expr $(stat -c %Y $DUTY_CYCLE_FILE) - $(stat -c %Y $LOOP_SUCCESS_FILE))

        if [ "$DUTY_CYCLE" -gt "0" ]; then
            if [ "$DIFF_SUCCESS" -gt "0" ]; then
                # fast return if last loop was unsuccessful
                echo "Last loop was not successful --> start new cycle."
                return 0
            elif [ "$DIFF_SECONDS" -gt "$DUTY_CYCLE" ]; then 
                touch "$DUTY_CYCLE_FILE"
                echo "$DIFF_SECONDS (of $DUTY_CYCLE) since last run --> start new cycle."
                return 0
            elif [ "$DIFF_NEXT_SECONDS" -gt "$DUTY_CYCLE" ]; then
                WAIT=$(expr $DUTY_CYCLE - $DIFF_SECONDS)
                echo -n "Wait for $WAIT seconds till duty cylce starts... "
                # we want to avoid wait since it keeps the CPU busy
                sleep $WAIT
                touch "$DUTY_CYCLE_FILE"
                echo "start new cycle."
                return 0
            else 
                echo "$DIFF_SECONDS (of $DUTY_CYCLE) since last run --> stop now."
                exit 0
            fi
        else
            #fast return if duty cycling is disabled
            #echo "duty cycling disabled; start loop"
            return 0 
        fi
    elif [ "$DUTY_CYCLE" -gt "0" ]; then
        echo "$DUTY_CYCLE_FILE does not exist; create it to start the loop duty cycle."
        # do not use timestamp from system uptime, since this could result in a endless reboot loop...
        touch "$DUTY_CYCLE_FILE"
        return 0
    fi
}
danamlewis commented 5 years ago

Resolved in https://github.com/openaps/oref0/pull/1112