search

InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.71k stars 926 forks source link

Alarms sometime do not trigger #1086

Closed brabalan closed 2 years ago

brabalan commented 2 years ago

Verification

What happened?

A repeating alarm did not ring

What should happen instead?

It should have rung

Reproduction steps

More details?

This seems related to #774 but it does not seem to be about time change (unless something special happens at midnight).

My observations are:

Version

v1.9.0

Companion app

GadgetBridge v.0.66.0

rev22 commented 2 years ago

I've tried to reproduce the issue reported by using the Set Time function in the Settings, by using the following steps:

Asdew95 commented 2 years ago

I can reproduce the issue with a non-repeating alarm, not passing midnight.

  1. Disconnect or disable Bluetooth.
  2. Set a non-repeating alarm to a few minutes from now.
  3. Set the time to a minute before the alarm should go off.

Before the alarm should go off after setting the new time, the time to alarm is correct. It becomes tens of thousands of days (it always seems to become around 49710 days and 6 hours) after the alarm should have gone off. I can also reproduce this by not setting the time manually but having the watch connected to Gadgetbridge (v0.66.0) when setting the alarm and reconnecting from Gadgetbridge after the alarm is set.

medeyko commented 2 years ago

I opened #1088, and I'm not sure it is a duplicate of this one.

The alarms I missed seems to have nothing with this technical issue. Because all these five times when I missed the alarms, I din't do any special actions. I just set up the single, not repeating alarm (after midnight, so midnight is not an issue) and immediately came to sleep (not playing with Gadgetbridge, etc) and then wasn't alerted by the alarm.

On the other hand, I tried to set up an alarm and check it being alert, and it vibrated as planned.

So it seems to be two separate issues. One is a technical bug, and another one is a user experience problem.

rev22 commented 2 years ago

One potential issues I noticed while eyeing the differences between 1.8.0 and 1.9.0 is this:

However I couldn't test this possibility yet.

Riksu9000 commented 2 years ago

I tried reproducing this earlier, and I could get it to happen a few times, but later I could no longer reproduce it and I couldn't figure out why. I'm slightly suspicious of app_timers. They're also used in MotorController to stop the motor, but the motor sometimes fails to stop as well.. #724

rev22 commented 2 years ago

@Riksu9000 suspicions confirmed, indeed turning alarmAppTimer into a FreeRTOS timer solved the problem, at least for my test method I mentioned yesterday. I haven't made a pull request yet but the modification I tested is here https://github.com/rev22/InfiniTime/commit/6e1bddcd639da1501d0eed8eba87cc8b69586e2f

rev22 commented 2 years ago

While testing the removal of "apptimer*" from the code base, I noticed that app_timer_init is called twice, in

MotorController::Init SystemTask::Work

Perhaps the double initialization is causing unreliable operation?

mruss77 commented 2 years ago

I missed an alarm this morning after upgrading InfiniTime from 1.6.0 to 1.9.0 yesterday. The alarm was working well on 1.6.0.

Some others seem to say it worked well on 1.8.0 so it may be worth looking at those recent changes.

At one point @geekbozu suggested that instead of relying on a timer, we should just have just have SystemTask or some other task wake up once a minute to compare the current date & time to any set alarms. This could help avoid bugs involving time changes, and potentially make it easier to support multiple alarms in the future.

If the FreeRTOS timer approach works well, that's great. I attempted to make that conversion myself at one point but I was doing something wrong and kept crashing InfiniTime.

Itai-Nelken commented 2 years ago

I think the problem with the alarm in 1.9.0 is that it stops ringing after a minute which isn't enough to wake everyone. This was changed in #945. I suggest habing a "ring time" setting for the alarm.

medeyko commented 2 years ago

@Itai-Nelken Indeed, I agree, limiting the ringing time to 60 seconds in order to avoid ringing when the watch is not worn looks like is not optimal decision. I think it should be redesigned (for example, "not worn" state should only be active if no any movement has been detected last 20 minutes) or indeed there should be a setting for stone-proof alerting for those who wishes to be sure in the alarm reliability.

However, I have looked at the source code https://github.com/InfiniTimeOrg/InfiniTime/pull/945/files and I guess there may be another cause why the alarm may not wake the user up. The problem could be changes made to the Alarm::OnButtonEvent function: it doesn't check the event type when calling StopAlerting(), and there may be some unintended event.

brabalan commented 2 years ago

One symptom of the alarm not ringing that can be observed after the fact is to check if the alarm is still planned (for a "once" alarm) and the time till it should trigger. If it’s many days in the future, it means it did not ring.

I’ve had "once" alarm not ring when they were for 7:00. I’ve been setting them to 06:58 for a couple days and they all rang.

Riksu9000 commented 2 years ago

This issue seems to be caused by calling app_timer_start() soon after app_timer_stop() when the timer is started before either function. What happens is that when app_timer_stop() is called, a stop command is sent to the timer task. Next, app_timer_start() is called before the timer task has had the opportunity to process the command. app_timer_start() checks directly if the timer is currently running, and doesn't do anything if it is. The issue is that since the timer task hasn't yet processed the command, app_timer_start() sees that the timer's running and doesn't do anything, and soon after the timer task processes the command and stops the timer, making it so the alarm never rings.

This can be solved by raising the timer priority to force a task switch as soon as a timer command is sent, or removing app_timers to avoid the issue entirely, which is my preferred solution.

medeyko commented 2 years ago

@Riksu9000 But why this bug was not present or was rare earlier? As far as I see, your description suggests that the problem was in 1.6.0 vesion too?

Riksu9000 commented 2 years ago

The issue happens when the alarm is rescheduled, and it was never rescheduled on time change before 1.9.0. Gadgetbridge syncs the time on reconnect, which will cause a rescheduling. This explains how this issue can happen seemingly at random and when manually changing the time. Also the next time the alarm is rescheduled, app_timer_stop() is skipped, and app_timer_start() actually successfully starts the timer, so every other time the alarm will work.

j2g2rp commented 2 years ago

I found a way to reproduce the bug. I almost always use the watch without companion app so I have the bluetooth disabled but from time to time I connect it with Siglo on my PC. Steps to reproduce: -Turn on bluetooth -Change screen on to 30 sec (I usually have it to 5 secs but I increase it to avoid screen off while I connect to siglo so maybe it is irrelevant) -Connect with siglo -Close siglo -turn off bluetooth

If you go to alarm setting and click on info the popup will show the correct info time left but once the time is reached alarm won't work and will show a very big time left as I explained here: https://github.com/InfiniTimeOrg/InfiniTime/issues/1088

Regards

brabalan commented 2 years ago

I confirm that this bug is fixed in 1.10.0.