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

Use slower "fast ticks" (8 Hz instead of 128 Hz) to detect button press and light duration #294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alesgenova
Copy link

Every time a button is pressed, the light turns on, or a sound is playing, Movement enters a 128 Hz fast tick mode.

The cb_fast_tick() function is called 128 times per second. In this function we do among other things some checks on the time a button is held down or the amount of time left on an alarm playback.

With this PR I propose to lower the frequency of the fast tick mode to 8 Hz (the frequency is adjustable by changing FAST_TICK_FREQUENCY). This way we will reduce the invocation of cb_fast_tick() by over an order of magnitude.

We potentially lose a little bit of accuracy (up to 0.125s at 8 Hz) in the amount of time needed for a button longpress (0.5s +/- 0.125), or the light duration (1s +/- 0.125), but I think this inaccuracies are not noticeable.

Another consequence of this PR is that the 128 Hz mode is no longer reserved by Movement, and any face can now request this tick frequency.

@wryun
Copy link
Contributor

wryun commented Oct 15, 2023

👍 seems like a good thing to me.

@wryun
Copy link
Contributor

wryun commented Dec 23, 2023

I've been using this on my watch without issue.

Copy link
Collaborator

@matheusmoreira matheusmoreira left a comment

Choose a reason for hiding this comment

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

Do any of the maintainers have any concerns over an 8 Hz fast tick as opposed to 128 Hz?

The code itself looks good to me. @wryun has tested this on hardware so this PR can be tagged tested-on-hw and he can be credited on the merge commit with a git trailer:

Tested-by: James Haggerty (@wryun) ...

Can also be used to credit reviewers and maintainers who signed off on it:

Reviewed-by: Matheus ...
Reviewed-by: Other reviewers ...
Signed-off-by: Maintainer ...

@@ -22,7 +22,8 @@
* SOFTWARE.
*/

#define MOVEMENT_LONG_PRESS_TICKS 64
#define FAST_TICK_FREQUENCY 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be surrounded in #ifndef to allow the user to override FAST_TICK_FREQUENCY. I also suggest a prefixing on the symbol to avoid namespace clashes.

@wryun
Copy link
Contributor

wryun commented Jul 20, 2024

As a continuing data point, this has been fine for me for 6 months.

@matheusmoreira matheusmoreira added enhancement New feature or request tested-on-hw This feature or pull request has been tested on a physical watch labels Sep 7, 2024
@matheusmoreira
Copy link
Collaborator

@voloved Do you think this will impact debouncing in any way?

@matheusmoreira matheusmoreira added the 2.0-wait-list This feature or pull requests has been deferred until the movement 2.0 refactor is complete label Sep 7, 2024
@voloved
Copy link
Contributor

voloved commented Sep 8, 2024

@voloved Do you think this will impact debouncing in any way?

It would. This would now make the lowest reliable debounce time be a quarter second, which is too high for debounce. Though there are other ways to debounce, such as creating a new timer each time the button is pressed and killing it once the debounce time is over.

Using 8 Hz also causes any watch face that uses 8Hz to also need to be modified to not use 8Hz. Though they can be moved to 16Hz and checked if their subec mod 8 is zero.

What is the benefit of making this change to begin with? Does it save battery? Is the battery savings noticeable on the watch's life?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0-wait-list This feature or pull requests has been deferred until the movement 2.0 refactor is complete enhancement New feature or request tested-on-hw This feature or pull request has been tested on a physical watch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants