Bangle.js: Add activity tracks support #3153
No reviewers
Labels
No labels
device mi band 7
activity post processing
activity/health
Android 12
Android 13
android integrations
architecture
Bangle.js
bug
changes requested
charts
deprecation notice
details not provided
developer documentation
device amazfit band 5
device amazfit bip
device amazfit cor
device Casio
device fossil
device garmin
device gtr 2e
device gts 2 mini
device h30
device hplus
device huami
device Huawei
device liveview
device mi band
device mi band 2
device mi band 3
device mi band 4
device mi band 5
device mi band 6
device no.1 f1
device pace
device pebble
device pebble 2
device pinetime infinitime
device request
device sony
device support
device watch 9
device xiaomi
discussion
documentation
duplicate
enhancement
feature request
Gadgetbridge
good first issue
help wanted
i am developing my own app can you help
icebox
intent api
internationalisation
invalid
needs work
network companion app
new device
no feedback
not a bug
notifications
one of the 1000 issues about disconnection
pairing/connecting
potentially fixed / confirm and close
question
research
security
seems abandoned
Solved, waiting for F-Droid release
suggest to close
task
user interface / UX
wear os
weather
wontfix
Zepp OS
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Freeyourgadget/Gadgetbridge#3153
Loading…
Reference in a new issue
No description provided.
Delete branch "Ganblejs/Gadgetbridge:bangle-activity-tracks"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
WIP - I do work on this from time to time. But I don't know when/if I will get it finished. So if you read this and want to help out I'm happy to pass the torch. Feel free to use the code in this PR, but please leave a note so we don't end up working on it in parallel.
Requested on the Bangle.js forum.
Companion PR to the BangleApps repo: https://github.com/espruino/BangleApps/pull/2819
TODO:
while
tosetInterval
.currently I try to stop the transmission from Bangle - but that does not work yet, logic is wrong somewhere.To work on in a future PR:
setInterval
(currently 50 ms)On the Gadgetbridge changes, take a look into these:
You need to add support for TYPE_GPS_TRACKS on onFetchRecordedData:
this.fetchOperationQueue.add(new FetchSportsSummaryOperation(this, 1));
Which calls the correct operation, fetches the summary + gps tracks and persists the data to the db:
Only Huami devices support this so far. I have attempted to refactor some things in the past to make future devices slightly easier to add, but may have missed a couple of things, feel free to reach out here or on matrix if something looks weird :)
4fa7b4c66a
to17486aee73
17486aee73
tofc77724ffb
@joserebelo
Thank you for the pointers!
I started mimicking in BangleJSDeviceSupport.java. When you get the time please look at the diff and say if I'm on the right track.
I get this error to be solved in some way:
Should it be handled by introducing a
FetchSportsSummaryOperation
class specific to Bangle.js?Yup, that one is specific to Huami devices. You might not need to follow the structure from Huami specifically - I am not yet aware of how we usually fetch data from the Bangle.
Huami has 2 operations (summary and details), maybe the bangle can provide everything in one, for example.
I just copied the huami files to the corresponding banglejs destination. I did some small tweaks to remove errors in Android studio. But not any substantial changes to logic - so it shouldn't work yet.
I also see in the diff that I've removed some imports - I don't understand when though. That should be undone, but not doing it right now.
EDIT: I guess mainly copying and modifying is unnecessarily complex and it's better to do the bangle FetchSportsSummaryOperation class more from the ground up.
Yup - especially if the bangle logic is being build on the watch side from scratch as well, the Huami implementation is overkill.
I imagine something along the lines of this:
t:"fetch", data:"recorder"
or similar.Bluetooth.println(JSON.stringify({t:"push", data:"data read from e.g. recorder.log20230610a.csv on Bangle.js storage"}))
The recorder app: https://github.com/espruino/BangleApps/tree/master/apps/recorder
Recorder app stores data in files named like "recorder.log20230610a.csv", where the ending letter is incremented when there are new recordings within a day.
Contents of recorder files:
Time,Latitude,Longitude,Altitude,Heartrate,Confidence,Source,Battery Percentage,Battery Voltage,Charging,Steps,Barometer Temperature,Barometer Pressure,Barometer Altitude 1686404996,,,,61,57,,63,3.32226562499,false,0,32.51630350748,1016.57829926224,-27.67358933984
The types and frequency of samples can be changed by the user.
Yup, that is pretty much it.
I think you will have to generate a gpx file somewhere in order to get the map / share button to work, but GB can still keep the raw csv data.
Started working on the Bangle.js side in this PR: https://github.com/espruino/BangleApps/pull/2819
One thing I missed: we probably want to send a timestamp of the latest log that GB has, so that the Bangle can start iterating from that day onward.
42904286d6
to419cb515ab
How I imagine it now:
GB({t:"listRecs"})
to Bangle.js. (the id of last synced track could be supplied in the message to Bangle.js to shorten the returned array in the next step)GB({t:"fetchRec", id:"20230616a"})
. (Here I think some logic to wait between tracks is needed)For (1.) I need to find out how to hook into the circular arrow button on the "Sports Activities" page.
(2.) is largely solved.
(4.) is largely solved.
(3.) and (5.) still needs work and I'm not sure of the best way to do it. Currently I plan to do it inside the
handleUartRxJSON
function inBangleJSDeviceSupport.java
in the casescase "trksList"
andcase "actTrk"
. (see current diff of this PR)Here's the current code on Bangle.js side: https://github.com/espruino/BangleApps/pull/2819/files
See: https://codeberg.org/Freeyourgadget/Gadgetbridge/src/branch/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/ActivitySummariesActivity.java#L384
tl;dr: It calls
onFetchRecordedData
on the support class withRecordedDataTypes.TYPE_GPS_TRACKS
.Thanks!
I'm inching closer to some naive solution. Gadgetbridge now receives the csv files fine, basically. And also keeps track of the ID of latest synced recorder log.
Next steps are to:
EDIT:
984b6d6301
tode85400d7c
I'm stuck on how to get the data into the database from the received csv files. @joserebelo do you have some suggestion or another pointer? I guess I should be able to work it out by looking at the huamisupport class, but it's not easy for me.
And also I don't currently know how to generate gpx files from the gps data.
@Ganblejs it's a somewhat weird part of the code, it took me a while to make sense of it.
BaseActivitySummary
, where you can set some data such as the name, start and end times. I believe the only mandatory fields are start time, end time, and activityKind. This class is auto-generated.You then must persist it to the database - here is an example on Huami, lines 98 to 109. You can probably ignore the part about the summaryParser and raw summary data / binary data.
This summary can also contain a rawDetailsPath - here is where you should save the full csv fetched from the Bangle. In Huami we do it separately (see here).
On order to generate the gpx, you must populate an ActivityTrack. You can then use a GpxExporter to write it to disk, and set the gpxTrack on the summary. We do it here for Huami.
On Huami devices, step 1 is done separately from 2 and 3 because of how the watches work. For the bangle, you can probably persist everything in one step, since you have all the csv data from the start.
Thanks, that will help me a bunch I think!
de85400d7c
toa3db96ffb3
624a3bd4f1
to9eb78dcc1a
I now manage to persist an activity to the database. It has the correct date and time set, but I don't think much else at the moment.
74d876eca7
to7cdbd1ce69
c27aacbde4
tof8451d560a
f8451d560a
to63c6a0a56f
I now have the GPS points show up in the "Sport Activity Detail" page:
7e28dc645e
to5967399c43
How do I set details so they show up like in the docs? :
I don't really see how I would add most of this (e.g. steps) to a
BaseActivitySummary
,ActivityTrack
orActivityPoint
. I do manage to add gps-data and hr-data to theActivityPoint
, and they then show up in theActivityTrack
and the exported gpx-file - but the hr-data is not displayed on my "Sports Activity Detail" page.@Ganblejs you need to set the summaryData object, which is a json string. The object contains a set of keys, each key to an object with a value and a unit. Eg.
Here are the available keys:
private JSONObject createActivitySummaryGroups(){
And where we set them on HuamiActivitySummaryParser (see addSummaryData and its usages): https://codeberg.org/Freeyourgadget/Gadgetbridge/src/branch/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/devices/huami/HuamiActivitySummaryParser.java#L384
This is one of the things that is on my TODO list for a while: the HR data on activity details pages is not pulled from the activity points, but only from the synced activity data - which from what I see, the Bangle does not yet support.
We should eventually update this details page to use the data from gpx if available, and only then fallback to the activity data.
Ok, thanks again!
WIP: Bangle.js: Add activity tracks supportto Bangle.js: Add activity tracks supportBangle.js: Add activity tracks supportto Bangle.js: Add activity tracks supportI think we should start considering breaking some functionality to standalone classes, this switch/class is becoming huge.
Something like a BangleActivityTracksService, where we could just handle everything related with tracks in a self-contained manner.
Maybe not on this PR.
Bangle.js: Add activity tracks supportto WIP: Bangle.js: Add activity tracks supportSounds sensible to me. There's already this PR for some refactoring: #3149
5967399c43
todba4b7c5f0
@ -675,0 +872,4 @@
} catch (FileNotFoundException e) {
throw new RuntimeException(e);
} catch (IOException e) {
throw new RuntimeException(e);
This crashes GB :)
We should probably handle it a bit more gracefully.
Thanks! :) Still very much a work in progress :P
I realize I probably haven't fixed this yet. I might have hit it some.
@ -1305,6 +1552,29 @@ public class BangleJSDeviceSupport extends AbstractBTLEDeviceSupport {
@Override
public void onFetchRecordedData(int dataTypes) {
if (dataTypes == RecordedDataTypes.TYPE_GPS_TRACKS) {
One thing I forgot to mention: We probably want to mark the bangle as busy as this operation starts (
getDevice().setBusyTask("Operation name");
), and unmark it as it finishes (getDevice().unsetBusyTask();
)This should show the operation spinner both in the main activity card, as well as a spinner in the activity list, and prevent multiple fetches from being triggered in parallel.
I just had a go at this with this commit:
14415215fe
... and this one to the companion PR:
20ce944469
Before this I would get a perpetual spinner in the activity list which would go away when I navigated away from that page. With the change I did just now I will see no spinner at all.
I'll not worry too much about this currently. But before a potential merge of this PR the spinner should work.
Is it so fast that the spinner does not even have time to show? 🤔
No I don't think so. If I clear the data in Gadgetbridge and fetch all my logs again, the logcat shows the process is taking more than 10 seconds.
But lets worry more about this when the new code is closer to being finalized 👍
14415215fe
to94e7b1cdb0
94e7b1cdb0
toa323776c67
@joserebelo, am I correct in assuming then that there's no function in Gadgetbridge to calculate distances/speeds/etc from the gpx-file for adding to the summary? I've tried to understand
GpxParser.java
and I don't think it is used for this?@Ganblejs yup, you are correct. GpxParser really just reads a gpx file to a GpxFile object, but that's really just a wrapper for the tracks and waypoints, there's currently nothing to build statistics from that :/
If I were to make a class for extracting statistics from gpx-files, would it be good to use GpxParser inside it? Or is it not suited for that and it's better to build more from the ground up (will be a naive implementation if I make it)?
Either having the GpxParser inside or receive a GpxFile as argument, both should be fine. I tried to make that class reusable for anything gpx-parsing-related, as this :)
a323776c67
to09821065d8
I seem to have broken this PR/fork. How do I mitigate that? I force pushed yeaterday and that has not showed up here.
Edit: Looking at the branch on my fork in the web ui the force push is shown there, but it's not forwarded to this PR.
I suppose I could just close this PR and open a new one.
Maybe try to force-push again?
It seems like Codeberg was going through some hiccups, this is not the only PR affected..
@ -695,0 +797,4 @@
JSONArray valueArray2 = new JSONArray();
// Add analytics based on GPS coordinates.
Weirdly I get in the logcat:
Which seems like it should come from the
handleUartRxLine
function. The error goes away if I comment out the analytics below. So I guess I must indirectly use that function somewhere in these calculations. But I can't understand why that would be. Will have to look at it more. Maybe the whole operation is wrapped insidehandleUartRxLine
, I guess that could be it...The
Value at 13 of type java.lang.String cannot be converted to double
part of the logcat has said stuff aboutNaN
andinfinity
before, but I seem to have resolved those by turning some zeros to0.001
and changing anull
to0
.It's cumbersome since it will not give me the line number where the problem occurred...
I suspect it is the new
sumOfJSONArray
function et al that are acting up on bad inputs and throwsJSONException
.BangleJSDeviceSupport in handleUartRxLine, I would replace the log of
UART RX JSON parse failure
with:ec879373d0
toede5445e21
ede5445e21
to365bc7b85f
Progress so far:
Edit: added a new screenshot with todays progress.
cdc3ae8da8
to72fae57665
72fae57665
to458cb14c5d
@gfwilliams @joserebelo Do you have any thoughts on this:
Often (but far from always) when I fetch recorder logs the json received by Gadgetbridge will come in malformed where parts of the string was missing. I'm unsure if the problem lies in Bangle.js or with Gadgetbridge.
If possible I'd like to solve the problem, I tried to change from using a while-block to an interval in bangle but that didn't work well (commented out here: https://github.com/espruino/BangleApps/pull/2819/files).
If I can't solve it I'd have to implement logic to back up and refetch the log currently being synced. If so I'd like to be able to cut short the currently executing while-loop on the bangle to restart it - but I don't know if that will be possible?
Edit: Added a logcat that caught a bunch of "Malformed JSON" errors, search for that in the attached file to look at some examples.
@Ganblejs I have seen this happen once or twice in the past, but I attributed it to me doing something wrong. Definitely hasn't been happening enough to cause issues like that :/
Sorry for the delay - honestly this is a strange one. It looks like we've just totally missed a packet (which shouldn't happen!). You can see from:
It just hasn't received anything from BtLEQueue between, where there should have been the end and start of a new object. I don't believe it's a Bangle.js/Espruino issue as when using WebBLE we can back the whole watch up without losing any data. @joserebelo do you recall anyone reporting issues in Gadgetbridge with 'lost' data notifications?
Interestingly fanoush on the forums/GitHub noticed yesterday I'd commented out some code that could have pushed 5x more data through BLE with a note saying that Nordic's Cloud Gateway app lost data if >1 packet was sent per connection interval. I wonder if it's related - it is possible that occasionally we could accidentally send >1 packet per connection interval and then maybe Gadgetbridge could lose it?
Moving from a while loop to an interval would be much better from the point of view of not locking up the Bangle while we're receiving the track data (I don't think users would appreciate the whole watch not working for 1+ minute during the upload), but I don't think that will solve the underlying problem you have there.
But as you're wrapping the CSV file line in JSON, maybe you could add a line counter in there too, and then if you lost a line you could request the transfer restarted from that line on? It wouldn't fix the underlying problem but would work around it.
Also, I could imagine you have a situation where the watch is downloading and then the user wants to switch apps, and that itself ends up cancelling the upload and you have to restart it anyway.
So maybe I'd suggest:
setInterval
like you'd done - that looks good, but you want to ensure you store fetchRecInterval and clear it next timefetchRec
is called.actTrk
event? It would reduce a bit of overhead and would make the upload faster (as well as not having to interrupt Bangle.js so often)Also, about the data loss: I could try doing a build that does push more data out per connection interval, and I guess that might really break things which would help track it down more.
I did spot this thread: https://stackoverflow.com/questions/24817107/android-receiving-multiple-ble-packets-per-connection-interval
This doesn't entirely make sense since we're not seeing duplicate events with the same value, but could this be related I wonder? On StackOverflow it feels like a bunch of people are complaining about Android dropping notification events, but then Chrome seems to work just fine so whatever they did it works...
It's also interesting that we only seem to get 20 bytes per event (and Bangle.js could send far more - 128 iirc?) so it looks like we could add:
in
initializeDevice
like is done for PineTime and we could massively increase download/upload speed.Thanks for your comments!
I'll look at getting those changed/added!
I don't know the limit here, how many lines should I expect to be able to send per event?
That could be good I guess, so I don't have to do repeated fetches hoping to get one that breaks.
I have certainly noticed especially firmware updates takes eons from my phone, where it is relatively quick from my laptop.
It's all a bit of a bit hand-wavey. I'd say it'd be nice if we didn't end up adding more than 10-20% overhead in the JSON that we send, so maybe just send 4 lines at a time?
Yes - I'll see about getting that change put in
edit: it's in now
Just a note to say that sometimes when I'm connected
Bangle.js watch <-> Gadgetbridge Web IDE Remote Access <-> Web IDE on my laptop
, uploads will come in with some characters missing, breaking the code.That seems to be the same bug discussed above here.
You mean when sending data to the Bangle? That one's actually the opposite problem :(
Yes. Most recently when uploading the
messagegui
with the modifications for swipe up/down (edit: from the Web IDE right hand side window).I managed to upload it on the subsequent try though.
I think you'd need to look at a Gadgetbridge log when it happens - to see whether the requests from the WebView were missing data, or it was the Internal
Intent
that got lost (which is my guess - I feel like I've seen that before) or whether it was going all the way through to BtLEQueue and then getting lost.I've found the section of the log where it happened but haven't gone through to identify where the problem occurs yet (will get to it later). I'll send the log snippet in a message to you @gfwilliams on the espruino forum.
Sorry, but I don't really have the bandwidth to look at that one at the moment - if you can figure it out it'd be great but I've just got too much on at the mo
Just looked a bit closer at the above:
So what's happened is it was in the middle of the upload, and you received a notification, which sent out
GB({"t":"notify","id":1709118388,"src"...
right in the middle of the stream of data coming from the IDE.It could happen with the app loader too.
So it's not actually losing any data anywhere, it's inserting it where it shouldn't!
So we could detect if we'd been asked to send any data by the WebView in the last 5 seconds, and if so could delay sending any notifications until we were sure nothing had been sent for a while.
Or in fact we could just refuse to send any data to the Bangle from Gadgetbridge when the WebView is active, and send it all when it's closed...
9b5328c5fe
toe02e7aed77
LOG.info
A screenshot where activity and hrm data is shown to be pulled from the 'general' activity data in is attached. Summary details and a mapping of the gps points also shown.
a9fce966ec
toffda7b535d
Fetch
recorder
logs logic:@gfwilliams
The
setInterval
works wonders now - thanks for the tip!Some kind of packet counter implemented. Not sure if I did it like you meant. But it should help in the event we miss a packet. Also - I haven't had the
malformed JSON
problem since gettingsetInterval
to work even without packet counting.4 lines a packet works well, didn't try with more.
I still haven't added the ~5 sec timeout for if the transmission was interrupted. Currently I don't know how but shouldn't be too hard to work out.
So with that I think we're close to being done with transferring and storing
recorder
logs!My biggest problems now are:
@joserebelo if you get the time I will appreciate if you have some thoughts on those :) I might be able to work them out eventually though.
(I did not yet managed to look into the current diff of this PR, just going from the questions :))
You can just call
device.sendDeviceUpdateIntent
, which should reload it:case GBDevice.ACTION_DEVICE_CHANGED:
Although I've noticed on some of my devices that that doesn't work 100% of the time. I'm thinking we should update this to react to
GB.signalActivityDataFinish
, but did not yet managed to look into that.Yup, it should go away once you unmark the device as busy + send the previous intent.
We are lacking this in other devices as well.. The reset happens in
for(GBDevice device : devices){
I think we should split this into a reset for activity fetch time and sports fetch time, since it may actually happen that one of these fetches is not idempotent for some devices, and will cause data to become messed up / duplicated.
Sorry in advance, it's probably a little unwieldy... (however I did finally pull it out to a separate class/file) Don't hesitate to ask questions if you start looking at it - either here or on matrix chat.
Absolutely awesome - thank you!
It did!
OK, interesting :)
Thanks again!
This looks great - thanks!
If you can, the timeout might be good - now we're using setInterval it's actually pretty likely it would be interrupted - and alarm, incoming message(sometimes) or the user changing app could well interrupt it and we don't want Gadgetbridge to get confused (especially given someone's experiencing problems on https://forum.espruino.com/conversations/370102/#comment17306552 - unrelated, but I guess it shows stuff like that can happen)
@ -97,3 +97,3 @@
swipeLayout.setRefreshing(true);
} else {
boolean wasBusy = swipeLayout.isRefreshing();
boolean wasBusy = swipeLayout.isRefreshing(); // FIXME: This check of swipeLayout.isRefreshing does not cover the case where fetching was initiated by user clicking the blue button with a circle-arrow. In that case no auto-reload will happen.
@joserebelo This check of swipeLayout.isRefreshing does not cover the case where fetching was initiated by user clicking the blue button with a circle-arrow. In that case no auto-reload will happen.
I wonder if it would be better to just not do the check and just refresh() every time we end up in the else-statement?
resolved by adding
swipeLayout.setRefreshing(true)
in thefetchTrackData()
function.@ -0,0 +66,4 @@
};
private Timer timer = new Timer("Activity Fetching Timeout");
timer. // FIXME: I don't get any hints for timer here, so I must be doing something wrong.
I don't get Android Studio to help me with
Timer
+TimerTask
... Do you know how to set upTimer
-s in Java/Android @gfwilliams ? I tried reading the Android docs and watching a youtube video on javaTimer
/TimerTask
. Android studio would not provide me the method hints fortimer
.Just started trying with these instructions: https://examples.javacodegeeks.com/android/core/activity/android-timertask-example/
Hopefully that works.
Sorry I'm a bit late to this - I don't have any experience with timers though I'm afraid. My Android experience is pretty minimal
It works now :)
698f08bfdf
to1ea3639c36
WIP: Bangle.js: Add activity tracks supportto Bangle.js: Add activity tracks support@gfwilliams @joserebelo @halemmerich
I feel like this could be good enough to go in now or soon. My testing indicate all UI stuff work now. We could eventually introduce more of the summary data entries - but I don't want to work on it now. What's there is already useful. See attached screenshot.
So if you get the time, please review the code and/or try the functionality and provide feedback! Thanks in advance :)
The required modifications to
Android Integration
Bangle app can be loaded from my app loader: https://thyttan.github.io/BangleApps/?q=android@ -90,0 +97,4 @@
* @param dst the file to write to
* @throws IOException
*/
public static void copyStringToFile(String string, File dst, String mode) throws IOException{
If there's an exception, this will leak the file.
This should probably be:
ensuring the writer gets closed, but we still propagate the exception.
I tried refactoring with your suggestion in mind - does it look good now?
It will still leak. For example, if
writer.write
throws an exception,close
will never be called.If you use the snippet I added in the comment, it will not leak, as the writer will be closed automatically by the try-with-resources call.
But Android Studio doesn't like that snippet, marking as error.
Screenshot here: https://codeberg.org/attachments/589c0748-92bb-4107-a9bf-0480e77779ff
Sorry -works now I think...
Just a few notes, will take a more in-depth look at the BangleJSActivityTrack during this week and try to test it.
@ -0,0 +44,4 @@
import nodomain.freeyourgadget.gadgetbridge.util.FileUtils;
import nodomain.freeyourgadget.gadgetbridge.util.GB;
class BangleJSActivityTrack extends BangleJSDeviceSupport {
Extending BangleJSDeviceSupport shouldn't be needed - why do we need that?
Removed the extends modifier.
@ -0,0 +422,4 @@
//summaryData = addSummaryData(summaryData,"caloriesBurnt",3,"mm"); // TODO: Should this be calculated on Gadgetbridge side or be reported by Bangle.js?
//summaryData = addSummaryData(summaryData,"totalStride",3,"mm"); // FIXME: What is this?
if (storedLogObject.has("Heartrate")) {
summaryData = addSummaryData(summaryData, "averageHR", averageOfJSONArray(storedLogObject.getJSONArray("Heartrate")), "bpm");
FYI, I extracted quite a few of these string constants to https://codeberg.org/Freeyourgadget/Gadgetbridge/src/branch/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/model/ActivitySummaryEntries.java, for both the data types and units.
I changed to reference those instead - thanks!
@ -0,0 +782,4 @@
String dayString = String.valueOf(day);
if (day<10) dayString = "0" + dayString;
return yearString + monthString + dayString;
You can simplify the 0-padding:
return String.format("%d%02d%02d", year, month, day);
Did that - thanks!
@ -513,2 +513,4 @@
}
private JSONArray tracksList;
private int packetCount;
I feel like we could make BangleJSActivityTrack keep the state, and move these 2 variables to there, so that all the logic is encapsulated there.
Just a suggestion though, would not block the PR on this.
As above, but if we're keeping these, renaming might be nice as it's not apparent that
packetCount
is specifically for tracks?I think I've achieved most of this now. Will leave a review comment re returning tracksList since uartTxJSON can't be called from static context.
@ -1387,3 +1406,3 @@
}
if ((dataTypes & RecordedDataTypes.TYPE_DEBUGLOGS) != 0) {
if (dataTypes == RecordedDataTypes.TYPE_GPS_TRACKS) {
dataTypes is supposed to be a bitmask, although I haven't yet checked how well the bangle would handle fetching multiple data types at once.
Still, it might be better to replace this with (dataTypes & RecordedDataTypes.TYPE_GPS_TRACKS) != 0` and return inside the if.
I hope I do it the right way now?
@ -1390,0 +1409,4 @@
JSONObject requestTracksListObj = BangleJSActivityTrack.compileTracksListRequest(getDevice(), getContext());
uartTxJSON("requestActivityTracksList", requestTracksListObj);
}
if (dataTypes == RecordedDataTypes.TYPE_DEBUGLOGS) {
Same here, should be seen as bitmask.
Updated this too, is it good now?
Yup, looks good.
Thank you @joserebelo - I'll look at those!
This looks great! @joserebelo knows Gadgetbridge way better than me and his suggestions looks great - but from the point of view of how this all works and integrates with Bangle.js I'm very happy and with those tweaks suggested above I'd think we should merge?
@ -0,0 +636,4 @@
} catch (IOException e) {
throw new RuntimeException(e);
} catch (JSONException e) {
throw new RuntimeException(e);
#3153 (comment)
Said by @joserebelo
Please advice on how I best handle/refactor this.
I've now changed from throwing RuntimeException to logging the exceptions as errors.
1ea3639c36
toa69f8832cf
@ -562,1 +562,4 @@
break;
case "actTrksList":
JSONObject requestTrackObj = BangleJSActivityTrack.handleActTrksList(json, getDevice(), getContext());
uartTxJSON("requestActivityTrackLog", requestTrackObj);
If I could instead call
uartTxJSON
from insideBangleJSActivityTrack.handleActTrksList
that would look nicer, and not need the returnedrequestTrackObj
above. But that would need changing of modifiers foruartTxJSON
, I believe (E.g. Android Studio would complain on calls of non-static method calls from static contexts). But that trigger a cascade of further needed changes it seems...What do you think @gfwilliams @joserebelo, do you have suggestions?
I seems to me like it's theswitch - case
statement that enforces/requires the static modifier. If we changed toif
statements maybe it wouldn't be required anymore anduartTxJSON
could be called from the new class?After trying - that was not it.
It's probably easier for you to add the bangle support as an argument on the constructor of the BangleJSActivityTrack, that way you can call back that function directly.
It's not the cleanest way to implement this (it introduces a circular dependency between these 2 classes), but given the current Gadgetbridge architecture I think it might be the simplest.
Do you think it's better to just leave it like I've done it in the current state of the PR? Or better to do the change you describe?
I think the current way is fine.
While maybe it doesn't seem ideal, the benefit is it's clear from reading the code that when
actTrksList
is received something else is transmitted back - if it were hidden in handleActTrksList you might not see that.If anything it'd be nicer I guess if we made
uartTxJSON
's first argument actually be the value oft
that should be in the object, so then you can tell at a glance what packet is sent - but that's not a change for this PR!Just a note to say that the
FileUtils
andActivitySummariesActivity
related commits should not be squashed together with the Bangle specific stuff. I can do separate PRs for them if preferred, but maybe just eventually have three commits on this PR - Bangle specific, FileUtils, and ActivitySummariesActivity.e54fe7957d
to2a45b91a52
@ -562,1 +562,4 @@
break;
case "actTrksList": {
JSONObject requestTrackObj = BangleJSActivityTrack.handleActTrksList(json, getDevice(), getContext());
uartTxJSON("requestActivityTrackLog", requestTrackObj);
requestTrackObj can be null, which is sending
GB(null)
and throws an error.@ -0,0 +543,4 @@
boolean hasHRMReading = false;
for (int i = 0; i < storedLogObject.getJSONArray("Time").length(); i++) {
timeOfPoint.setTime(storedLogObject.getJSONArray("Time").getLong(i)*1000L);
point.setTime(timeOfPoint);
All the gpx points are ending up with the same timestamp, because the date object is used directly.
Maybe change this to
point.setTime(timeOfPoint.clone());
@ -0,0 +110,4 @@
//GB.toast(context, "actTrk says hi!", Toast.LENGTH_LONG, GB.INFO);
String log = json.getString("log");
LOG.debug(log);
String filename = "recorder.log" + log + ".csv";
The probability of a collision here is higher than on other devices (if the user has multiple bangles), since the resolution is the day + a letter.
Should we consider saving these to a folder with the mac address as the name? (maybe replacing : with _ or similar.
Sounds sensible - what does @gfwilliams say?
Maybe we just use the four last characters as it's done on the preinstalled widget?
I now add the four last (excluding
:
) characters of the mac address to the files. But think I like the the thought of putting them inside a folder instead.Ok - now I store all files in a device specific subfolder (named with
getDevice().getName()
) so I removed the four appended mac characters.@ -0,0 +115,4 @@
try {
dir = FileUtils.getExternalFilesDir();
} catch (IOException e) {
resetPacketCount();
I think we should log the error here, might make it tricky to troubleshoot if it does fail.
Added an error, but the string should be referenced from strings.xml - I have not done that now.
I don't think it should to be honest - I meant just a LOG.error("something"), no need for a toast imo.
@ -0,0 +592,4 @@
break;
}
String fileName = FileUtils.makeValidFileName("gadgetbridge-" + trackType.toLowerCase() + "-" + summary.getName() + ".gpx");
Same here - I think there's a somewhat high probability of collision if we use the summary name.
Maybe we could use the timestamp of the first point as the activity date?
I added that timestamp but also kept the bangle style stamp as well - for now at least. Makes it easier to match it to the csv and name in Gadgetbridge.
I changed to adding the last four characters of the mac-address (not including
:
) of the device, which I read from the end of the device name (getDevice().getName().substring( ... )
).Ok - now I store all files in a device specific subfolder (named with
getDevice().getName()
) so I removed the four appended mac characters.Thank you again @joserebelo - will take a look!
95440ed95e
to12c73954c6
12c73954c6
tob91cef6d6d
Screenshot showing error in Android Studio from suggested snippet is attached.
Edit: was just a parenthesis mismatch.
@joserebelo I think I have made changes that resolve your latest review comments (and marked them as such).
12f5b40baa
to8794878755
@Ganblejs thanks! This was quite a big effort.
Thanks again for the guidance and feedback @joserebelo and @gfwilliams !
And sorry for the onslaught of commits that went into master 😅
You had mentioned not to squash everything into a single commit, and I forgot to ask to squash into the 3 commits :')
Anyway, it's fine.
I suppose you don't want to reset master to before the merge? I could squash into three commits now if preferred.
@Ganblejs no need, resetting master is not a very good practice, and the commits messages are clean.