InfiniTime icon indicating copy to clipboard operation
InfiniTime copied to clipboard

Vibration scheduler

Open minacode opened this issue 2 years ago • 7 comments

This PR aims to solve #1223 . It implements intermediate logic between the motor controller and the rest of the system. The main goal is to prevent simutaneously activation of vibrations. The side effects of the old behaviour seem to be weird strong vibrations and vibrations cancelling themselves out.

This implementations exposes the following API on the MotorController: ActivatePhoneCall, ActivateTimer, ActivateAlarm, ActivateNotification to mark the different vibrations as something that has to happen. DeactivatePhoneCall, DeactivateTimer, DeactivateAlarm to mark the vibrations as being over. SingleVibration to trigger a vibration for in-app use (eg. metronome). The single vibration is skipped, when another vibration is already happening.

The activated vibrations are handled according to their priority. The priorities from high to low are:

  1. phone call
  2. timer
  3. alarm
  4. notification

The active one with the highest priority is run. When it is over, the next highest one is run afterwards. This ensures that none of them is lost.

In the Update function, we can also define custom durations/pattern for each of the vibrations, such that they can be easier distinguished.

TODO:

  • [ ] Are chimes as single vibration ok?
  • [ ] Test this code.

minacode avatar Sep 14 '22 13:09 minacode

I thought about something like this too for a little while, but then went for the simple implementation, because what are we gaining from the abstract one?

In the long run, I also wanted to add that when a vibration starts, the corresponding app is opened.

Based on your idea, I could see some kind of initial registration from an app where it submits a config of something like

  • it's name
  • priority
  • duration
  • recurrence

And then we (de)activate based on the name.

How do we store them? Making an array of n priorities? Or one with n entries and give out ids like you did? This leads to cleaner code for the future, but uses more RAM. I like the cleaner API though.

minacode avatar Sep 16 '22 07:09 minacode

I think the MotorController should not handle general notifications. If someone wants to implement a visual indication for notifications in the future for example this would all need to be copied or moved somewhere else.

I think it might make sense to put the handler for this into a separate Notification Controller. It might seem like bloat but I think it might get more important in the future since notifications are one of the main components of this watch.

This is also something that would enable this: In the long run, I also wanted to add that when a vibration starts, the corresponding app is opened.

taukakao avatar Sep 18 '22 16:09 taukakao

Thanks for the feedback. I will do this.

minacode avatar Sep 18 '22 17:09 minacode

I am stuck with getting

/sources/src/systemtask/SystemTask.cpp: In constructor 'Pinetime::System::SystemTask::SystemTask(Pinetime::Drivers::SpiMaster&, Pinetime::Drivers::St7789&, Pinetime::Drivers::SpiNorFlash&, Pinetime::Drivers::TwiMaster&, Pinetime::Drivers::Cst816S&, Pinetime::Components::LittleVgl&, Pinetime::Controllers::Battery&, Pinetime::Controllers::Ble&, Pinetime::Controllers::DateTime&, Pinetime::Controllers::TimerController&, Pinetime::Controllers::AlarmController&, Pinetime::Drivers::Watchdog&, Pinetime::Controllers::NotificationManager&, Pinetime::Controllers::MotorController&, Pinetime::Drivers::Hrs3300&, Pinetime::Controllers::MotionController&, Pinetime::Drivers::Bma421&, Pinetime::Controllers::Settings&, Pinetime::Controllers::HeartRateController&, Pinetime::Applications::DisplayApp&, Pinetime::Applications::HeartRateTask&, Pinetime::Controllers::FS&, Pinetime::Controllers::TouchHandler&, Pinetime::Controllers::ButtonHandler&)':
/sources/src/systemtask/SystemTask.cpp:84:21: error: cannot bind non-const lvalue reference of type  Pinetime::Controllers::AlertController&' to an rvalue of type 'Pinetime::Controllers::AlertControlle '
   84 |     alertController(motorController),
      |                     ^~~~~~~~~~~~~~~
In file included from /sources/src/systemtask/SystemTask.h:21,
                 from /sources/src/systemtask/SystemTask.cpp:1:
/sources/src/components/alert/AlertController.h:10:7: note:   after user-defined conversion: 'Pinetime::Controllers::AlertController::AlertController(Pinetime::Controllers::MotorController&)'
   10 |       AlertController(MotorController& motorController);
      |       ^~~~~~~~~~~~~~~

and can not figure out how to do this right. I would apprechiate some help with this. All I want to do is adding the AlertController to the SystemTask.

minacode avatar Sep 19 '22 17:09 minacode

@minacode I got it to compile although I haven't tested it yet. But you can take some inspiration from what I did if you want.

https://github.com/Joheyy/InfiniTime/commit/73b12c649504c8b8e1bed7619b3d29e65e341959

taukakao avatar Sep 19 '22 19:09 taukakao

Thank you very much! You can make a PR at my fork and I will merge your changes to this PR. I am still a little confused why your approach compiles though. For example your change in SystemTask.cpp seems to break the pattern of the other lines around.

Edit: I learned about C++ variable initialization. Was not expecting this... But things make more sense now.

minacode avatar Sep 19 '22 21:09 minacode

Ok, so the last commit does a few things we surely will discuss.

  1. It moves the call of Update to the SystemTask. This ensures that all active alerts will be run in the case that Update will do nothing because a single vibration from an app is running. There is still the possibility of an app spamming single vibrations or doing a very long one, but this wont happen in practice. We could add a StopAll function to the MotorController like @ght suggested. This should then only be called by the AlertController to gain priority.

  2. It implements support for the DisplayApp to restore the original functionality (apps opening when vibrations start). Update returns whether it started a new vibration. If it did, the SystemTask gets the corresponding Message from DisplayMessage and then forwards it to the DisplayApp. This part seems to me like we might refactor this. One idea is to only ping the DisplayApp and letting it catch the value by itself (although that would only be more complicated). Right now, the reference to the AlertController in the DisplayApp is not needed.

minacode avatar Sep 21 '22 16:09 minacode

Yes, that issue was the start of this work 😊

The missing part is to test it. The functionality is complete, I think. Sorry that I haven't done it yet. My focus went to other PRs.

Also I have to rebase it. This PR is behind by 100+ commits.

minacode avatar Nov 17 '22 08:11 minacode

This is my focus PR once I have time again in the next days. If you want to help you can take a look at the code or test it. It's behind develop but it seems to run stably. We need to find out if it works as intended.

minacode avatar Nov 19 '22 14:11 minacode

There is a minor problem with this PR: when more than one notification happens, the notification app triggers only once and will probably show only the last one. If more than five notifications happen, the first one will be deleted before it could be read by the user. What do you think about it?

minacode avatar Dec 10 '22 09:12 minacode

@minacode I have gadgetbridge set to only allow a notification every 5 seconds anyways.

ghost avatar Dec 17 '22 03:12 ghost