Fix buzzer in LE mode.

[WesleyAC]

Fixes: #275

[WesleyAC]

Here's the diff I used to test this, FYI, which sets the default LE timeout to 30 seconds and makes the hourly chime a minutely chime instead.

diff --git a/movement/movement.c b/movement/movement.c
index 0e40d46..fd4af54 100644
--- a/movement/movement.c
+++ b/movement/movement.c
@@ -76,7 +76,7 @@
 movement_state_t movement_state;
 void * watch_face_contexts[MOVEMENT_NUM_FACES];
 watch_date_time scheduled_tasks[MOVEMENT_NUM_FACES];
-const int32_t movement_le_inactivity_deadlines[8] = {INT_MAX, 3600, 7200, 21600, 43200, 86400, 172800, 604800};
+const int32_t movement_le_inactivity_deadlines[8] = {INT_MAX, 30, 7200, 21600, 43200, 86400, 172800, 604800};
 const int16_t movement_timeout_inactivity_deadlines[4] = {60, 120, 300, 1800};
 movement_event_t event;

diff --git a/movement/watch_faces/clock/simple_clock_face.c b/movement/watch_faces/clock/simple_clock_face.c
index fbc2c4b..2f4b076 100644
--- a/movement/watch_faces/clock/simple_clock_face.c
+++ b/movement/watch_faces/clock/simple_clock_face.c
@@ -157,5 +157,5 @@ bool simple_clock_face_wants_background_task(movement_settings_t *settings, void

     watch_date_time date_time = watch_rtc_get_date_time();

-    return date_time.unit.minute == 0;
+    return date_time.unit.second == 0;
 }

[theAlexes]

we're also building a watch with this merged in, for testing.

[sntyj-dev]

Reporting in. Not sure how long this needs to sit in order to be stable but...I've been running this code pretty much since the closure of #327 without issue. I am surprised it's not already merged into the main branch.

[WesleyAC]

The reason this hasn't been merged yet is because I don't want to merge my own changes without review, and no one else has reviewed it yet — but i agree it'd be good to get this fixed given how significant a bug it is. I'll ping @joeycastillo, and if he doesn't get to it soon, I'll just go ahead and merge this, since I agree that it's important to fix.

[theorangerider]

So I cherry-picked this PR and last evening my watch got in a state where it wasn't in LE mode, but the button noises weren't working and neither was the light. It could be completely unrelated, but I figured I'd mention it in case you think the "hack" of blocking could have somehow caused this.

I set my watch aside and it somehow auto-resolved itself by the morning.

Other than that the patch is working quite well, I appreciate it!

[theAlexes]

that is also the behavior we're getting with this patch, but there's a few tricks left up our sleeve that we wanna try...

[theAlexes]

question: what's stopping us, in LE mode, from setting movement_state.needs_wake to true, and putting a reasonable count of ticks necessary for the beeper to sound in movement_state.le_mode_ticks?

[theAlexes]

we have an earlier version of this diff (one that doesn't use is_buzzing, and has a hardcoded le_mode_ticks) running on our watch and it has much improved behavior around sleep mode —‌ the watch does wake up for a second, but then it goes right back to sleep.

diff --git a/movement/movement.c b/movement/movement.c
index 0e40d46..8c71a09 100644
--- a/movement/movement.c
+++ b/movement/movement.c
@@ -293,31 +293,32 @@ void movement_request_wake() {
     _movement_reset_inactivity_countdown();
 }

+void end_buzzing() {
+    movement_state.is_buzzing = false;
+}
+
+void end_buzzing_and_disable_buzzer(void) {
+    end_buzzing();
+    watch_disable_buzzer();
+}
+
 void movement_play_signal(void) {
-    watch_enable_buzzer();
-    watch_buzzer_play_sequence(signal_tune, watch_disable_buzzer);
+    void *maybe_disable_buzzer = end_buzzing_and_disable_buzzer;
+    if (watch_is_buzzer_or_led_enabled()) {
+        maybe_disable_buzzer = end_buzzing;
+    } else {
+        watch_enable_buzzer();
+    }
+    movement_state.is_buzzing = true;
+    watch_buzzer_play_sequence(signal_tune, maybe_disable_buzzer);
     if (movement_state.le_mode_ticks == -1) {
-   // This is somewhat of a hack. In order to play a sequence, we need to
-   // be awake. We should ideally be able to tell movement that we need to
-   // be awake for a given amount of time, but there's no good way to do
-   // this, so we block instead. This might be bad in the case that a
-   // watch face has housekeeping to do after calling this, since it could
-   // in theory do that housekeeping concurrently, but alas.
-   //
-   // You might wonder, why not just put the instruction to go back to
-   // sleep in the callback? It's a good idea, but I can't figure out how
-   // to get it to work - you're basically kicking the can down the road,
-   // since at some point movement will be done doing what it's doing and
-   // have to wait. At that point, you're delaying anyways, but it's
-   // harder to figure out how much time to delay for, since you don't
-   // know how much time has elapsed since starting the sequence. I'd
-   // rather this block than have to read from the RTC to figure that
-   // out.
-   //
-   // Don't ask me what the +50ms is doing. The sequence gets cut short
-   // with the exact time, I have no idea why. 50 extra millisecons seems
-   // like a safe value.
-        delay_ms(sequence_length(signal_tune) * 1000 / 64 + 50);
+        movement_state.needs_wake = true;
+        // le_mode_ticks are one per second; sequence_length counts 128ths of a
+        // second. this gives us one second of awake time to start with, and
+        // then shifting away the fractional bits away ought to be alright - if
+        // the sequence is 129/128ths of a second long, this will tick 2
+        // seconds.
+        movement_state.le_mode_ticks = (128 + sequence_length(signal_tune)) >> 7;
     }
 }

[joeycastillo]

question: what's stopping us, in LE mode, from setting movement_state.needs_wake to true, and putting a reasonable count of ticks necessary for the beeper to sound

this is a really clever idea, and I see no reason it wouldn’t “just work”

[theAlexes]

I've gone and updated my diff with the correct shifting factor to make it sleep an appropriate amount for the sequence at hand, and flashed it onto one of our watches. Works well enough so far, we'll see how it fares overnight.

[theAlexes]

so i think this trick is mostly working, but whether or not the sequence finishes is dependent on the subsecond; if the interrupt fires closer to the end of an le_mode_tick than its start, the sound will shut off early.

we've got one more trick up our sleeve to try: make the Movement main loop block at the end until is_buzzing stops.

(this is where an "OS layer" between Movement and the hardware would come in handy, to arbitrate the various uses of the buzzer)

[theAlexes]

here's a revised patch against this branch, which is cooking on our watch overnight (it would drive our housemates mad if we left a minutely-beeping watch in action for more than a few minutes)

diff --git a/movement/movement.c b/movement/movement.c
index 0e40d46..31b9c4d 100644
--- a/movement/movement.c
+++ b/movement/movement.c
@@ -293,31 +293,31 @@ void movement_request_wake() {
     _movement_reset_inactivity_countdown();
 }

+void end_buzzing() {
+    movement_state.is_buzzing = false;
+}
+
+void end_buzzing_and_disable_buzzer(void) {
+    end_buzzing();
+    watch_disable_buzzer();
+}
+
 void movement_play_signal(void) {
-    watch_enable_buzzer();
-    watch_buzzer_play_sequence(signal_tune, watch_disable_buzzer);
+    void *maybe_disable_buzzer = end_buzzing_and_disable_buzzer;
+    if (watch_is_buzzer_or_led_enabled()) {
+        maybe_disable_buzzer = end_buzzing;
+    } else {
+        watch_enable_buzzer();
+    }
+    movement_state.is_buzzing = true;
+    watch_buzzer_play_sequence(signal_tune, maybe_disable_buzzer);
     if (movement_state.le_mode_ticks == -1) {
-   // This is somewhat of a hack. In order to play a sequence, we need to
-   // be awake. We should ideally be able to tell movement that we need to
-   // be awake for a given amount of time, but there's no good way to do
-   // this, so we block instead. This might be bad in the case that a
-   // watch face has housekeeping to do after calling this, since it could
-   // in theory do that housekeeping concurrently, but alas.
-   //
-   // You might wonder, why not just put the instruction to go back to
-   // sleep in the callback? It's a good idea, but I can't figure out how
-   // to get it to work - you're basically kicking the can down the road,
-   // since at some point movement will be done doing what it's doing and
-   // have to wait. At that point, you're delaying anyways, but it's
-   // harder to figure out how much time to delay for, since you don't
-   // know how much time has elapsed since starting the sequence. I'd
-   // rather this block than have to read from the RTC to figure that
-   // out.
-   //
-   // Don't ask me what the +50ms is doing. The sequence gets cut short
-   // with the exact time, I have no idea why. 50 extra millisecons seems
-   // like a safe value.
-        delay_ms(sequence_length(signal_tune) * 1000 / 64 + 50);
+        // the watch is asleep. wake it up for "1" round through the main loop.
+        // the sleep_mode_app_loop will notice the is_buzzing and note that it
+        // only woke up to beep and then it will spinlock until the callback
+        // turns off the is_buzzing flag.
+        movement_state.needs_wake = true;
+        movement_state.le_mode_ticks = 1;
     }
 }

@@ -450,6 +450,7 @@ static void _sleep_mode_app_loop(void) {
 }

 bool app_loop(void) {
+    bool woke_up_for_buzzer = false;
     if (movement_state.watch_face_changed) {
         if (movement_state.settings.bit.button_should_sound) {
             // low note for nonzero case, high note for return to watch_face 0
@@ -493,7 +494,11 @@ bool app_loop(void) {
         // _sleep_mode_app_loop takes over at this point and loops until le_mode_ticks is reset by the extwake handler,
         // or wake is requested using the movement_request_wake function.
         _sleep_mode_app_loop();
-        // as soon as _sleep_mode_app_loop returns, we reactivate ourselves.
+        // as soon as _sleep_mode_app_loop returns, we prepare to reactivate
+        // ourselves, but first, we check to see if we woke up for the buzzer:
+        if (movement_state.is_buzzing) {
+            woke_up_for_buzzer = true;
+        }
         event.event_type = EVENT_ACTIVATE;
         // this is a hack tho: waking from sleep mode, app_setup does get called, but it happens before we have reset our ticks.
         // need to figure out if there's a better heuristic for determining how we woke up.
@@ -575,8 +580,13 @@ bool app_loop(void) {
     // if the watch face changed, we can't sleep because we need to update the display.
     if (movement_state.watch_face_changed) can_sleep = false;

-    // if the buzzer or the LED is on, we need to stay awake to keep the TCC running.
-    if (movement_state.is_buzzing || movement_state.light_ticks != -1) can_sleep = false;
+    // if we woke up for the buzzer, stay awake until it's finished.
+    if (woke_up_for_buzzer) {
+        while(watch_is_buzzer_or_led_enabled());
+    }
+
+    // if the LED is on, we need to stay awake to keep the TCC running.
+    if (movement_state.light_ticks != -1) can_sleep = false;

     return can_sleep;
 }

[theorangerider]

Thanks, @theAlexes. I just put it on my watch to try it out too

[underscorephil]

I put this fix on my watch today. Will report back if I run into any issues.

[theorangerider]

Seems to be working for me so far

[underscorephil]

Working well for me. No issues.

[sntyj-dev]

I'll have a chance to flash and test both the branch with the patch this weekend. I've replicated @theorangerider 's issue a few times since I applied this code initially.

[sntyj-dev]

So far, running the latest patch against this pull request works well. I can keep the buzzer on, and it hasn't caused a situation where the watch light is unresponsive if it's left on. Thanks again everyone!

Hopefully the situation hasn't changed for others who have already tested.

I'm concerned that the patch isn't being captured in this pull request though (it is in the comments but not commits obviously). I apologize since I'm not trying to sound demanding or anything, since I'm a nobody and not actually contributing code. I also don't want to come across like I want to be a project manager. I'm just an excited user to have this fixed, and look forward to having it in the main branch.

But it'd be cool to have everyone's work merged into a branch and ready to go once it's deemed stable.

[theAlexes]

@WesleyAC could apply our patch and use git commit --author 'Alex Maestas <git@se30.xyz>' — that would be the easiest way to apply our work while still giving us credit.

[WesleyAC]

@theAlexes done! let me know if you would prefer a different commit message.

i haven't tested this, but it sounds like enough other people have that it should be good to merge, @joeycastillo any thoughts on that? if not i will go ahead and do it soon.

[theAlexes]

that commit message looks good to me! 👍

[WesleyAC]

Going ahead and merging. Please speak up if anyone experiences any problems with this!