Call improvements #342

Raupinger · 2021-05-13T00:14:33Z

Incoming calls now cause a periodic vibration. The vibration stops when the call is accepted, rejected, muted or the notification is dismissed. The timeout for the call notification preview doesn't start until one of the three buttons is pressed.

Overview of the changes:

introduced new methods for stopping and starting the vibration. MotorController::startRunning(uint8_t motorDuration) starts a vibration pattern with a period of motorDuration and 50% duty cycle. stopRunning() stops that pattern. In the meantime no other vibrations can be triggered.
renamed APP_TIMER_DEF(shortVibTimer) to MotorController::runForDuration(uint8_t motorDuration) for clarity.
Screens::Notifications and Notifications::NotificationItem now require a reference to the motor Controller. This is needed so they can turn off the vibrations based on user interaction.
SystemTask::Work() will check the notification category and either starts a vibration pattern or a one time vibration based on it.
Notifications::NotificationItem now gets a pointer to timeoutTickCountEnd and timeoutTickCountStart. This is needed because it needs to set those times based on when one of the buttons is pressed.
the constructor of NotificationItem sets the timeoutOnHold flag when the notification is an incoming call
the event handlers for the buttons on the call notification now reset the timeout, clear the timeoutOnHold flag and stop the vibration pattern. A helper function is used to prevent dublicating that code.
Notifications::Refresh() checks if the timeoutOnHold flag is set before moving the progress bar. It also includes a check to make sure the vibration pattern gets stopped before the app quits.

Caveats:

the motor takes a lot of energy. It might be worth thinking about reducing the duty cycle of the pattern. I choose 50% because it allowed me to use a single timer. It might also be a good idea to add a timeout after which the vibration stops.
while working on this, the SWD interface on my devkit stopped working. Because of that, I could not test the finished code. In particular the sections relevant for resting the timeout when the timeoutOnHold flag is cleared. It looks fine to me and I didn't want to wait for a new kit to open this PR. I would appreciate if somebody could confirm that its working.

Raupinger · 2021-05-13T00:24:12Z

call.mp4

Video of Gadgetbridges "find me" feature. As you can see the progress bar makes a jump when the mute button is pressed. The reason is that this build doesn't reset the timeout.

Raupinger · 2021-05-14T12:28:53Z

I found a different adapter. tested the remaining changes and everything works.

…ve been interacted with

…gs like additional whitespaces

JF002 · 2021-07-04T19:10:33Z

Thanks for this PR, and sorry for the delay to review it!

It looks mostly good to me, and the vibration is a lot more perceptible when receiving a call. No one will ever miss a call with this PR!

1 small formatting detail : Could you use the CamelCase for the methods you created (first letter is a capital one) so they are consistent with (most of) the other ones?

Also, could you fix the conflicts with develop so I can merge it?

Thanks!

# Conflicts: # src/components/motor/MotorController.cpp # src/displayapp/DisplayApp.cpp # src/systemtask/SystemTask.cpp

Raupinger · 2021-07-06T15:38:20Z

ok, i changed the method names and merged current develop into the branch. didint rebase this time because i cant test it right now and the conflicts weren't 100% trivial so i would like to keep the working history.

JF002 · 2021-07-06T17:54:20Z

Thanks for the merge. However, I think something went wrong during conflict fixing, and new changes that are not related to this PR got added (in DfuService.cpp (IsSleeping() is called 2x, once as a field, once as a method), SystemTask.h (IsSleeping was a method, it's now an atomic field), SystemTask.cpp (I think the code that actually enables the new vibrations for the call have been removed).
Could you check that out?

Raupinger · 2021-07-06T19:07:10Z

Sure. Ill look it once im back at my desk

JF002 · 2021-07-06T19:27:54Z

Of course, take your time, we're not in a hurry ;)

Riksu9000 · 2021-07-12T13:35:42Z

src/components/motor/MotorController.cpp

 }

-void MotorController::vibrate(void* p_context) {
+void MotorController::startRunning(uint8_t motorDuration) {


uint8_t isn't big enough

Or maybe we don't need this parameter at all.

It defines the length of a the individual vibration and wait times the continuous pattern is made up of. You're right that using uint8_t limits that lenght to 255ms. Ill switch to 16 bit, but its really not that big of a deal (the only place this is currently used, the call notification sets it to 10ms). Removing this would mean hard-coding the "period" of the pattern which would mean applications cant vary their patterns.

Exactly, however I think it might be better for consistency to not let applications change it. And "hard coding" isn't always a bad thing if the alternative is to re define the same thing in many places.

In one of the earlier commits, there was this line, which overflowed the parameter. I don't know if you changed this later, as currently this line doesn't exist in this PR.
motorController.startRunning(500);

Hardcoding this parameter is a possibility, indeed, mostly if we only use 1 single value.
If you want to future proof the class, the parameter is nice too. IMHO, the best solution for the type is not uint8_t or uint16_t or uint32_t but a stronger type that clearly represents a duration : std::chrono::millisecond or a custom type Duration, for example.

Agree with using std::chrono::milliseconds, it's large enough and eliminates any ambiguity as to what the expected units of motorDuration are without needing to dig into the implementation.

Riksu9000 · 2021-07-12T13:39:09Z

src/components/motor/MotorController.cpp

+void MotorController::vibrate(void* p_context) {
+  if (nrf_gpio_pin_out_read(pinMotor) == 0) {
+    nrf_gpio_pin_set(pinMotor);
+  } else {
+    nrf_gpio_pin_clear(pinMotor);
+  }


Maybe this function should play a pattern on each call. This would work for both a short and continuous notification and make them more noticable, and nicer. Just two short vibrations could work.

Vibration patterns would be a nice addition to this class MotorController. However, I think we can create a new PR to add them :)

I was looking at extending the MotorController interface with something like void vibrate(std::chrono::milliseconds pulseDuration, int pulseCount = 1, std::chrono::milliseconds = some_reasonable_default) (or maybe a 0-100 duty cycle as the last parameter, defaulted to 50). If you think something like that would be useful here I can keep working on it.

I think that the current 50% duty cycle is too aggressive and uncomfortable for the user. We should also limit the maximum duration the motor is running, because even in Metronome we can hear it get weaker.

jonvmey · 2021-07-14T00:30:08Z

src/components/motor/MotorController.cpp

+}
+
+void MotorController::vibrate(void* p_context) {
+  if (nrf_gpio_pin_out_read(pinMotor) == 0) {


nrf_gpio_pin_toggle exists as an option here.

JF002 · 2021-08-04T19:42:42Z

@Raupinger do you plan on continuing working on this PR? Or should we focus on #544 instead? (no pressure, I'm just trying to plan my reviews of some PRs on the project :))

Raupinger · 2021-08-04T21:23:05Z

Better to work on #544. I'll focus on other things.

JF002 added the enhancement Enhancement to an existing app/feature label May 15, 2021

Raupinger force-pushed the Call-Improvements branch 2 times, most recently from 8a1ad9a to 4fcef4f Compare May 15, 2021 22:24

Raupinger added 4 commits May 16, 2021 00:42

implemented continuous vibration pattern for incoming calls

d13dd6d

only activate the timeout on call notification previews after they ha…

5da6549

…ve been interacted with

cleaned up the code and reduced the size of the diff by removing thin…

56af4a0

…gs like additional whitespaces

NotificationItem now also redefines the start of the timeout

9e8dd9a

Raupinger force-pushed the Call-Improvements branch from 4fcef4f to 9e8dd9a Compare May 15, 2021 22:42

Raupinger mentioned this pull request May 16, 2021

Timer App #355

Merged

This was referenced May 23, 2021

Timer ending is not obvious enough #391

Open

Improvements to "Find my Watch" #393

Open

Timer v2 #407

Closed

JF002 added this to the Version 1.3 milestone Jul 4, 2021

nlfx and others added 2 commits July 6, 2021 17:24

Wait for SPI flash to wakeup before starting OTA DFU

fdf0b64

Merge branch 'develop' into Call-Improvements

4e3ea06

# Conflicts: # src/components/motor/MotorController.cpp # src/displayapp/DisplayApp.cpp # src/systemtask/SystemTask.cpp

Riksu9000 reviewed Jul 12, 2021

View reviewed changes

jonvmey reviewed Jul 14, 2021

View reviewed changes

JF002 modified the milestones: Version 1.3, Version 1.4 Jul 23, 2021

This was referenced Aug 1, 2021

Call improvements #544

Merged

Timer: let timer go overtime (with ringing) #557

Open

Raupinger closed this Aug 4, 2021

Uh oh!

Call improvements #342

Call improvements #342

Uh oh!

Conversation

Raupinger commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Raupinger commented May 13, 2021

Uh oh!

Raupinger commented May 14, 2021

Uh oh!

JF002 commented Jul 4, 2021

Uh oh!

Raupinger commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JF002 commented Jul 6, 2021

Uh oh!

Raupinger commented Jul 6, 2021

Uh oh!

JF002 commented Jul 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Raupinger Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonvmey Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JF002 commented Aug 4, 2021

Uh oh!

Raupinger commented Aug 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Raupinger commented May 13, 2021 •

edited

Loading

Raupinger commented Jul 6, 2021 •

edited

Loading

Raupinger Jul 12, 2021 •

edited

Loading

jonvmey Jul 14, 2021 •

edited

Loading