[WIP][RFC] Introduce a ContentProvider for HR Data #1138
Conversation
- Introduce a contract class for all constants - Provide the device as parameter and try to connect to that - Do not overwrite buffered_sample if info is bogus - Bogus Measurements with -1 HeartRate should not trigger contentobservers - Send HeartRate, Steps and Battery Level to callers
|
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. |
There was a problem hiding this comment.
Thanks a lot, very nice. And bonus points for the testcase 👍 👌
I haven't done much with content providers yet, but in general this looks good to me. Would this also start gadgetbridge in case it was not running before? And if BT is enabled, it should return an error, I suppose?
I think we should try not to provide RAW sample data, but normalized data, so that it would work with different devices. Atm, it's just HR and steps, so that's normalized already.
Great job!
app/src/main/AndroidManifest.xml
Outdated
| @@ -413,6 +413,11 @@ | |||
| android:authorities="com.getpebble.android.provider" | |||
| android:exported="true" /> | |||
|
|
|||
| <provider | |||
| android:name=".contentprovider.HRContentProvider" | |||
| android:authorities="com.gadgetbridge.heartrate.provider" | |||
There was a problem hiding this comment.
gadgetbridge.com is not our domain, gadgetbridge.org is. But why do you explicitly not use the nodomain.freeyourgadget.org name?
There was a problem hiding this comment.
Indeed, I wanted to follow the name of the java packages, however on a second thought this should then have been nodomain.freeyourgadget.gadgetbridge. I'll change it to org.gadgetbridge. Because I a am tempted to keep the provider more general i'll go with
org.gadgetbridge.realtimesamples.provider
Feel free to oppose
| @@ -0,0 +1,238 @@ | |||
| /* Copyright (C) 2016-2018 Andreas Shimokawa, Carsten Pfeiffer | |||
There was a problem hiding this comment.
This should be your name, here :-)
| LocalBroadcastManager.getInstance(this.getContext()).registerReceiver(mReceiver, filterLocal); | ||
|
|
||
| //TODO Do i need only for testing or also in production? | ||
| this.getContext().registerReceiver(mReceiver, filterLocal); |
There was a problem hiding this comment.
Gadgetbridge's own events are sent via LocalBroadcastManager, so this should not be necessary.
There was a problem hiding this comment.
Either way, for testing I need that hook. Maybe it is because I did not understand the testing framework completely. I'll have a look. I think there is a GBApplication.isLocal() call, so I can call it only if a testcase is running.
| public static final String[] realtimeColumnNames = new String[]{"Status", "Heartrate", "Steps", "Battery"}; | ||
|
|
||
| static final String AUTHORITY = "com.gadgetbridge.heartrate.provider"; | ||
|
|
| // Stuff context into provider | ||
| provider.attachInfo(app.getApplicationContext(), null); | ||
|
|
||
| ShadowContentResolver.registerProviderInternal("com.gadgetbridge.heartrate.provider", provider); | ||
| } |
| for (GBDevice device : deviceManager.getDevices()) { | ||
| if (deviceAddress.equals(device.getAddress())) { | ||
| Log.i(HRContentProvider.class.getName(), String.format("Found device device %s", device)); | ||
| return device; |
There was a problem hiding this comment.
Please use the slf4j logger instead of Log.i, Log, e, etc.
Device is disconnected - Connected but not sending Realtime data - sends realtime data
|
So I addressed your comments for the better part. What is left now to do is:
|
Introduce a timer that repeatedly asks the device to send measurements. Found in LiveActivityFragment. Strangely this works for Steps without punching it. Needs testing
There was a problem hiding this comment.
Thanks a lot, great job! 💪
I currently lack the time to investigate the problem with Travis. If all fails, we could @Ignore the testcase and fix it later.
Re permission for requesting HR data: yes, that totally makes sense.
Re stopping of hr measurements: the log should hopefully contain some information about the reason.
| @@ -195,20 +198,27 @@ public Cursor query(@NonNull Uri uri, String[] projection, String selection, Str | |||
| @Nullable | |||
| private GBDevice getDevice(String deviceAddress) { | |||
| DeviceManager deviceManager; | |||
|
|
|||
| if (mGBDevice != null && mGBDevice.getAddress() == deviceAddress) { | |||
There was a problem hiding this comment.
This should be equals(), not ==
app/src/main/AndroidManifest.xml
Outdated
| @@ -415,7 +415,7 @@ | |||
|
|
|||
| <provider | |||
| android:name=".contentprovider.HRContentProvider" | |||
| android:authorities="com.gadgetbridge.heartrate.provider" | |||
| android:authorities="org.gadgetbridge.realtimesamples.provider" | |||
There was a problem hiding this comment.
Thanks, but I still don't get the rationale: why org.gadgetbridge instead of nodomain.freeyourgadget.gadgetbridge? I'm not totally opposed, but I'd like to understand.
There was a problem hiding this comment.
Then I misunderstood you. I'll finally change it to nodomain.freeyourgadget.gadgetbridge
|
I found a Workaround for the missing samples problems. In LiveActivityFragment there is this: 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. |
|
O.k. I finally found the problem with the tests: 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 @cpfeiffer actually it was you, who committed that change two years ago: 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: It simply calls getDeviceManager() in two testcases. The first works, the second fails. |
|
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 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,
|
|
@cpfeiffer |
| @@ -758,6 +758,7 @@ public void onHeartRateTest() { | |||
| } | |||
| } | |||
|
|
|||
| // Unser Einstig | |||
There was a problem hiding this comment.
If these comments are intended to be submitted, they should be in English :-)
There was a problem hiding this comment.
Sorry, I managed to merge the [DO NOT MERGE] commit
| @@ -1193,6 +1197,7 @@ public void doCurrentSample() { | |||
| try (DBHandler handler = GBApplication.acquireDB()) { | |||
| DaoSession session = handler.getDaoSession(); | |||
|
|
|||
| // Why is this so complicated???? | |||
There was a problem hiding this comment.
Do you have an idea how to make it less complicated?
| } | ||
|
|
||
| @NonNull | ||
| private Cursor realtime(String[] projection, String[] args) { |
There was a problem hiding this comment.
From looking at the name and the arguments, I would have no idea what it is or what arguments it expects. Maybe name it getRealtimeHeartRateCursor()?
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Nullable | ||
| private Cursor devices_list() { |
There was a problem hiding this comment.
Suggestion for a better method name (and please try to use camel case): getDevicesCursor()? Or just getDevices()?
| } | ||
|
|
||
|
|
||
| private void enable_realtime() { |
There was a problem hiding this comment.
enableContinuousRealtimeHeartRateMeasurement()? Long, but descriptive.
|
Thanks for your work, really looking forward to getting this in! 👍 |
|
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:
|
This reverts commit 6269b41.
|
After removing the guard all 61 tests still pass. onCreate is called 61 times. |
|
And to answer that question: Yes this starts gadgetbridge in case it was not running before. |
There was a problem hiding this comment.
I'll merge this in, great work! 👍 Do you want to squash anything before merging?
I'll add a tiny, a small and a slightly bigger TODO for a future update. For the tiny TODO, see the next comment (use HeartRateUtils).
The small one is that we need an entry for the changelog. This feature also warrants a blog post, but probably not before your patch for RunnerUp is merged, right?
The bigger TODO is that since Gadgetbridge is now being remote controlled, we should provide the user a means to stop this. E.g. in case an application is buggy, crashes, or forgets to stop the realtime measurement, the user should be able to stop it from within Gadgetbridge.
I think the easiest thing would be to have a notification to inform the user, that realtime measurement is being done at all, and it should have a "Stop" button to stop it. This would be independent of the content provider of course, it should also happen for the live activity tab. What do you think?
| case DeviceService.ACTION_REALTIME_SAMPLES: | ||
| ActivitySample tmp_sample = (ActivitySample) intent.getSerializableExtra(DeviceService.EXTRA_REALTIME_SAMPLE); | ||
| //LOG.debug("Got new Sample " + tmp_sample.getHeartRate()); | ||
| if (tmp_sample.getHeartRate() == -1) |
There was a problem hiding this comment.
Better use HeartRateUtils.isValidHeartRateValue() here.
|
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:
|
|
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 :)
|
|
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. |
|
@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!
|
|
@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. |
|
I consider your point highly hypothetical :) However feel free to implement it. |
|
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). |
|
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. |
|
What's the current state of this? Can we move this forward? |
|
Hey there, |
|
@lesensei Is this reproduceable? Can you provide logs (privately, if you want)? |
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:
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!