-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Time schedule: sync if conditions are fulfilled and last sync was more than 1h ago (fixes #592) #766
Conversation
when running on schedule, this allow syncthing to run if it was last running more than WAIT_FOR_NEXT_SYNC_DELAY_SEC ago instead of keeping a strict schedule
wow, cool 👍 . Many thanks! I think we will have a merging session together if I'm back at my pc. |
I think we should store mLastRunTime as a PREF_ so the app considers it when closed and relaunched. I can help with that. |
Assume: run on schedule, run on WiFI: a)
b)
c)
Current behavior: With this PR: When storing mLastRunTime in settings (not tested): I think for consistency the initial state of mTimeConditionMatch should depend on mLastRunTime, something like Then I would expect: But if syncthing does not run at a),
Negative values should not matter, in this case And: when storing the time |
or better: So |
Looks good to me what you suggest, but currently, I've got no time, sorry. Will come back later here, compile and test it on the emulator to get an impression. |
No problem, this just means longer testing on my side. Meanwhile I've adjusted the version running on my phone to use But I'll keep using |
I'm fine with your decision :-). Every improvement over how it was before is good. |
|
||
private long mLastRunTime = 0; | ||
private Boolean mTimeConditionMatch = false; | ||
private Boolean stopJobScheduled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take mStopJobScheduled , optionally does the positive way ease understanding of the code (mStartJobScheduled)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't know what name would be best for understanding...
Basically I needed some variable to store whether we are in run-for-5-min phase, where the next intent with ACTION_SYNC_TRIGGER_FIRED
stops this phase (this intent is scheduled when entering the phase)
mStopJobScheduled is not nice, intially I had named it something like runningAllowed, but that's not better...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, something with "m" in front is best. choose what you like or we take mRunAllowedStopScheduled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too long, and clear enough. Thanks!
@@ -29,7 +29,8 @@ | |||
public static final String PREF_RESPECT_MASTER_SYNC = "respect_master_sync"; | |||
public static final String PREF_RUN_IN_FLIGHT_MODE = "run_in_flight_mode"; | |||
public static final String PREF_RUN_ON_TIME_SCHEDULE = "run_on_time_schedule"; | |||
|
|||
public static final String PREF_LAST_RUN_TIME = "last_run_time"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> down to line 88 because it's a cached pref the user does not see nor can edit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that would definitely be the right place
Thanks, do you consider this production ready now? Should we change some UI strings or put explanation somewhere (in app, wiki) to tell users the changed feature behavior? |
Yes, I think the code is ok now. https://github.com/Catfriend1/syncthing-android/blob/main/app/src/main/res/values/strings.xml#L532 |
Good! we only need to adjust the values/strings.xml (no translations, only the english one). Transifex will later do the rest. |
Merged into "beta" branch. |
@Helium314 Would you mind to also show when the last sync was as that would be more useful for our users? How did you test it? I'm currently failing to test because of SystemElapsedTime (which is not a bad idea but I cannot change via date/time control to jump an hour to the future). |
The simple and blunt way: switching off WiFi before first hour ends, switching back on a bit later and checking how long ST stays running.
Currently testing with something like
|
btw: I've fixed the Android 11 emulator detection so it's easier for you to test the sync scheduling. See 35cbb17 (intervals are lower on the emulator) |
@Helium314 Would you mind adding more comments between the lines on how the new timing mechanisms work in PR #766 and PR #768 ? |
PASSED. Log when running without "run according to time schedule checked" -> result: Syncthing stays running all time.
```
2021-03-28 16:58:08.567 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 10(+120) seconds.
2021-03-28 16:58:18.597 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 16:58:38.630 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 16:59:08.842 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 16:59:39.062 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:00:09.242 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:00:39.413 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:01:09.604 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:01:39.786 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:02:12.105 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:02:44.470 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:03:14.678 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:03:44.880 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:04:15.064 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:04:45.254 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:05:05.285 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:05:25.327 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:05:55.560 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:06:25.749 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:06:55.955 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:07:15.989 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:07:36.022 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:08:06.157 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:08:36.224 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:09:06.344 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:09:36.413 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:10:06.502 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:10:36.625 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:11:06.708 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:11:36.761 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:12:09.104 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:12:39.281 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:13:09.466 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
```
|
PASSED. Log when running with "run according to time schedule checked" -> result: works correct. Syncthing flip flops on/off. 2021-03-28 17:14:40.132 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 10(+120) seconds. |
PASSED: Turning wifi (required condition off) -> Log repeats "Scheduled SyncTriggerJobService to run in 20(+120) seconds" ("off" intervals are scheduled over and over again). After the first 20 secs off interval passed by, the run condition monitor shows this signalising it's time for a new sync schedule but wifi is required. Now enabling WiFi. Got 10 seconds of ON interval and syncing. 2021-03-28 17:20:30.501 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 10(+120) seconds. After that, with wifi on, the flip flopping continues as usual. Log:
2021-03-28 17:17:55.064 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:18:25.311 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:18:55.451 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:19:25.623 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:20:00.471 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:20:30.501 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 10(+120) seconds.
2021-03-28 17:20:41.146 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-03-28 17:21:06.427 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 10(+120) seconds.
2021-03-28 17:21:17.032 13758-13758/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
|
@@ -104,7 +105,8 @@ public void run() { | |||
* Holds true if we are within a "SyncthingNative should run" time frame. | |||
* Initial status true because we like to sync on app start, too. | |||
*/ | |||
private Boolean mTimeConditionMatch = true; | |||
private Boolean mTimeConditionMatch = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this switched to intially be "false"? The comment above it needs to be adjusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you already figured that out... sorry, a short comment might have saved you a lot of time
There is a problem when the system clock resets (very high positive values to wait for next sync) or the last sync was more than an hour ago (force stop case, negative scheduled values). Log:
2021-04-02 14:24:40.741 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 11(+120) seconds.
2021-04-02 14:24:46.096 7467-7467/com.github.catfriend1.syncthingandroid.debug D/RunConditionMonitor: JobPrepare: mTimeConditionMatch=false, elapsedRealtime=826916, lastSyncTimeSinceBootMillisecs=812514, elapsedSecondsSinceLastSync=14
2021-04-02 14:24:46.104 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 6(+120) seconds.
2021-04-02 14:24:49.429 7467-7467/com.github.catfriend1.syncthingandroid.debug D/RunConditionMonitor: JobPrepare: mTimeConditionMatch=false, elapsedRealtime=830249, lastSyncTimeSinceBootMillisecs=812514, elapsedSecondsSinceLastSync=17
2021-04-02 14:24:49.440 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 3(+120) seconds.
2021-04-02 14:24:53.271 7467-7467/com.github.catfriend1.syncthingandroid.debug D/RunConditionMonitor: JobPrepare: mTimeConditionMatch=false, elapsedRealtime=834091, lastSyncTimeSinceBootMillisecs=812514, elapsedSecondsSinceLastSync=21
2021-04-02 14:24:53.274 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in -1(+120) seconds.
2021-04-02 14:24:53.718 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
2021-04-02 14:24:58.656 7467-7467/com.github.catfriend1.syncthingandroid.debug D/RunConditionMonitor: JobPrepare: mTimeConditionMatch=false, elapsedRealtime=839477, lastSyncTimeSinceBootMillisecs=812514, elapsedSecondsSinceLastSync=26
2021-04-02 14:24:58.659 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in -6(+120) seconds.
2021-04-02 14:24:59.057 7467-7467/com.github.catfriend1.syncthingandroid.debug I/JobUtils: Scheduled SyncTriggerJobService to run in 20(+120) seconds.
|
I didn't see the "updateShouldRunDecision();" already has code to catch the negative values and very positive values. So if I remove my simulation code line "elapsedRealtime = 0;" (l.175) it works correctly. Though, I feel more comfortable with more comments, especially on the "mRunAllowedStopScheduled" mechanism. I am sitting over the code for more than an hour now and fail to understand how it works, even the "run immendiately when share to" is used works correctly during tests. |
Anyway, I'm giving this a shot. It worked too well on my tests :-). But in return, I hope you'll assist me in the future if we need to polish the whole SyncTriggerJob thingy together, add more comments on code (would be my wish for you next PR @Helium314 if I'm allowed to make one :-) ). Especially this "systemClock.elapsedTime" thingy could be split in functions to stamp out negative / postive "wrong" values maybe in the init check of RunConditionMonitor before the first code parts might run into calculations and requiring to fix the values up "last-minute". Thanks for the contribution! |
Wow, there was a lot going on while I wasn't watching 😄 sooo... slowly going though your updates:
sure, will go through it again and comment
there seems to be a lot of scheduling going on even when not running on schedule... this doesen't look nice (but on a real device it would only be every hour, so not that bad). |
|
With this PR, syncthing running on schedule will start immediately once all other run conditions are fulfilled and last sync was more than 1h ago.
Currently it is working as intended on my device.