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

New app: pace #3565

Merged
merged 26 commits into from
Oct 18, 2024
Merged

New app: pace #3565

merged 26 commits into from
Oct 18, 2024

Conversation

bobrippling
Copy link
Collaborator

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

@bobrippling
Copy link
Collaborator Author

@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"
 ]

apps/pace/app.ts Outdated Show resolved Hide resolved
@thyttan
Copy link
Collaborator

thyttan commented Sep 10, 2024

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

@bobrippling
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

atjn commented Sep 11, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@bobrippling bobrippling marked this pull request as ready for review October 14, 2024 17:28
@bobrippling
Copy link
Collaborator Author

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?

apps/pace/app.ts Outdated Show resolved Hide resolved
@thyttan
Copy link
Collaborator

thyttan commented Oct 14, 2024

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.

  • should it be optional through settings?
  • EDIT: one may not want it to trigger if:
    • the recording was very short
    • there were no gps data recorded
  • eventually, could pace be one of several "summary" apps that could be chosen to auto show?
  • 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?

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
Copy link
Collaborator Author

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.

@@ -0,0 +1 @@
require("heatshrink").decompress(atob("mEwwkBiIA/AH8QRYoX/AAcAZqwXuMIQYCiAcOO456OCwwACF5orEDghzOE4oZCLxw+GDAKsXeSIqEGBKKGBwIRFGBCfIC7p5RC7oGHC8RxFC453JAw4Xda5AIEC5IAPO6AXmO5BuHC67OIUA4aERyIXEIxAAJiAuFC5gTEcgpDNBwoWCIxp4CCwp1OCIYAEOiDFNDBwVQAH4AvA="))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This draws black on black for me in dtlaunch (Desktop Launcher) when using Dark BW system theme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, thanks! Sorted

apps/pace/app.ts Outdated

const drawSplit = (i: number, y: number, pace: number | string) => {
g
.setColor("#fff")
Copy link
Collaborator

@thyttan thyttan Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use g.theme.fg, right (or some other solution)? On Light BW theme it's white on white for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good spot!

@thyttan
Copy link
Collaborator

thyttan commented Oct 15, 2024

  • 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:

  • 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 change.

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
Copy link
Collaborator Author

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
Copy link
Collaborator

thyttan commented Oct 16, 2024

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
Copy link
Collaborator Author

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:

BangleApps/apps/pace/app.ts

Lines 188 to 191 in 47a8d26

// subtract <how much we're over> off the next split notify
exs.state.notify.dist.next -= thisSplit;

@bobrippling
Copy link
Collaborator Author

@gfwilliams are you ok with the changes to exstats ?

@gfwilliams
Copy link
Member

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

@bobrippling
Copy link
Collaborator Author

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

@thyttan
Copy link
Collaborator

thyttan commented Oct 18, 2024

Yes!

@thyttan thyttan merged commit 5060403 into espruino:master Oct 18, 2024
1 check passed
@thyttan
Copy link
Collaborator

thyttan commented Oct 18, 2024

Thanks!

@bobrippling bobrippling deleted the feat/pace-app branch October 18, 2024 11:31
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.

4 participants