[WIP][RFC] Introduce a ContentProvider for HR Data #1138

Open
boun wants to merge 21 commits from boun/runnerup into master
boun commented 4 years ago (Migrated from github.com)
Owner

Hi,

as I really want to get HR Data into runnerup here is a patch to do so. It is still not finished however I am proposing at an early stage to get some feedback. The Idea is to provide three query apis to:

  • Discover Device(s?) The Interface looks like you can pair multiple devices. Hence an endpoint to discover them. As I only have a MiBand2 I am not 100% sure how Gadgetbridge behaves here
  • Start / Stop Realtime DataCollection: It seems like I have to send an Intent to start that
  • Get Realtime Data: Send Samples and inform ContentObservers

I have written some tests. What works for sure is discovery of a device. Realtime data is currently delivered as String, that is also a TODO. Furthermore my activity_start / stop are a stub in the tests.

However before implementing Contract Classes, deciding on the data format, writing more tests etc. I would like to have some feedback, if the API meets your "taste", what Data you would send (only Heartrate, Heartrate and Steps, Full Sample Object). Furthermore some input on the Code, how to test everything is highly appreciated. If you think ContentProvider is not the way to go, that is also fine.

Have fun!

Hi, as I really want to get HR Data into runnerup here is a patch to do so. It is still not finished however I am proposing at an early stage to get some feedback. The Idea is to provide three query apis to: - Discover Device(s?) The Interface looks like you can pair multiple devices. Hence an endpoint to discover them. As I only have a MiBand2 I am not 100% sure how Gadgetbridge behaves here - Start / Stop Realtime DataCollection: It seems like I have to send an Intent to start that - Get Realtime Data: Send Samples and inform ContentObservers I have written some tests. What works for sure is discovery of a device. Realtime data is currently delivered as String, that is also a TODO. Furthermore my activity_start / stop are a stub in the tests. However before implementing Contract Classes, deciding on the data format, writing more tests etc. I would like to have some feedback, if the API meets your "taste", what Data you would send (only Heartrate, Heartrate and Steps, Full Sample Object). Furthermore some input on the Code, how to test everything is highly appreciated. If you think ContentProvider is not the way to go, that is also fine. Have fun!
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

About the Travis error: The testsuite does not set up the DeviceManager and I have no idea how to do that. Strangely locally all tests pass.

About the Travis error: The testsuite does not set up the DeviceManager and I have no idea how to do that. Strangely locally all tests pass.
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

So I addressed your comments for the better part. What is left now to do is:

  • After some measurements I do not get any more heartrates from my Mi2BandSupport. This is unfortunate. It may have several reasons including: A bug in my code, a cursed firmware, a thread inside of GB which just shuts that reporting down. Any thoughts / pointers here? Could someone with another device test? Do I need to take a Wakelock somewhere?
  • Fix Travis: Tests fail because I try to query the device Manager which is not set up correctly (at least getDeviceManager() in GBApplication returns null) in the test environment. I am open to suggestions for workarounds.
  • Permission System to read HR Data: I think android asking for permission instead of just send HR Date to every interested party would be a nice thing to do
So I addressed your comments for the better part. What is left now to do is: - After some measurements I do not get any more heartrates from my Mi2BandSupport. This is unfortunate. It may have several reasons including: A bug in my code, a cursed firmware, a thread inside of GB which just shuts that reporting down. Any thoughts / pointers here? Could someone with another device test? Do I need to take a Wakelock somewhere? - Fix Travis: Tests fail because I try to query the device Manager which is not set up correctly (at least getDeviceManager() in GBApplication returns null) in the test environment. I am open to suggestions for workarounds. - Permission System to read HR Data: I think android asking for permission instead of just send HR Date to every interested party would be a nice thing to do
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

I found a Workaround for the missing samples problems. In LiveActivityFragment there is this:

c493df2423/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/charts/LiveActivityFragment.java (L340)

which repeatedly calls that function. So I also added a Timer, that calls onEnableRealtimeHeartRateMeasurement over and over again. Though I think this is strange behaviour, as the realtimesteps keep coming without any periodic enabling... Anyway, everything works now.

I also added the permissions, hence I finished my todo list and will commit the changes to this branch.

I found a Workaround for the missing samples problems. In LiveActivityFragment there is this: https://github.com/Freeyourgadget/Gadgetbridge/blob/c493df242344b75ea8b42c09da046109faa148e3/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/charts/LiveActivityFragment.java#L340 which repeatedly calls that function. So I also added a Timer, that calls onEnableRealtimeHeartRateMeasurement over and over again. Though I think this is strange behaviour, as the realtimesteps keep coming without any periodic enabling... Anyway, everything works now. I also added the permissions, hence I finished my todo list and will commit the changes to this branch.
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

O.k. I finally found the problem with the tests:

11c545362a/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/GBApplication.java (L148)

GBApplication is not set up correctly in onCreate(). It checks wether a static variable is set already and if so it returns. Rationale is to guard against multiple invocations from robolectric. However the DeviceManager is not static, therefore each invocation of the testsuite with more than one testcase will get null as a result for ((GBApplication)getContext()).getDeviceManager() in the second and following testcases.

@cpfeiffer actually it was you, who committed that change two years ago:

49b8b9ebca

I could now try to fix this, however this is core code which I do not know well enough.

A testcase is in my debug branch:
https://github.com/boun/Gadgetbridge/blob/debugging/app/src/test/java/nodomain/freeyourgadget/gadgetbridge/test/SampleProviderTest2.java

It simply calls getDeviceManager() in two testcases. The first works, the second fails.

O.k. I finally found the problem with the tests: https://github.com/Freeyourgadget/Gadgetbridge/blob/11c545362ab4597dcf7333fd4d2982a264984e69/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/GBApplication.java#L148 GBApplication is not set up correctly in onCreate(). It checks wether a static variable is set already and if so it returns. Rationale is to guard against multiple invocations from robolectric. However the DeviceManager is **not** static, therefore each invocation of the testsuite with **more than one** testcase will get `null` as a result for `((GBApplication)getContext()).getDeviceManager()` in the **second** and following testcases. @cpfeiffer actually it was you, who committed that change two years ago: https://github.com/Freeyourgadget/Gadgetbridge/commit/49b8b9ebca7273189f352835a212be4218110376 I could now try to fix this, however this is core code which I do not know well enough. A testcase is in my debug branch: https://github.com/boun/Gadgetbridge/blob/debugging/app/src/test/java/nodomain/freeyourgadget/gadgetbridge/test/SampleProviderTest2.java It simply calls getDeviceManager() in two testcases. The first works, the second fails.
Owner

re timer: yes, we have to re-enable the realtime measurements again and again for Mi2. It is also kind of sensible, because delivering realtime data is much more battery intensive than sending the data in one go. So Mi2 only sends it for a short period of time if it doesn't get another enable-command.

re timer: yes, we have to re-enable the realtime measurements again and again for Mi2. It is also kind of sensible, because delivering realtime data is much more battery intensive than sending the data in one go. So Mi2 only sends it for a short period of time if it doesn't get another enable-command.
Owner

re DeviceManager: back then we got multiple invocations of GBApplication#onCreate() by robolectric, either due to a bug or by design. Things may have changed since then, so we might revise this. I totally agree that this code is a little convoluted, with the db being setup either by GBApplication (in normal mode) or by TestBase (in test mode). Initially, robolectric did not support custom Application subclasses at all and I don't know how it handles the Application instance lifecycle at the moment. For db tests, we want to make sure that every test gets a fresh db environment.

According to your findings, I suspect that we do get a new GBApplication instance (hence deviceManager field being null). The static fields survive, of course.

So to sum up,

  • we can check if we still get multiple onCreate() calls on the same instance
  • we can try to make all static instances in GBApplication non-static and then remove the guard
  • or we could make the deviceManager instance also static (urgh)
  • or come up with something else?
re DeviceManager: back then we got multiple invocations of GBApplication#onCreate() by robolectric, either due to a bug or by design. Things may have changed since then, so we might revise this. I totally agree that this code is a little convoluted, with the db being setup either by GBApplication (in normal mode) or by TestBase (in test mode). Initially, robolectric did not support custom Application subclasses at all and I don't know how it handles the Application instance lifecycle at the moment. For db tests, we want to make sure that every test gets a fresh db environment. According to your findings, I suspect that we do get a new GBApplication instance (hence deviceManager field being null). The static fields survive, of course. So to sum up, - we can check if we still get multiple onCreate() calls on the same instance - we can try to make all static instances in GBApplication non-static and then remove the guard - or we could make the deviceManager instance also static (urgh) - or come up with something else?
Owner

@cpfeiffer
I think we can have realtime measurements without sending commands over and over again. Not 100% sure though

@cpfeiffer I think we can have realtime measurements without sending commands over and over again. Not 100% sure though
Owner

Thanks for your work, really looking forward to getting this in! 👍

Thanks for your work, really looking forward to getting this in! :+1:
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

About the tests: I did not see multiple invocations of onCreate during my (limited) tests. I saw one invocation per testcase. Hence from that point of view I should be o.k. to remove the guard.

Currently I am playing around with the integration of the contentprovider into runnerup, https://github.com/boun/runnerup/commits/contentprovider and some minor cleanup. When I am done I'd rebase, open up a new pull request and squash everything together?

Future Ideas for this provider would be:

  • Add a library via maven which contains the ContractClass for easier integration
  • Change the ContentProvider from this quite rudimentary buffered_sample in Memory thing to use the database as every realtime measurement automatically lands there too
  • Provide historic data
About the tests: I did not see multiple invocations of onCreate during my (limited) tests. I saw one invocation per testcase. Hence from that point of view I should be o.k. to remove the guard. Currently I am playing around with the integration of the contentprovider into runnerup, https://github.com/boun/runnerup/commits/contentprovider and some minor cleanup. When I am done I'd rebase, open up a new pull request and squash everything together? Future Ideas for this provider would be: - Add a library via maven which contains the ContractClass for easier integration - Change the ContentProvider from this quite rudimentary `buffered_sample` in Memory thing to use the database as every realtime measurement automatically lands there too - Provide historic data
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

After removing the guard all 61 tests still pass. onCreate is called 61 times.

After removing the guard **all** 61 tests still pass. onCreate is called 61 times.
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

And to answer that question: Yes this starts gadgetbridge in case it was not running before.

And to answer that question: Yes this starts gadgetbridge in case it was not running before.
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

Sound lovely, I'll address the comment and squash it to a bare minimum. I'll ping you when I am done. In the meantime, might I motivate you to compile my runnerup fork and try it out with different / multiple devices? You don't need to go running, just some 5 mins of testing in your pocket would be fantastic.

Furthermore, for the record: as soon as this change is merged, you have an external api, which others will develop against. Hence I'd flag it as "beta" or "unstable" in the changelog entry for the first version.

Sound lovely, I'll address the comment and squash it to a bare minimum. I'll ping you when I am done. In the meantime, might I motivate you to compile my runnerup fork and try it out with different / multiple devices? You don't need to go running, just some 5 mins of testing in your pocket would be fantastic. Furthermore, for the record: as soon as this change is merged, you have an external api, which others will develop against. Hence I'd flag it as "beta" or "unstable" in the changelog entry for the first version.

I have one suggestion though, this content provider should be protected by an authorization dialogue before being released to the public and doing it right now while nothing depends on it sounds like a reasonable idea:

  1. Fragment for managing content provider accesses
  2. Database or shared preferences model for storing package names and granted access(es, if we add more providers later on)
  3. Content provider has to check calling package (For ex. ContentProvider.getCallingPackage)
I have one suggestion though, this content provider should be protected by an authorization dialogue before being released to the public and doing it right now while nothing depends on it sounds like a reasonable idea: 1. Fragment for managing content provider accesses 2. Database or shared preferences model for storing package names and granted access(es, if we add more providers later on) 3. Content provider has to check calling package (For ex. `ContentProvider.getCallingPackage`)
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

Hi @TaaviE, thx for your comment. I think your wish is actually an OS function based on the android permissions system (see screenshot, the default is off, which I changed). Together with the suggestion by cpfeiffer (the big todo) it is guaranteed that the user first has to give his consent an then also sees an indicator that realtime monitoring is happening. And this will be the topic of my next comment :)

screenshot_1532211117

Hi @TaaviE, thx for your comment. I think your wish is actually an OS function based on the android permissions system (see screenshot, the default is off, which I changed). Together with the suggestion by cpfeiffer (the big todo) it is guaranteed that the user first has to give his consent an then also sees an indicator that realtime monitoring is happening. And this will be the topic of my next comment :) ![screenshot_1532211117](https://user-images.githubusercontent.com/3662452/43040436-2eef862c-8d44-11e8-8dbe-aea77eedd739.png)
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

Hi @cpfeiffer, I like the idea of showing a notification to the user and allowing her to interrupt realtime sampling. However that is actually not as easy to do, because there is a thread inside the contentprovider (and LiveActivityFragment but that should not be the problem), that restarts that sampling each second.

Coincidently I do not like this implementation. And even more of a coincident is, that for my mi band 2 it is probably not correct either (https://github.com/Freeyourgadget/Gadgetbridge/issues/913). Hence should this not be moved up the stack closer to the "drivers"?

Hi @cpfeiffer, I like the idea of showing a notification to the user and allowing her to interrupt realtime sampling. However that is actually not as easy to do, because there is a thread inside the contentprovider (and LiveActivityFragment but that should not be the problem), that restarts that sampling each second. Coincidently I do not like this implementation. And even more of a coincident is, that for my mi band 2 it is probably not correct either (https://github.com/Freeyourgadget/Gadgetbridge/issues/913). Hence should this not be moved up the stack closer to the "drivers"?

@boun Gadgetbridge's minimum API is Kitkat (19, 4.4) and runtime permissions are a Marshmallow (23+) thing meaning that this has to be implemented by Gadgetbridge.

@boun Gadgetbridge's minimum API is Kitkat (19, 4.4) and runtime permissions are a Marshmallow (23+) thing meaning that this has to be implemented by Gadgetbridge.
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

@TaaviE It is not about runtime permissions. Here the screenshot from KitKat. I do not think it makes sense to rebuild a OS function, that is available in later releases. The Users is informed upon install that the app wants to access these services. And if the "big todo" is implemented she is informed in realtime and might even stop it from happening.

However this is my opinion, if you want to implement that feel free!

screenshot_1532261435

@TaaviE It is not about **runtime** permissions. Here the screenshot from KitKat. I do not think it makes sense to rebuild a OS function, that is available in later releases. The Users is informed upon install that the app wants to access these services. And if the "big todo" is implemented she is informed in realtime and might even stop it from happening. However this is my opinion, if you want to implement that feel free! ![screenshot_1532261435](https://user-images.githubusercontent.com/3662452/43045450-252bdc10-8db9-11e8-8f20-96b324d2f4da.png)

@boun Exactly my point, everyone who wants gets access when there's just that permission on KK. Say some proprietary app actually adds Gadgetbridge support I and I suspect many others would not want it to instantly have access to my heart rate data.

@boun Exactly my point, everyone who wants gets access when there's just that permission on KK. Say some proprietary app actually adds Gadgetbridge support I and I suspect many others would not want it to instantly have access to my heart rate data.
boun commented 4 years ago (Migrated from github.com)
Poster
Owner

I consider your point highly hypothetical :) However feel free to implement it.

I consider your point highly hypothetical :) However feel free to implement it.
Owner

Given the nature of the project, I agree that the feature outlined by @TaaviE is important and should be present when the new functionality is available in the published application.

Which means (in my opinion) that it could either be implemented in the PR, or after the merge (but before we release).

Given the nature of the project, I agree that the feature outlined by @TaaviE is important and should be present when the new functionality is available in the **published** application. Which means (in my opinion) that it could either be implemented in the PR, or after the merge (but before we release).
Owner

I'm in a middle position. IMHO it shouldn't be a blocker for the first release. AFAIU, this is only a real problem for KK, where you cannot deny permissions. On Lollipop and above, a user could install a "rogue" app and simple deny the single permission.

A simple solution might be to have this feature on Lollipop+ only. Not sure if that would be a problem for the RunnerUp userbase. but I could imagine they wouldn't want to require a Gadgetbridge permission anyway, if they still targeted KK.

I personally had not the possibility to try out the integration yet, maybe next week.

I'm in a middle position. IMHO it shouldn't be a blocker for the first release. AFAIU, this is only a real problem for KK, where you cannot deny permissions. On Lollipop and above, a user could install a "rogue" app and simple deny the single permission. A simple solution might be to have this feature on Lollipop+ only. Not sure if that would be a problem for the RunnerUp userbase. but I could imagine they wouldn't want to require a Gadgetbridge permission anyway, if they still targeted KK. I personally had not the possibility to try out the integration yet, maybe next week.
Owner

What's the current state of this? Can we move this forward?

What's the current state of this? Can we move this forward?
lesensei commented 3 years ago (Migrated from github.com)
Owner

Hey there,
Since I'm very interested in being able to track my HR in an opensource running app, I tried this with my miband3. I probably did something wrong, because GB loses the connection to my mb3 after about 5 minutes and crashes when I try to export the logs from the debug screen. @boun if you're still interested in working on this, I can help test it but I'm no code wizard so I won't be able to do much by myself except run ADB and find out where it fails. I have runnerup from your contentprovider branch crashing as well when trying to get into the history pane, but I don't think this has anything to do with your changes.
Anyway, if I can help with something that's not way over my head, please let me know, I'd be happy to.
Regards,

Hey there, Since I'm very interested in being able to track my HR in an opensource running app, I tried this with my miband3. I probably did something wrong, because GB loses the connection to my mb3 after about 5 minutes and crashes when I try to export the logs from the debug screen. @boun if you're still interested in working on this, I can help test it but I'm no code wizard so I won't be able to do much by myself except run ADB and find out where it fails. I have runnerup from your contentprovider branch crashing as well when trying to get into the history pane, but I don't think this has anything to do with your changes. Anyway, if I can help with something that's not way over my head, please let me know, I'd be happy to. Regards,
Owner

@lesensei Is this reproduceable? Can you provide logs (privately, if you want)?

@lesensei Is this reproduceable? Can you provide logs (privately, if you want)?
This pull request has changes conflicting with the target branch.
app/build.gradle
app/src/main/AndroidManifest.xml
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.