Skip to content
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

Dex Share Follow Delay #3663

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Navid200
Copy link
Collaborator

@Navid200 Navid200 commented Sep 14, 2024

Fixes: #1849

This PR adds the ability to add an optional delay to the Dex share follow polling time.
This becomes necessary if the setup has unavoidable delay on the source side (pump/internet/ ...) as explained in this open issue: #1849. Without this, xDrip polls only twice, 5 minutes after the last time stamp and 5 minutes and 20 seconds after the last time stamp. After that, if xDrip does not get a new reading, it will go to sleep for 5 minutes.

Adding a small delay to polling could avoid an extra 5-minute extra delay in readings.

The following image shows the Settings page after this PR, where you can see the new Dex Share Follow delay setting.
Screenshot_20240914-091039
Tapping on that will lead you to the following page where you can choose one of several options for the delay.
Screenshot_20240914-104706


Testing
My main phone uploads to Nightscout.
I have 2 test phones.
Test phone 1 is a Nightscout follower as well as a Dex Share master. It gets readings from my main Nightscout site.
Test phone 2 is a Dex Share follower getting readings from test phone 2.

By adjusting the Nightscout follower delay on test phone 1, I can adjust the delay Dex share master (same phone) experiences.

The following image shows the status page of test phone 2 under the following conditions:
Nightscout follow delay set to 40 seconds
Dex Share follow delay set to 0.
Screenshot_20240914-101414
You can see that the delay from Last BG time to next poll time is 10 minutes. This is because the poll at 5 minutes and 5 minutes and 20 seconds have failed and xDrip has gone to sleep for 5 minutes after which the total delay is 10 minutes.

The following image shows the status page of test phone 2 with the Dex Share follow setting set to 40 seconds.
Screenshot_20240914-103022

Comparing Last BG time and next poll time, you can see a delay of 40 seconds. So, by intentionally delaying the reading by 40 seconds, we have avoided a 5-minute extra delay.

I have tried all possible delays for test phone 1 and choosing enough delay for test phone 2, I have been able to avoid the extra 5-minute delay.

The following image shows the status page of test phone 2 with the Dex Share Follow delay set to 2 minutes.
Screenshot_20240914-104028
You can see an effective delay of 7 minutes, which is an extra delay of 2 minutes over the default 5 minutes.

@Navid200
Copy link
Collaborator Author

Why did I change Anticipate?

In a previous PR (#3179), I added the ability to add a delay to Nightscout follow. I did that by changing Anticipate.
But, Anticipate is used by other methods like Dex share follow.

Now, I have made a change to do it right.
Nothing inside Anticipate is specific to Nightscout or Dex Share any longer.
Anticipate now has an optional parameter, lag.
The calling method can set this optional parameter to a non-zero value to delay the next poll time.

This way, the different methods can continue to use Anticipate without interfering with each other.

@Navid200
Copy link
Collaborator Author

Navid200 commented Oct 2, 2024

We made a similar change to the Nightscout follower previously: #3179

This PR does the same for the Dex share follower.
The Anticipate method has been changed now to make it a more modular approach, which I believe how I should have done it when I submitted the other PR for improving Nightscout follow.

I have tested both Nightscout follow and Dex share follow after this PR to make sure both function as expected.

@@ -58,6 +58,7 @@ public class NightscoutFollowService extends ForegroundService {
private static volatile Treatments lastTreatment;
private static volatile long lastTreatmentTime = 0;
private static volatile long treatmentReceivedDelay = 0;
private static long lag = Constants.SECOND_IN_MS * Pref.getStringToInt("nsfollow_lag", 0); // User can choose a wake delay with a 0 default.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By setting this in a static field it will not get updated if the user changes the preference value while the class remains loaded.

@@ -57,6 +57,7 @@ public class ShareFollowService extends ForegroundService {
private static volatile long lastBgTime;

private static ShareFollowDownload downloader;
private static long lag = Constants.SECOND_IN_MS * Pref.getStringToInt("dex_share_follow_lag", 0); // User can choose a wake delay with a 0 default.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about storing this statically.

@@ -20,9 +17,11 @@ public class Anticipate {
* If last + period and a bit >= now, ask again after last + period and grace
*/

public static long next(long now, final long lastTimeStamp, final long period, final long grace) {
final long lag = Constants.SECOND_IN_MS * Pref.getStringToInt("nsfollow_lag", 0); // User can choose a wake delay with a 0 default.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this from inside Anticipate is probably a good idea

@jamorham
Copy link
Collaborator

Can't we just remove the lag handling from inside Anticipate and then add the lag on to the anticipated timestamp from the calling class rather than passing it as a parameter? Would that work?

@Navid200
Copy link
Collaborator Author

I will look into this.

My intent here was to keep everything working with Nightscout as Nightscout and Dex share use the same Anticipate method. So, I just brought the parameter out but kept everything unchanged.
I will look at this carefully to see if we can change it as you like.

@Navid200
Copy link
Collaborator Author

Can't we just remove the lag handling from inside Anticipate and then add the lag on to the anticipated timestamp from the calling class rather than passing it as a parameter? Would that work?

If I understand you correctly, we will then need to add the lag handling to multiple methods, Nightscout and Dex share and anything else that also uses Anticipate. Any change we may need to make to the mechanism will have to be applied multiple times to different sections.
I think it is best to have the mechanism inside Anticipate and used by all methods that need it.

@Navid200
Copy link
Collaborator Author

I'm sorry about the mistake.
I confirmed that indeed the delay was not being adjusted by the setting.
I changed the variable and made it non-static and confirmed with a test that the delay is adjusted by the setting now.

@Navid200
Copy link
Collaborator Author

This is ready for a review again now. I believe I have addressed your concern and thanks for the review and advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dexcom Share: Ability to Specify Seconds Until Polling Retry or Allow Double Poll Retry
2 participants