Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#2584 closed Bug (fixed)

Checks for AltTime (Turtle mode) start and stop may be missed

Reported by: Grug Owned by: charles
Priority: Normal Milestone: 1.90
Component: libtransmission Version: 1.76
Severity: Normal Keywords:
Cc: ian@…

Description

I've noticed a few times that turtle mode has not enabled (set to enable at 2:15am and disable at 10:00am). I took a look around the code and found this:

libtransmission/session.c:onAltTimer() - around 1199

        isBeginTime = currentMinute == session->altSpeedTimeBegin;
        isEndTime = currentMinute == session->altSpeedTimeEnd;
        if( isBeginTime || isEndTime )

I read this as, if the minute matches that of start or finish then (after a couple of other checks) enable/disable turtle mode. If the timer for this function is delayed then turtle mode will fail to either start or stop.

Attachments (2)

speedlimit.diff (2.0 KB) - added by livings124 13 years ago.
Potential fix, that constantly tries to set the limit on/off, regardless of if it's at the edge
speedlimit2.diff (2.0 KB) - added by jdoliner 12 years ago.
Modification of speedlimit.diff that won't override the users setting

Download all attachments as: .zip

Change History (16)

Changed 13 years ago by livings124

Potential fix, that constantly tries to set the limit on/off, regardless of if it's at the edge

comment:1 Changed 13 years ago by livings124

You could remove that "if (enable)" part, too lazy to reupload

comment:2 Changed 12 years ago by charles

livings124: if the change is that easy, we can just use isAltTime() instead of reinventing the wheel wrt figuring out whether or not we're within the alt time boundaries.

However, if we have this test every minute, what happens if alt speeds are on because of the time, and we then turn them off by clicking the blue turtle? won't it re-enable itself a minute later? That doesn't sound desirable to me.

comment:3 Changed 12 years ago by Grug

Would it be better to check for an edge within the period of 3-5 mins after the start time?

Something like:

isBeginTime = currentMinute == session->altSpeedTimeBegin
              || (currentMinute - 1) == session->altSpeedTimeBegin
              || (currentMinute - 2) == session->altSpeedTimeBegin;

comment:4 Changed 12 years ago by livings124

Perhaps the timer can be set to fire at the exact time it should turn on or off (perhaps with a bool to know what we're doing) instead of every minute.

comment:5 Changed 12 years ago by charles

As an aside, how much of a problem is this in practice? There aren't many things that will block the libtransmission thread for a full minute.

comment:6 Changed 12 years ago by livings124

My suggestion just seems cleaner, but blocking for a full minute seems pretty crazy.

comment:7 Changed 12 years ago by charles

livings124: I made a quick stab at implementing your suggestion, but calculating the next switchover time was kind of ugly. Do you want to give it a try?

comment:8 Changed 12 years ago by livings124

sure, why not?

comment:9 Changed 12 years ago by charles

  • Milestone changed from None Set to Sometime

comment:10 Changed 12 years ago by charles

  • Milestone changed from Sometime to None Set

Changed 12 years ago by jdoliner

Modification of speedlimit.diff that won't override the users setting

comment:11 Changed 12 years ago by jdoliner

Can't we solve this directly by just checking if the user was the one who originally turned the turtle on/off and not change if that's the case. Also in both the modified and trunk version it doesn't seem like the function will ever disable alttime. Which seems kind of weird to me since the pre function comment says it should.

P.S. this is my first time contributing to this project at all so I apologize in advance for any breaches in etiquette I may have made.

comment:12 Changed 12 years ago by charles

  • Milestone changed from None Set to 1.90
  • Status changed from new to assigned

fixed in trunk for 1.90 by r10093

comment:13 Changed 12 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:14 Changed 12 years ago by charles

xref: #2907

Note: See TracTickets for help on using tickets.