Basic alarm app #662

mruss77 · 2021-09-11T14:46:09Z

I added a basic alarm app that allows you to set one alarm to go off once, every day, or on weekdays (M-F).

I know there is discussion in issue 366 about a full-featured alarm app with multiple alarms, which looks great. This basic alarm meets my simple need to wake up on time in the morning.

The UI is one screen based off the Timer app UI:

The plus/minus buttons set the hours and minutes.
The bottom-left button turns the alarm off/on, and becomes the "stop vibrating" button when the alarm goes off.
The bottom right button toggles between repeating alarm settings.
The "i" button will pop up a message showing how long until the alarm will go off.

Video of the alarm in action: https://www.youtube.com/watch?v=9U8QRy7Ixpw

I added a method to MotorController to activate the "ringer" (repeating vibrations) even if notifications are turned off in settings. I like to turn off notifications when I go to sleep so I'm not woken up by some random 3 AM email, but I still need the alarm to go off in the morning.

Any comments or suggestions welcome.

src/components/alarm/AlarmController.cpp

JF002 · 2021-09-11T19:08:57Z

Thank you very much for this PR, many people are requesting this app for a long time, it's more than welcome!
And also thank you for this very well documented PR (description, picture, video), it makes the review process much easier!

I know there is discussion in issue 366 about a full-featured alarm app with multiple alarms, which looks great. This basic alarm meets my simple need to wake up on time in the morning.

That's OK for me : we start with a first simple implement and we improve it later on!

At first sight, the code looks pretty good! There's just this minor detail: all method should start with a capital letter, even private ones. Ex : https://github.com/JF002/InfiniTime/pull/662/files#diff-f09dce847462bd3e0850bdd9ead3aef31f554a3736ee1d5378fa3e394d8f7de2R47

adocampo · 2021-09-11T21:46:29Z

Any comments or suggestions welcome.

Great! I miss this so much on PT. It's a great start and yet it would be cool to have more than one alarm, I must admit I usually have just one active most of the time. I know people who has plenty of them, but I could live with just one.

I have a couple of suggestions to improve the app without making it a full-featured one.

Put a snooze button. Without revamping the interface, perhaps moving the "OFF" function on the screen to the physical button and using the current OFF button to let the watch sleep for a 5, 10, or 15 minutes (perhaps tapping one, two or three times and showing the number in the button just like you did with WKDAYS, ONCE, DAILY text on the other button).
Use a more stressful vibration, I would suggest a melody, like reveille.

src/displayapp/screens/Alarm.cpp

src/displayapp/screens/Alarm.h

src/components/alarm/AlarmController.cpp

mruss77 · 2021-09-12T13:41:36Z

Thanks all for the comments and suggested fixes. Looks pretty straightforward.

"Size must be set before alignment" - thanks! I finally understand why the buttons were never where I expected them. Makes sense in hindsight.

mruss77 · 2021-09-12T13:56:46Z

@adocampo thanks for the feedback. Here's what I think on these features:

Multiple alarms - I think this is tough to do without the full UI design discussion in the other issue linked above. Also would be good to have persistent storage, since right now the alarm setting only lives in RAM and is lost on reboot. That would be more frustrating if you had several alarms set.
Snooze button - this seems very doable. Will work on it.
Stronger vibration / melody - hadn't thought of that but it looks like some of these motor functions are being worked on in add different virbrations tunes/patterns support as bzzzt for 50 or 35ms is boring #605. Can definitely look at that once it's done.

kieranc · 2021-09-12T17:20:49Z

Multiple alarms - I think this is tough to do without the full UI design discussion in the other issue linked above. Also would be good to have persistent storage, since right now the alarm setting only lives in RAM and is lost on reboot. That would be more frustrating if you had several alarms set.

The settings controller is very easy to interact with so you can store the alarms there - have a look at the steps goal settings app

Avamander · 2021-09-12T17:36:42Z

I'd keep the scope as it is right now and discuss improvements under #366. Discussing improvements there would help focusing on things in the scope of this PR and polishing it up.

JF002 · 2021-09-13T17:30:25Z

I agree, with @Avamander, let's keep the scope of the PR simple, so that we can have a first basic alarm app, and let's improve it in future PRs ;)

Itai-Nelken · 2021-09-13T18:20:10Z

I think that mon-fri and sun-thu is better then WKDAYS as not everyone uses the same first day of week.
maybe making it a "drop-down menu" like the one in the mertonome app to is will make it easier to select the option?

Avamander · 2021-09-13T19:23:37Z

@Itai-Nelken

I think that mon-fri and sun-thu is better then WKDAYS as not everyone uses the same first day of week.
maybe making it a "drop-down menu" like the one in the mertonome app to is will make it easier to select the option?

First day of week setting is very likely a future improvement.

src/components/alarm/AlarmController.cpp

src/components/alarm/AlarmController.h

src/displayapp/screens/Alarm.cpp

mruss77 · 2021-09-13T19:45:15Z

I think that mon-fri and sun-thu is better then WKDAYS as not everyone uses the same first day of week.
maybe making it a "drop-down menu" like the one in the mertonome app to is will make it easier to select the option?

To address this in the short term, I changed the label for the "weekdays" mode to "MON-FRI", so at least it's clear when the alarm will go off.

Thanks for bringing this up; I wasn't aware of how much variation there is around the world in the standard work-week. Of course, even in each region there are lots of people who don't work the standard work-week.

Avamander

All the qualms I had have been resolved. (But I very much invite others to take a look as well)

Riksu9000 · 2021-09-14T07:13:43Z

src/displayapp/screens/Alarm.cpp

+  lv_obj_set_size(btnHoursDown, 60, 40);
+  lv_obj_align(btnHoursDown, lv_scr_act(), LV_ALIGN_IN_LEFT_MID, 20, 35);
+  txtHrDown = lv_label_create(btnHoursDown, nullptr);
+  lv_label_set_text(txtHrDown, "-");


All of these lv_label_set_text functions can be changed to lv_label_set_text_static

Riksu9000 · 2021-09-14T07:17:34Z

src/displayapp/screens/Alarm.h

+        uint8_t alarmHours = 0;
+        uint8_t alarmMinutes = 0;


These get initialised to the AlarmController values in the constructor, so setting them to 0 here can be confusing.

Riksu9000 · 2021-09-14T07:18:14Z

src/components/alarm/AlarmController.h

+      uint8_t hours;
+      uint8_t minutes;


These are uninitialized, and should probably be set to around 7 o'clock by default instead of midnight.

Agree, initialized hours to 7 and minutes to 0

Riksu9000 · 2021-09-14T07:25:47Z

src/components/alarm/AlarmController.cpp

+void AlarmController::SetAlarm(uint8_t alarmHr, uint8_t alarmMin) {
+  hours = alarmHr;
+  minutes = alarmMin;
+  state = AlarmState::Set;
+  ScheduleAlarm();
+}
+


If I change the time without enabling the alarm, the new time won't be saved. Perhaps this function could just set the time, and ScheduleAlarm() could be made public.

Agreed - changed SetAlarm to SetAlarmTime and made ScheduleAlarm public.

mruss77 · 2021-09-14T12:19:23Z

I realized that StartRingingDisregardSettings() doesn't work, since RunForDuration also checks the notification setting and won't vibrate if notifications are disabled.

Maybe I should remove my changes to MotorController and wait for #664 - that looks like a better solution anyway.

JF002 · 2021-09-14T19:16:55Z

ettings() doesn't work, since RunForDuration also checks the notification setting and won't vibrate if notifications are disabled.

Maybe I should remove my changes to MotorController and wait for #664 - that looks like a better solution anyway.

I agree, we can handle the notifications/vibrations in #664

The-Linux-User · 2021-09-15T19:47:53Z

Would be really nice if there would be an icon on the watch face if the alarm is set
Looks great, I hope it will ve included in V 1.5

This reverts commit 1d43adc.

kieranc · 2021-09-17T10:45:10Z

I've tested this a little bit, it seems to work, the vibrations could be stronger but that's for another PR, and the alarm should definitely be stored in settings IMO, but that can also come later.

JF002 · 2021-09-18T15:16:02Z

Thanks again for this Alarm App, @mruss77!

For those interested, I can see some nice additions forfuture PRs:

Use FreeRTOS timers instead of timers from the NRF SDK (see Replace app_timer with FreeRTOS timers in TimerController #494)
Handle DST, Timezone,...
Save the timer settings in the Settings file
Support multiple alarms

adocampo · 2021-09-18T21:31:22Z

Great job @mruss77 !!

Hope InfiniTime devs can handle persistent storage for both time/date and alarms, because right now the bluetooth disconnects quite often and the only way to re-pair is rebooting the watch, so re-setting the alarm at each reboot would lead to an unreliable alarm system, as probably people can forget to set it again and... well, be late at work for this.

moriel5 · 2021-09-19T03:36:58Z

This is really great!
How would it handle 12h format, i.e. AM/PM?

mruss77 · 2021-09-19T13:05:30Z

@adocampo - thanks! Yeah, we definitely need to store the alarm. That'll be part of building the more fully functional alarm app.

@moriel5 - The alarm works fine with the 12-hour clock, but you have to set the time in 24-hour format (so if you want 5 PM, set it for 17:00). I agree it would be nice to use the 12-hour format on this screen if the user has selected that option, but it only affects the UI, not the functionality. Another to-do for the improved alarm app.

moriel5 · 2021-09-19T13:12:26Z

So pretty much as I had expected.

I have no qualms in setting it using 24h, 12h format is simply more comfortable to me (in my country, both are used interchangeably, 24h mostly when displayed, 12h mostly used verbally).

ncartron · 2021-09-20T10:24:47Z

@mruss77 it seems the alarm does not vibrate when the InfiniTime "DND" setting (notifications disabled) is on - I almost didn't wake up this morning because of that ;)
is that a known issue? I tried again just now, and with DND on -> no vibration, if I switch DND off, then the alarm vibrates correctly.

Apart from that, super nice job, I love the app!! <3

adocampo · 2021-09-20T11:19:25Z

Sorry if this is not the right place to make this question, but as I'm interested in testing this app, I will ask and I expect someone to point in the right direction.

How can I install this on a sealed PT without waiting for the firmware upgrade?

Avamander · 2021-09-20T11:28:22Z

@adocampo You can build your own firmware package, documentation is available in the repository.

mruss77 · 2021-09-20T12:08:47Z

@ncartron - yes, sorry about that but it's a known issue. That is being fixed in #664. Currently the DND setting disables all vibrations from all apps, including the alarm, metronome, timer, etc. That update will set it to disable only notifications.
EDIT - looks like #664 has now been merged, so if you create a new build from develop you should get the fix!

ncartron · 2021-09-20T12:45:48Z

right - thanks for the update @mruss77 , and noted!

mruss77 · 2021-09-20T13:06:55Z

@ncartron I was mistaken, #664 has not been merged yet. I was looking in the comments at another referenced PR that has been merged. #664 is still open.

ncartron · 2021-09-20T14:55:22Z

ok - i'll see with @JF002 what the plans are :)

mruss77 · 2021-09-23T20:09:09Z

@ncartron - the change to DND has been merged this afternoon!

ncartron · 2021-09-24T07:27:50Z

yup, thanks @mruss77 - JF told me already ;)

medeyko · 2021-12-16T19:29:59Z

I think there's an easy way to make several alarms - just switch, say, three identical screens by horizontal swipes. It shouldn't take too much code, but handy when you have several things not to forget.

Created basic alarm app

1fb5757

Avamander requested changes Sep 11, 2021

View reviewed changes

src/components/alarm/AlarmController.cpp Outdated Show resolved Hide resolved

JF002 added this to the 1.5.0 milestone Sep 11, 2021

Riksu9000 mentioned this pull request Sep 12, 2021

Toggle notifications only, keep vibrations. #664

Merged

Riksu9000 suggested changes Sep 12, 2021

View reviewed changes

Fixes based on code reviews (formatting, UI code)

bfe13d9

Avamander requested changes Sep 13, 2021

View reviewed changes

License header fix, add missing braces

2bf339a

Avamander approved these changes Sep 13, 2021

View reviewed changes

Riksu9000 suggested changes Sep 14, 2021

View reviewed changes

mruss77 added 4 commits September 16, 2021 15:38

Fixes per Riksu9000's feedback

f857a75

Merge upstream

1d43adc

Revert "Merge upstream"

cdf99b0

This reverts commit 1d43adc.

resolve conflict in src/systemtask/Messages.h

b9e5684

JF002 merged commit 9cd0def into InfiniTimeOrg:develop Sep 18, 2021

schultzryan mentioned this pull request Sep 23, 2021

Add ability to set time if not connected to phone #687

Closed

Basic alarm app #662

Basic alarm app #662

Conversation

mruss77 commented Sep 11, 2021

JF002 commented Sep 11, 2021

adocampo commented Sep 11, 2021 • edited Loading

mruss77 commented Sep 12, 2021

mruss77 commented Sep 12, 2021

kieranc commented Sep 12, 2021 • edited Loading

Avamander commented Sep 12, 2021

JF002 commented Sep 13, 2021

Itai-Nelken commented Sep 13, 2021

Avamander commented Sep 13, 2021 • edited Loading

mruss77 commented Sep 13, 2021

Avamander left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruss77 commented Sep 14, 2021

JF002 commented Sep 14, 2021

The-Linux-User commented Sep 15, 2021

kieranc commented Sep 17, 2021 • edited Loading

JF002 commented Sep 18, 2021

adocampo commented Sep 18, 2021

moriel5 commented Sep 19, 2021

mruss77 commented Sep 19, 2021

moriel5 commented Sep 19, 2021

ncartron commented Sep 20, 2021

adocampo commented Sep 20, 2021

Avamander commented Sep 20, 2021

mruss77 commented Sep 20, 2021 • edited Loading

ncartron commented Sep 20, 2021

mruss77 commented Sep 20, 2021

ncartron commented Sep 20, 2021

mruss77 commented Sep 23, 2021

ncartron commented Sep 24, 2021

medeyko commented Dec 16, 2021

adocampo commented Sep 11, 2021 •

edited

Loading

kieranc commented Sep 12, 2021 •

edited

Loading

Avamander commented Sep 13, 2021 •

edited

Loading

Avamander left a comment •

edited

Loading

kieranc commented Sep 17, 2021 •

edited

Loading

mruss77 commented Sep 20, 2021 •

edited

Loading