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

Fix run notifications bugs and improve accuracy #1619

Merged
merged 1 commit into from Mar 25, 2022

Conversation

GrandVizierOlaf
Copy link
Contributor

This fixes a few issues with the run app, mostly ones I introduced with the notifications, but not exclusively. It also removes an unnecessary variable assignment in the recorder app.

  • Don't try and enable/disable recording if the recorder widget is unavailable
  • Don't configure notifications for stats which do not have a box enabled
  • Look up the vibTimes rather than the vibPatterns to determine the current notification configuration
  • Use the previous notification state to determine the next state, rather than the current stat (decreases notification drift)
  • Reset GPS state in reset to prevent distance miscalculation when stopping one run and starting another

@gfwilliams
Copy link
Member

Thanks! Does the GPS state thing really help to fix a problem though?

It seems that because we measure distance as that between the current and last period, setting it to {} may actually have the effect of skipping out one second's worth of distance?

@gfwilliams gfwilliams merged commit 4d1cf76 into espruino:master Mar 25, 2022
@GrandVizierOlaf
Copy link
Contributor Author

Thanks! Does the GPS state thing really help to fix a problem though?

It seems that because we measure distance as that between the current and last period, setting it to {} may actually have the effect of skipping out one second's worth of distance?

If you stop a run and start another it would have the old GPS state and if you had moved at all it could cause the initial distance calculation of the second run to be wrong by the distance you had traveled between stopping and starting.

@gfwilliams
Copy link
Member

Do you actually see this happening though? Or is it just what you think might be an issue?

Because I believe GPS is on all the time so those should update every second regardless of whether you have started the run or not.

@GrandVizierOlaf
Copy link
Contributor Author

I see what you're saying and it might have then been a coincidence because I was having trouble with the initial distance on a second run and then it went away after making that change. Perhaps I had a better GPS fix by that point, though it had been around 30 minutes of consistent testing by then. I had assumed that when run told the recorder widget to setRecording(false) it would stop GPS updates to exstats too, but see now that wouldn't be the case because exstats is doing its own setGPSPower.

If the GPS is always on with exstats then does that mean leaving the watch on the run app screen will run the battery out faster than not having the app open, even when not running? The run app is my primary use of the watch and I leave it open a lot, even between runs.

@gfwilliams
Copy link
Member

It looks like the GPS 'fix' likely caused this issue? #1630

If the GPS is always on with exstats then does that mean leaving the watch on the run app screen will run the battery out faster than not having the app open, even when not running?

Yes, that's the case. But it's important the GPS is turned on as soon as possible, because it can take a while to get a lock. Last thing you want it to start recording and then have it miss the first 10 minutes of your run

@GrandVizierOlaf
Copy link
Contributor Author

@gfwilliams I went back and tested and retested and could not duplicate the issue that caused me to introduce this breaking change, instead confirming the GPS fix is updating nearly once per second as you said, regardless of whether a run is active. Would you like me to revert it and/or the change from #1630 or leave things as-is?

@gfwilliams
Copy link
Member

Thanks for checking into this! I've just removed the ={} bits - I guess #1630 is probably a good thing to have in anyway as maybe there's a case where you start the app and then start the run within 1 sec of boot - and you could hit the issue in that case :)

gfwilliams added a commit that referenced this pull request Mar 29, 2022
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.

None yet

2 participants