search

joeycastillo / Sensor-Watch

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

movement: add custom hourly chime tunes #209

Closed neutralinsomniac closed 1 year ago

neutralinsomniac commented 1 year ago

I’m re-submitting this. The more I think about it, the more this feels like something that makes more sense as a “core” feature instead of a watch face.

joeycastillo commented 1 year ago

Thanks for this PR! This actually helps get us to something I've been wanting Movement to do, which is to use the new watch_buzzer_play_sequence function instead of beeping and resting.

Can I ask you to do one more thing? Just because it would be a nice way to transition to this fully. Can you add a SIGNAL_TUNE_DEFAULT that just emulates the current behavior? That way I can merge this in and we can test the sequence-based hourly chime method a bit both with custom chimes and the default. If it works well, we can eventually remove the #ifdef and have one consistent method for both default and custom hourly chimes.

Thanks!

PieterJanSterk commented 1 year ago

@neutralinsomniac Won't it also make sense to add some sort of setting that turns these longer tunes off at night? Like, select a time in the morning to turn hourly chimes on, and one at night that turn them off? I know this is just a simple button press in standard time mode, but this way the watch can remain in low energy mode. I would to take a crack at this, based on the alarm watch face or an older version of this code. But I'm still learning C, I'm afraid it won't be much help. Just an idea of course, I love the concept of these custom chimes.

814d3 commented 1 year ago

I really like the idea of these tunes. What steps need to be done to merge the code? I see that the SIGNAL_TUNE_DEFAULT was added. Are there more tests necessary or is it just a lack of time? Thank and Greetings

joeycastillo commented 1 year ago

The lack of time is just me lacking the time to test this comprehensively; I usually merge watch faces in more quickly since they only affect the people who use those watch faces, but I'm more hesitant about changes that affect the core that everyone uses.

Still, I did test the basics, and if folks have been using this and not raising any issues for four months, I'm down to merge it in.

814d3 commented 1 year ago

Thank you very much for explaining and doing so. Just for curiosity, it should be possible to add a entry to the settings face, where you can choose the hourly chime, but you wouldn't support it as not everybody needs this feature, right? I'm just thinking about usability as I don't want to open the watch and flash the firmware that often :-).

edit: Maybe a general settings entry for defining a hourly chime period would be useful for many users. Then there also could be a selection of a different tune - just as an idea for engaged coding experts ;-)