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

Bugfix/xiaomi missed readings #3567

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

NiK27711
Copy link

These changes are intended to fix problems that, among other things, lead to "missed readings" when receiving G7 sensor data on Xiaomi devices (with OS MIUI 14 - see also Troubleshooting - Xiaomi MIUI Optimizations). In addition, the <always_scan> mode, which is automatically activated when necessary, can lead to frequent interruptions in receiving the values. For this reason, this mode will now be deactivated again as soon as possible in "normal operation".
To analyze and isolate problems of this kind, I have added additional debug logs to the . For the option <allow_scan_by_mac> introduced by @jamorham, I added an additional (menu) property <ob1_allow_scan_by_mac>.
In order to increase the readability of parts of the class , I've also moved the code for determining the different Bluetooth properties as well as the handling of the connection state changes into separate methods.

…n-mode" to reduce missed readings and added some debug log outputs.
# Conflicts (solved):
#	app/src/main/java/com/eveningoutpost/dexdrip/services/Ob1G5CollectionService.java
# Conflicts (solved):
#	app/src/main/java/com/eveningoutpost/dexdrip/services/Ob1G5CollectionService.java
# Conflicts (solved):
#	app/src/androidTest/java/com/eveningoutpost/dexdrip/HomeEspressoTest.java
@Navid200
Copy link
Collaborator

Navid200 commented Jul 12, 2024

Has this been tested?
If yes, with what phone brands and models?
Has it also been tested with Android 8 or 9?
And with which Dexcom devices? G5, G6, Dexcom One?

I have been using G7 with xDrip for 9 months with a Pixel 6a. I don't have unexpected missed readings. Of course when I walk away from the phone, I have missed readings. But, it recovers automatically shortly after I return.

@Navid200
Copy link
Collaborator

Are you sure you have disabled Xiaomi's aggressive battery optimization schemes when testing xDrip as is?

@NiK27711
Copy link
Author

Has this been tested? If yes, with what phone brands and models? Has it also been tested with Android 8 or 9? And with which Dexcom devices? G5, G6, Dexcom One?

I have been using G7 with xDrip for 9 months with a Pixel 6a. I don't have unexpected missed readings. Of course when I walk away from the phone, I have missed readings. But, it recovers automatically shortly after I return.

Yes, I have been using these changes for a few weeks (since mid/end of May) without any problems with my Xiaomi POCO X6 5G (with MIUI 14.0.6 -> Android 13 based) and the G7. The problem before was that on my device (in <always_scan> mode, i.e. after I was back in range) at most every second value was read (often less). So there is a relatively big difference between a "direct" connect and the scan method. Therefore, in my opinion, it makes sense to deactivate this <always_scan> mode as soon as possible and (re)use the direct connection.

Are you sure you have disabled Xiaomi's aggressive battery optimization schemes when testing xDrip as is?

Yes, I did and it was really a challenge!^^ In this context I also wrote the instructions under [Troubleshooting - Xiaomi MIUI Optimizations](https://github.com/xdrip/xdrip_docs/blob/master/docs/troubleshoot/savings.md#xiaomi-miui-optimizations).

@Navid200
Copy link
Collaborator

I'm not the person who will have to approve this. So, please take my comments for whatever they are worth.
If I was to review this, I would need to see the problem before looking at the solution.
I would like to see exactly how xDrip as is, the latest Nightly, behaves with your device. You have not shown any of that.

I just put my phone (running the latest Nightly) in the microwave for more than 15 minutes to cause 3 consecutive missed readings. Then, I removed it and it reconnected.
The following shows the data table and the logs.

Screenshot_20240712-182025 Screenshot_20240712-182046

Looking at the data table, you can see that readings at 17:55, 18:00 and 18:05 are all backfills. And the following readings are direct collections with no backfill. This shows that xDrip reconnects with no issue.

You can do the same using the latest Nightly and demonstrate how your phone malfunctions before your PR. Without that, it may be hard to convince someone that we need to change anything.

@NiK27711
Copy link
Author

I just put my phone (running the latest Nightly) in the microwave for more than 15 minutes to cause 3 consecutive missed readings. Then, I removed it and it reconnected. The following shows the data table and the logs.
[...]
Looking at the data table, you can see that readings at 17:55, 18:00 and 18:05 are all backfills. And the following readings are direct collections with no backfill. This shows that xDrip reconnects with no issue.

Apparently, scanning for devices on a Pixel 6a behaves differently than on Xiaomi devices. The behavior of "skipped readings" after you are back in range (on Xiaomi devices) has already been noted and described in the discussion here: #3454 (see here)

@Navid200
Copy link
Collaborator

Thanks for helping improve xDrip for Xiaomi users.

My concern with this PR is that it adds a new setting that everyone can see for something that seems to affect only one phone brand and only G7.

The problem I see is that users make mistakes with the settings on the G5/G6/Dex1 Debug Settings page.
So, I have been reducing the number of settings on that page for that reason. It has taken me a long time to get to where we are today. I still need to reduce the number of settings on this page.
If a new setting is required, assuming it is absolutely required, I will urge it to be added to a different page.

The problem with this page is that there are some very critical settings on it. By having too many settings on it, it is possible that one setting will be set incorrectly by a user who is not very careful. Then, they will report it as a failure of xDrip.

Will you consider making your changes conditional based on the manufacturer of the phone?
You can see how we do that here:

|| Build.MANUFACTURER.toLowerCase().contains("xiaomi")

@NiK27711
Copy link
Author

My concern with this PR is that it adds a new setting that everyone can see for something that seems to affect only one phone brand and only G7.

The option <allow_scan_by_mac> was originally introduced (internally) by @jamorham and defines (for all phones and "Dexcom sensors") whether a MAC filter with the Bluetooth MAC address of the sensor should be used when scanning (as long as this MAC address is already known). Since this option is very useful in my opinion, I only made it accessible as a property via the user interface.

It has taken me a long time to get to where we are today. I still need to reduce the number of settings on this page.

OK, I understand! For me it is absolutely no problem to move this menu to another place. My reasoning when placing this property was simply to keep related properties together. Since the different Bluetooth scan properties were already present at this point (such as <ob1_minimize_scanning> or <ob1_avoid_scanning>), this was (contextually) the right place in my opinion.

If a new setting is required, assuming it is absolutely required, I will urge it to be added to a different page.

@Navid200: In your opinion, what would be a better place (contextually) for the property <ob1_allow_scan_by_mac>?

@Navid200
Copy link
Collaborator

I still don't see why a new setting is necessary. Samsung users already have a special setting that most of them forget to set and I have to tell them to enable special pairing workaround.
It takes a lot of time to look at failure reports everyday going through numerous settings and finding the errors of the users.

I am not in favor of adding new settings, which will add to the possibility of such errors.
I don't want xDrip to become something that only works for those who know how to set it up. I want xDrip to work out of the box.

Please lets wait for the lead developer's review.
He may not have any of my concerns.

@Navid200
Copy link
Collaborator

Navid200 commented Jul 13, 2024

We have informed users that Xiaomi is a known problem:
https://navid200.github.io/xDrip/docs/Intermittent.html
Screenshot 2024-07-13 105057


It is not like we have hidden it from them.

I'm looking at Xiaomi phones that run Android 14 and their prices are very high:
Screenshot 2024-07-13 105719

The same retailer shows Pixel 6a for less than half the price:
Screenshot 2024-07-13 110041

Pixel 6a currently runs Android 14 and it will upgrade to Android 15 when it goes public in 2-3 months.

xDrip works fine as is. Why should we modify xDrip and complicate it for a phone manufacturer that we know does not follow protocol?
What is Xiaomi going to do when Android 15 goes public in a few months? What will they break then in their super duper operating system that we have to change xDrip for?

I am sorry that you have a Xiaomi and it may seem to you that I have a grudge against you. I don't. I don't want this kind of publicity:

Screenshot 2024-07-13 110655


I hope this to be an open and unbiased conversation. Can you help me understand why people buy Xiaomi phones knowing that the manufacturer does not care about apps that run in the background?
And knowing that they can get an alternative awesome phone, that xDrip developers have and support, at a lower price?

@NiK27711
Copy link
Author

The same retailer shows Pixel 6a for less than half the price

I can't answer this question in general, but I think that your comparison is mixing up price ranges of smartphones. ;) In my case, my previous phone was a Huawei phone and xDrip+ worked on it for years with absolutely no issues. Therefore, my (perhaps somewhat naive ;) ) expectation was that it would work even better with a Xiaomi device.^^ And yes, now I know that the "optimizations" developed by Xiaomi in recent years are a massive pain in [...it is not allowed to write here what I'm currently thinking...] unless you know how to deactivate them (and have the chance to deactivate them). Since I deactivated the "MIUI optimizations" and implemented my changes, xDrip+ works absolutely reliably again on my Xiaomi device.

I don't want this kind of publicity

I understand your concerns. But isn't the quality of software precisely defined by the fact that it works reliably under difficult conditions on different devices? :) I suspect that the publicity could suffer more if certain smartphone manufacturers were to be required to use xDrip+. Can it not even be an additional unique selling point if xDrip+ works reliably even on Xiaomi devices? ;)

@jamorham
Copy link
Collaborator

@NiK27711 Can you describe the significant changes that you have implemented that you think improve Xiaomi reception? It isn't really clear from your code due to other structural changes.

@Navid200 Xiaomi do make good phones, which are usually relatively inexpensive, but there are many non-standard changes made to the operating system, adverts appear in system apps and I have had random games install while the phone was idle.

@NiK27711
Copy link
Author

Can you describe the significant changes that you have implemented that you think improve Xiaomi reception? It isn't really clear from your code due to other structural changes.

@jamorham: I think the key changes targeting Xiaomi devices are:

  • Adjustments related to the (automatically activated) <always_scan> mode:
    • Increase the lower limit of the check from about 5 minutes to about 7 minutes to match the timeout of the Bluetooth connection, so that this mode is only activated if a timeout has "actually" occurred. (Line 1622)
    • Deactivate <always_scan> mode again if conditions for this mode no longer apply and the mode is currently active, so that a direct Bluetooth connection is preferentially established. (Line 1638)
  • Synchronize the creation of the within the "scheduleWakeUp" method. (Line 1061)

@Navid200
Copy link
Collaborator

Can we remove the setting and have it enabled all the time?
I will then test it on Pixel. And may be @jamorham can test it on Samsung as well before consideration for merge.
If we test it and it either improves or makes no difference, there should be no need for a switch.

If this is a kill switch, can we put it under other misc options with the default set to enabled and agree that it will be removed in a year? I will remove it.

@NiK27711
Copy link
Author

Can we remove the setting and have it enabled all the time?

The thing is, from my point of view, that in this context we are talking about the configuration or the way of using the class com.polidea.rxandroidble2.scan.ScanFilter. This class has shown different behavior on different Android versions more than once in the past. Currently, the property <allow_scan_by_mac>, which already exists in the master branch (internally only), is set to false by default, so the MAC filter of this class is generally not used (although this would certainly make sense if the class works reliably at this point - across all versions). Therefore, I am not sure if it is a good idea to change the default configuration of such a central component without having a way to quickly and easily undo this setting via the UI.
If, after a sufficiently long test phase (with two Android version changes), it turns out that this MAC filter works reliably, I am absolutely with you in removing this property from the menu. :)

@jamorham
Copy link
Collaborator

Mac filtered scan does not work reliably which is why it isn't used

@jamorham
Copy link
Collaborator

@NiK27711 can you identify a handset which exhibits the problem this PR is designed to solve? The last checks I did there was not a problem with xiaomi so long as power optimization was disabled for xDrip. After reboot battery optimization also reverted but xDrip will warn of that as soon as you open it.

@NiK27711
Copy link
Author

@NiK27711 can you identify a handset which exhibits the problem this PR is designed to solve? The last checks I did there was not a problem with xiaomi so long as power optimization was disabled for xDrip. After reboot battery optimization also reverted but xDrip will warn of that as soon as you open it.

@jamorham: The problem occurred on my Xiaomi POCO X6 5G (with MIUI 14.0.6) even when battery optimization was disabled. :)

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.

3 participants