Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

joshijoe05
Copy link
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
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
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
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
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
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
Collaborator

@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
Collaborator

@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
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
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
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
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