search

espruino / BangleApps

Bangle.js App Loader (and Apps)
https://banglejs.com/apps
MIT License
498 stars 1.17k forks source link

New app: pace #3565

Closed bobrippling closed 1 month ago

bobrippling commented 2 months ago

This allows a runner to view their current pace and, when paused, splits from their run

bobrippling commented 2 months ago

@gfwilliams @thyttan @atjn I keep seeing memory errors with this but don't believe there's a memory leak in my code, any ideas?

Execution Interrupted during event processing.
Interpreter error: [
  "LOW_MEMORY",
  "MEMORY"
 ]
New interpreter error: LOW_MEMORY,MEMORY

And when I reset:

>reset()
=undefined
FW addr 0x0022afac fail
Status 0
FW addr 0x0022bbb0 fail
Status 0
FW addr 0x0022bbd0 fail
Status 0
FW addr 0x0022bc00 fail
Status 0
Execution Interrupted during event processing.
Interpreter error: [
  "LOW_MEMORY",
  "MEMORY"
 ]
thyttan commented 2 months ago

I'm quite ignorant about the layout module. I suppose its clear function wouldn't help?

bobrippling commented 2 months ago

I'm quite ignorant about the layout module. I suppose its clear function wouldn't help?

Thanks for the quick response! I'm afraid not, that just wipes the layout's part of the screen, but thanks for the suggestion - I'll dig more into the layout module. Although thinking about it, my app isn't using it too differently from others such as rep 🤔

gfwilliams commented 2 months ago

FW addr 0x0022bc00 fail is itself a bit concerning as that means that the Bangle hasn't been able to write to storage, although looking at it, that's probably caused by the MEMORY error. I'll have to look into that as potentially it could be a cause of storage corruption (if you can find an easy way to reproduce this please let me know!).

The app starts completely blank though and need two button presses just to display anything, so I feel like something's not right anyway.

Maybe run https://github.com/espruino/EspruinoMemView and see if you can visibly see what looks like the issue.

But I just noticed:

while (splitDist_1 >= 1) {
                splits_1.push(thisSplitTime_1());

and at no point does anything seem to take anything out of splits_1 so that looks a hell of a lot like a memory leak to me :)

>splits_1.length
=5597
gfwilliams commented 2 months ago

By the way, it's interesting that splits has been renamed to splits_1 and so on. Maybe TypeScript has an option not to do that? It feels needlessly wasteful...

atjn commented 2 months ago

The GPS on my watch has never really worked so I unfortunately can't reproduce the issue. On the other hand, that is a pretty good indicator that your problem lies somewhere in the GPS handling. Taking a quick look at the code, my best bet is that you have made a mistake with this loop, which makes it loop infinitely:

for(; ; i++) {
        const split = splits[i + splitOffset];
        if (split == null) break;     
...
bobrippling commented 2 months ago

FW addr 0x0022bc00 fail is itself a bit concerning as that means that the Bangle hasn't been able to write to storage, although looking at it, that's probably caused by the MEMORY error [...] if you can find an easy way to reproduce this please let me know!

Shall do - as far as I have seen, running the app as it stands reliably raises the error for me, perhaps the gradual increase in small memory usage (i.e. a number) is what brings it about?

The app starts completely blank though and need two button presses just to display anything, so I feel like something's not right anyway.

Interesting - the app for me starts with an initial split shown, that's quite odd!

Maybe run https://github.com/espruino/EspruinoMemView and see if you can visibly see what looks like the issue.

Didn't know about this, amazing

But I just noticed:

while (splitDist_1 >= 1) {
                splits_1.push(thisSplitTime_1());

and at no point does anything seem to take anything out of splits_1 so that looks a hell of a lot like a memory leak to me :)

>splits_1.length
=5597

I believe this is the issue, I expect my units are wrong, so it's adding to a split once per metre instead of once per K (or similar)

By the way, it's interesting that splits has been renamed to splits_1 and so on. Maybe TypeScript has an option not to do that? It feels needlessly wasteful...

I'll see what I can find

The GPS on my watch has never really worked so I unfortunately can't reproduce the issue. On the other hand, that is a pretty good indicator that your problem lies somewhere in the GPS handling. Taking a quick look at the code, my best bet is that you have made a mistake with this loop, which makes it loop infinetely:

for(; ; i++) {
        const split = splits[i + splitOffset];
        if (split == null) break;     
...

Thanks for the pointer, but this is ok - if we had an issue here we'd see the app hanging since it would be an infinite loop :)

bobrippling commented 2 months ago

Switched to using exstats, will be testing this for a little while

bobrippling commented 1 month ago

All sorted - I'm thinking of doing something with run+ so runners can see their splits at the end of their run, is that something that'd be accepted or better kept as another app?

thyttan commented 1 month ago

All sorted - I'm thinking of doing something with run+ so runners can see their splits at the end of their run, is that something that'd be accepted or better kept as another app?

I agree getting that summary at the end might be nice.

For the running layout, what differentiates this from what the run/run+ apps can do - since you can put current pace in one of it's boxes already? Is it the legibility? less taxing on battery?

bobrippling commented 1 month ago

I agree getting that summary at the end might be nice.

  • should it be optional through settings?

Yes I think so

  • EDIT: one may not want it to trigger if:
    • the recording was very short
    • there were no gps data recorded

Also very good points, agreed!

  • eventually, could pace be one of several "summary" apps that could be chosen to auto show?

Yes I like the sound of that, I think I'll keep this as a standalone app for anyone that might want something simpler (or to run on Bangle 1) and see about how we can integrate it into runplus

  • should pace be even more tightly integrated with runplus, so one could navigate to it in much the same way you swipe to the karvonen hr-zones screen?

Yeah that's what I was wondering, I think that'd be cool.

For the running layout, what differentiates this from what the run/run+ apps can do - since you can put current pace in one of it's boxes already? Is it the legibility? less taxing on battery?

Yes, a more lightweight app and you can see two things at once - the main things I need to see when racing are my pace and current time. So perhaps something else we consider for runplus is showing two boxes at once (now we can already show one - #3419), but I don't know if that would start to swell the app.

thyttan commented 1 month ago
  • should pace be even more tightly integrated with runplus, so one could navigate to it in much the same way you swipe to the karvonen hr-zones screen?

Yeah that's what I was wondering, I think that'd be cool.

... So perhaps something else we consider for runplus is showing two boxes at once (now we can already show one - #3419), but I don't know if that would start to swell the app.

I'm inclined to favor a solution like this:

I think this post on the forum is relevant when thinking about this more:

Maybe exstats could be modified so it stores the current run status on exit [It already does], and then you could switch apps while running - which would be a huge benefit anyway. Then it'd be as easy as allowing run to just fast-load another app on swipe and it'd be super configurable? ... but then what the swipe did would need configuring which isn't quite as easy as having it work straight away.

It's a hard one... I feel like maybe a separate app that does it all (run-karvonnen / run+) is best for now, and then if we start to end up with a bunch of fitness apps/duplication we revisit it and see if there needs to be a whole new app type.

bobrippling commented 1 month ago

I'm inclined to favor a solution like this:

  • touch twice on a box to focus only that one.
  • touch once each on two different boxes to display only them together.
  • Edit: if there is not a second touch within some timeout I think the following touch should count as a first touch again - not triggering the behaviour.

I like the sound of that - the delayed first touch counting as a single tap to focus a box sounds great to me.

I think this post on the forum is relevant when thinking about this more:

Maybe exstats could be modified so it stores the current run status on exit [It already does], and then you could switch apps while running - which would be a huge benefit anyway. Then it'd be as easy as allowing run to just fast-load another app on swipe and it'd be super configurable? ... but then what the swipe did would need configuring which isn't quite as easy as having it work straight away. It's a hard one... I feel like maybe a separate app that does it all (run-karvonnen / run+) is best for now, and then if we start to end up with a bunch of fitness apps/duplication we revisit it and see if there needs to be a whole new app type.

I think we stick with how it's been done with karvonnen for now, with the split display and we can split apart if the app does seem to bloat - I can get started on a PR over the next few weeks if that sounds good to you?

thyttan commented 1 month ago

I think we stick with how it's been done with karvonnen for now, with the split display and we can split apart if the app does seem to bloat - I can get started on a PR over the next few weeks if that sounds good to you?

OK! If you want to - no rush of course 🙂

Regarding this PR

All's looking good on the watch now - nice work!

I don't know anything about typescript types, so no comment from me there.

Maybe @gfwilliams could just take a look at the changes to exstats - they seem innocent but I'm not really familiar with it.

bobrippling commented 1 month ago

Thank you! Ah yes - the exstats changes are to allow users/callers to adjust the .next property of the notify object, without it then being overwritten when the callback returns:

https://github.com/espruino/BangleApps/blob/47a8d264abdd80dc34056110758f195c162f9a47/apps/pace/app.ts#L188-L191

bobrippling commented 1 month ago

@gfwilliams are you ok with the changes to exstats ?

gfwilliams commented 1 month ago

Looks good to me. I don't see anything else that uses 'notify' relies on the values so I think we're good?

bobrippling commented 1 month ago

Yes I think so - thanks for the check, @thyttan all good from you?

thyttan commented 1 month ago

Yes!

thyttan commented 1 month ago

Thanks!