search

joeycastillo / Sensor-Watch

A board replacement for the classic Casio F-91W wristwatch
Other
1.17k stars 234 forks source link

hourly chime not working in standby mode #275

Closed Niehztog closed 8 months ago

Niehztog commented 1 year ago

Some time ago I flashed a new firmware to my sensor watch built directly from the up-to-date main branch. Since then the hourly chime is not working in standby mode. My watches buzzer is working fine, so it is not related to that. It must have been some of the commits from the last few months which broke the feature.

Can anyone running a more resent built of movement on his watch confirm this?

CarpeNoctem commented 1 year ago

Hmm. I'm running a pretty fresh build. I'll enable the chime and comment back here later today.

CarpeNoctem commented 1 year ago

I can confirm that my watch is missing these hourly chimes in LE mode as well. I'll check my other watch when I get home. Its firmware was compiled (by me) from source as well, but sometime earlier. If it doesn't have the issue, that could help narrow down when the issue crept in.

CarpeNoctem commented 1 year ago

So I haven't made it home yet, but did another check out of curiosity. I set an alarm for a time during LE mode. Paying closer attention the second time, I confirmed that it actually wakes the watch up out of LE mode into normal mode. I'm not sure whether or not this used to be the case for the hourly chime, but may be worth noting.

fedpost commented 1 year ago

I have had this issue for a while. It has been intermittent and I haven't cared enough to investigate or give it any further thought, but I can confirm it happens with my build as well.

Niehztog commented 1 year ago

Commit c1580b356d16cc49db873b020d72465a5f8b3a45 replaced the function call that plays the hourly chime from watch_buzzer_play_note with watch_buzzer_play_sequence. watch_buzzer_play_note works in low energy mode, watch_buzzer_play_sequence obviously doesn't. (I tested that on my watch by reverting the change and the hourly chime worked again in low energy mode).

@TheOnePerson and @neutralinsomniac , does maybe one of you have an idea how we can get the beautiful custom hourly chime tunes from #209 working in low energy mode?

TheOnePerson commented 1 year ago

As far as I know does neither of watch_buzzer_play_note or watch_buzzer_play_sequence work in LE mode. Just from looking at the code:

The changes in the internal logic of movement_play_signal have led to tunes being played asynchronously. Consequently, the function returns immediately after initializing the buzzer, rather than waiting for the tune to finish playing. This means that the implementation here in simple_clock_face.c does not work as expected any more. When an hourly chime is to be played while the watch is in LE mode, it first enables the buzzer (which is good), initializes the sequence - and immediately after that the buzzer is disabled again (which is bad).

My take on this problem would be:

a) Restore movement_play_signal() to its former version, using watch_buzzer_play_note again. This approach would also maintain compatibility with other watch faces that rely on this function (e.g., tomato_face.c or rpn_calculator_alt_face.c). (However, we could consider whether playing a more elaborate hourly chime is appropriate in these contexts.)

b) Implement the hourly chime sequence directly in simple_clock_face.c, utilizing the callback functionality of watch_buzzer_play_sequence to control when the buzzer is disabled. This approach may provide more control over the chime's behavior and timing, ensuring it works as expected in LE mode.

What do you think, @neutralinsomniac ?

neutralinsomniac commented 1 year ago

How about this approach? https://github.com/joeycastillo/Sensor-Watch/pull/283

I kinda like this since it pulls the sleep-specific code into its own functions for easier use by the watch faces (they don't have to manually enable/disable the buzzer). I left the tunes code in movement instead of moving it into simple clock face so that other watch faces can opt-in to playing hourly tunes if they want, but I restored movement_play_signal() (and created a corresponding movement_play_signal_background() for LE use) and pulled the tune code into two new functions: movement_play_tune() and movement_play_tune_background() as well.

Also, apologies to anyone affected by this bug. I should've caught it a long time ago.

TheOnePerson commented 1 year ago

I like your approach, too @neutralinsomniac . This looks fine to me (haven't had a chance to test it yet).

neutralinsomniac commented 1 year ago

Thinking about this further, what if we moved the watch_is_buzzer_or_led_enabled() call and appropriate buzzer enabled/disable logic into movement_play_signal() and movement_play_tune() so that these functions always "just work" and don't require a decision to be made in every watch face before calling these library functions?

neutralinsomniac commented 1 year ago

I pushed this change into my MR if you want to see what it looks like

synthead commented 11 months ago

How about this approach? #283

I kinda like this since it pulls the sleep-specific code into its own functions for easier use by the watch faces (they don't have to manually enable/disable the buzzer). I left the tunes code in movement instead of moving it into simple clock face so that other watch faces can opt-in to playing hourly tunes if they want, but I restored movement_play_signal() (and created a corresponding movement_play_signal_background() for LE use) and pulled the tune code into two new functions: movement_play_tune() and movement_play_tune_background() as well.

Also, apologies to anyone affected by this bug. I should've caught it a long time ago.

I have built and flashed my red board with the latest main at 5c94111ea20e50cb9dab8f416603403185e933b3, and I can still reproduce this bug. I have SIGNAL_TUNE_ZELDA_SECRET defined in movement/movement_config.h, if it matters.

814d3 commented 11 months ago

I can confirm, that the SIGNAL_TUNE_DEFAULT is not working, when the watch is in low energy mode. I am using the latest (67be6af) version. Only two short clicks are recognizable, but no signal. I did not test timers or the alarm sound, when in LE mode, but probably they won't work either. Greetings

Edit: Timers and alarm sounds are working well. So the behavior is limited to the hourly sound.

WesleyAC commented 10 months ago

My PR above fixes this for the default tune by using the legacy functions, but I started looking into fixing this for custom tunes as well. The gist of it is that the main problem is that we are immediately turning the TCC off, because watch_buzzer_play_sequence is async, so as soon as we call that, the watch face exits its loop and calls watch_enter_sleep_mode which calls _watch_disable_all_peripherals_except_slcd which calls _watch_disable_tcc. The fix is probably for movement to know when a sequence has been requested to be played, and in that case, turn off all peripherals except slcd and tcc.

However, even doing that doesn't get it quite working — when I ensure the TCC is enabled the whole time, the tune still sounds quiet and muted. My phone mic can't really pick it up well, but https://hack.wesleyac.com/tune_sleep_mode.aac and https://hack.wesleyac.com/tune_no_sleep_mode.aac show the difference. My guess is some kind of clock scaling difference but I need to track it down.

maxz commented 9 months ago

This issue still persists (or returned) in the current head of the main branch (Commit 63d6bc6aa0ddf4cc1ce1918ef7650852a25e581b.)

WesleyAC commented 9 months ago

@maxz that's because the PR fixing it (#325) hasn't been merged in yet. should be very soon, but you can grab the code from that PR if you want a fix now.

maxz commented 9 months ago

Ah, thank you for the update.

I was not sure about the current state with various reverts I encountered when trying to check what's actually merged right now and already though about band-aid fixing it myself in my local copy.