Skip to content

Fixes #1768 : Add a notification to charge device when battery is smaller than 20%#1797

Merged
beastoin merged 2 commits intoBasedHardware:mainfrom
joshijoe05:feature/lowBatteryNotification
Feb 17, 2025
Merged

Fixes #1768 : Add a notification to charge device when battery is smaller than 20%#1797
beastoin merged 2 commits intoBasedHardware:mainfrom
joshijoe05:feature/lowBatteryNotification

Conversation

@joshijoe05
Copy link
Copy Markdown
Contributor

claim #1768
Fixes #1768

This PR introduces a notification alert when the connected device's battery drops below 20%. It ensures users are notified once per connection session to prevent spamming while handling reconnections properly.

@beastoin
Copy link
Copy Markdown
Collaborator

1/ can you drop a demo video ?
2/ why did we need the set has low level battery on device connected ? https://github.com/BasedHardware/omi/pull/1797/files#diff-c887ba3d8e70bca5b4d97dea1232849b8d79206eee68544d4752e9292c8f7224R273
3/ please re-format the code with line length 120

@joshijoe05 ~

@joshijoe05
Copy link
Copy Markdown
Contributor Author

joshijoe05 commented Feb 14, 2025

1/ can you drop a demo video ? 2/ why did we need the set has low level battery on device connected ? https://github.com/BasedHardware/omi/pull/1797/files#diff-c887ba3d8e70bca5b4d97dea1232849b8d79206eee68544d4752e9292c8f7224R273 3/ please re-format the code with line length 120

@joshijoe05 ~

1.) Right now, I do not have a device with me to test it perfectly. I need help from the person having device to test it,
2.) It's for an edge case, Assume the device battery is less than 20% and you got connected to the app then you will receive an alert notification. And now you disconnected the device but not yet closed the app and its still on foreground/background running and if you connect now with the device it will alert you again.

That is why _hasLowBatteryAlerted is set to false when device is reconnected again and the app is in foreground/background

3.) Yup, will do it now. But i wanted to know if it worked well with the device

@beastoin
Copy link
Copy Markdown
Collaborator

1/ could you find the people in our community yourself ? http://omi.discord.com . testing is a must.
2/ what do you think about, just reseting the state of _hasLowBatteryAlerted to false, then waiting for the change ? just in case the batteryLevel is belong the old device. and, should do we do on the device disconnected ?

@joshijoe05 ~

@joshijoe05
Copy link
Copy Markdown
Contributor Author

1/ could you find the people in our community yourself ? http://omi.discord.com . testing is a must. 2/ what do you think about, just reseting the state of _hasLowBatteryAlerted to false, then waiting for the change ? just in case the batteryLevel is belong the old device. and, should do we do on the device disconnected ?

@joshijoe05 ~

1/ yeah will find out and update
2/ the initial state of the _hasLowBatteryAlerted is false which means if any device that is connected having battery lower than 20% will be notified. But in an edge case where the first device is notified for low battery then _hasLowBatteryAlerted is set to true and if it is disconnected and again if the device is connected while still keeping the app state in foreground then it wont notify if we dont set _hasLowBatteryAlerted to false (default value)

@joshijoe05
Copy link
Copy Markdown
Contributor Author

@mdmohsin7
Can you help me by testing out this ?

@mdmohsin7 mdmohsin7 self-requested a review February 14, 2025 14:21
@mdmohsin7
Copy link
Copy Markdown
Member

@mdmohsin7 Can you help me by testing out this ?

IMG_FBEA7B07DD9C-1

@joshijoe05 it works! Tested quite a few times (I guess around 8-10), and 2 times I got the notification twice

Copy link
Copy Markdown
Member

@mdmohsin7 mdmohsin7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls set the line length to 120 in your code editor

@joshijoe05
Copy link
Copy Markdown
Contributor Author

joshijoe05 commented Feb 14, 2025

@mdmohsin7
Thank you man for testing it.
I have changed the line length to 120 !

@beastoin
Copy link
Copy Markdown
Collaborator

1/ are you done ?
2/ ✅ ok. overall, the logic is not super solid btw lgtm. absolutely, you can simplify it - but later.

@joshijoe05

@joshijoe05
Copy link
Copy Markdown
Contributor Author

1/ are you done ?

2/ ✅ ok. overall, the logic is not super solid btw lgtm. absolutely, you can simplify it - but later.

@joshijoe05

1/ yes done its working
2/ ok, let me know if more changes required, Im open to suggestions and modify it

@joshijoe05 joshijoe05 requested a review from mdmohsin7 February 17, 2025 04:20
@beastoin beastoin merged commit 2efd100 into BasedHardware:main Feb 17, 2025
@beastoin
Copy link
Copy Markdown
Collaborator

lgtm @joshijoe05 / congratulation 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a notification to charge device when battery is smaller than 20%

3 participants