InfiniTimeOrg / InfiniTime

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

add bluetooth disconnect alert #1442

Open Boteium opened 1 year ago

Boteium commented 1 year ago

This PR generates a notification on bluetooth disconnect event. A setting page for toggling on/off notification is also add.

https://i.imgur.com/NMrAmSV.jpg

This should address both https://github.com/InfiniTimeOrg/InfiniTime/issues/1289 https://github.com/InfiniTimeOrg/InfiniTime/issues/772

Boteium commented 1 year ago

Sure, I'll change it to Messages::BleDisconnect. On the other hand, the display app name and option name should still keep the "Alert" suffix, right ?

The reason why BleDisconnectAlertOption is not a boolean value is because I've thought of the idea of adding a third option and potentially a fourth. But I haven't iron out the idea.

Those options considered are:

I think cool downtime should be implemented as a separate option because this feature is for those who want to make sure no notification is missed but is not compatible with cooldown time. Shaky connection is something like randomly short period of disconnection, right ? I never really experience shaky connections and I would like to know how soon can a repeated connect/disconnect event happens. My experience of using this feature is that when I leave my phone for a while, my watch does correctly generate disconnect warning and does automatically re-connect when I'm back. But sometimes, when it unexpectedly lost connection, it never automatically re-connect.

Avamander commented 1 year ago

I'd recommend looking into adding integration with the Link Loss Alert service in order to give companion apps control over the alerts. This would allow user-initiated companion-side disconnects while also disabling LL alerts.

minacode commented 1 year ago

On the other hand, the display app name and option name should still keep the "Alert" suffix, right?

Yes, because they are about the alert.

I have shaky connections with my PC and itd. Itd sends a notification at connection. When I go to the next room the watch reconnects all the time and gets flooded with notifications.

But maybe we should keep this first PR as simple as it is and build more PRs on top of it. It sounds like you have good ideas.

Edit: Here is the link loss service.

Boteium commented 1 year ago

I'm currently testing the 5 minute timeout option. Because it involves many changes, I would like some feedback before committing to this PR. I don't want to make this PR too complicated. https://github.com/Boteium/InfiniTime/compare/ble_disconnect_alert...Boteium:InfiniTime:ble_disconnect_alert_5m_timeout

minacode commented 1 year ago

I found in your regular PR that you have to increase the settings version in Settings.h, because you changed the struct.

About your commit: can you explain your intuition a little? What does the pending stuff do?

Boteium commented 1 year ago

When setting the flag "pendingBleDisconnectAlert" to true, it means bluetooth is already disconnected and a notification should be sent, but is instead deferred because 5 minute timeout haven't been reached.

minacode commented 1 year ago

I took another look at your commit and have two critique points:

  1. I don't think that putting BLE stuff in the DateTimeController is a good idea. We should keep the separate components of the code as clean as possible. Send a message for every minute there and then use the SystemTask to couple that information with the disconnects.

  2. I am still not convinced of the pending disconnects, but that's maybe just me. You still want to notify on disconnects, just later on? What I expected from that feature was the check now - timestamp < 5*60 you also implemented.

Boteium commented 1 year ago

ok, I'll put 5 min timeout in a future separate PR then.

Note that there isn't any bluetooth stuff involved in DateTimeController or SystemTask/Message. Bluetooth status is never checked. This feature works because pendingBleDisconnectAlert flags will be toggle off on BleConnect event, which will also disable message sending in DateTimeConrtoller.

Boteium commented 1 year ago

add vibrate mode

Note: "5m timeout" option is not included in this PR.

minacode commented 1 year ago

Looks good to me. But I don't know enough about lvgl for a good review of the UI code.

Riksu9000 commented 1 year ago

This is often requested, so we should add something like this, however I really think it should utilize the link loss service. I think this option could be solely controlled by the companion app without any settings screen on the watch, so it can also be set to alert when out of range of a phone, but not when out of range of a computer.

Boteium commented 1 year ago

@Avamander @Riksu9000 I've taken a look at the link loss service. It shouldn't be difficult. However, there will be some implementation decisions to consider. Any feedback is welcome.

(1) The link loss service only offers three alert levels. We can implement "no alert," "vibration only," and "notification." correspondingly. But other options such as a 5-minute timeout or notifying once are not compatible with the specifications.

(2) Should we completely remove the setting from the watch? Although the link loss service allows companion apps to set the alert level on the phone, there may still be scenarios where this setting comes in handy, as described by @minacode :

I have shaky connections with my PC and itd. Itd sends a notification at connection. When I go to the next room the watch reconnects all the time and gets flooded with notifications.

(3) I envision that the companion app would handle "user-initiated companion-side disconnects" by disabling the link loss alert before requesting a disconnection. However, this approach has a downside: the user would have to manually re-enable the link loss alert for each manual re-connection request. And If companion apps were to automatically enable LL alert on connection, this downside can be avoided but then we can't implement watch side link loss control in point (2) because it would be overriden by companion apps.

I'd recommend looking into adding integration with the Link Loss Alert service in order to give companion apps control over the alerts. This would allow user-initiated companion-side disconnects while also disabling LL alerts.

(4) Another way to handle "user-initiated companion-side disconnects" is by having Pinetime ignore the "REMOTE USER TERMINATED CONNECTION (0X13)" error code. This approach would enable both sides to control the alert level. We'll also need to consider if this behavior should be an option.

(5) Similar to point (3), if the cellphone or computer automatically sets the alert level upon Bluetooth connection, implementing point (2) would not be possible. However, if the alert level is manually set by the user via the phone or computer, both the companion app and the watch can have control over the alert level.

This is often requested, so we should add something like this, however I really think it should utilize the link loss service. I think this option could be solely controlled by the companion app without any settings screen on the watch, so it can also be set to alert when out of range of a phone, but not when out of range of a computer.

sonnatager commented 1 year ago

This would be a very nice and handy feature :smile:

minacode commented 1 year ago

Thanks @sonnatager, I completely forgot this PR 😉

My current perspective after reading this again is to let companions set the level (no, vibration, notification) via link loss alert service.

And then add a small default cooldown without setting to avoid constant notifications. You could set something like 1 minute for the cooldown. That should not be too much.

In case of connection loss while the cooldown is or was active and notifications are enabled, the watch could show "Connection unstable" once instead of multiple "Connection lost".

This would solve all my problems, because, as @Riksu9000 said, I would just set the computer companion apps to not alert when I go into another room.

sonnatager commented 6 days ago

Would be cool if this is in the next release.