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

StopWatch: add persistence #2141

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

codingjourney
Copy link

@codingjourney codingjourney commented Oct 20, 2024

This PR solves Issue #303 and is based on previous work by @desttinghim (PR #783 ) and @pptime02 (PR #1410 ). Well, it's actually just a re-base of #1410 onto the current main branch. I did my best not to disrupt functionality implemented on main in the past 2 years. Any feedback is appreciated.

Copy link

github-actions bot commented Oct 20, 2024

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Haven't tested on hardware, but this is looking really nice overall. Thanks for pulling together the previous PRs :)

src/components/stopwatch/StopWatchController.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/StopWatch.cpp Outdated Show resolved Hide resolved
src/components/stopwatch/StopWatchController.h Outdated Show resolved Hide resolved
src/components/stopwatch/StopWatchController.cpp Outdated Show resolved Hide resolved
src/components/stopwatch/StopWatchController.h Outdated Show resolved Hide resolved
src/components/stopwatch/StopWatchController.h Outdated Show resolved Hide resolved
src/components/stopwatch/StopWatchController.h Outdated Show resolved Hide resolved
src/components/stopwatch/StopWatchController.h Outdated Show resolved Hide resolved
@mark9064
Copy link
Member

@FintasticMan Can you trigger CI?

@codingjourney
Copy link
Author

Added a fix for a minor issue: after exiting StopWatch in the Paused state and then reopening it, the displayed time would shift by 4, sometimes 5 hundredths of a second. I think this is the delta between the last rendering and the moment we switch state to Paused and set timeElapsedPreviously. (The delta is consistent with the ~24 Hz refresh frequency I've observed.) The fix simply renders time once more once we are Paused.

src/displayapp/screens/StopWatch.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/StopWatch.cpp Outdated Show resolved Hide resolved
Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Looks great! Tested on hardware and works flawlessly

Thanks for sticking through the many rounds of feedback 😅

src/displayapp/screens/StopWatch.cpp Outdated Show resolved Hide resolved
@codingjourney
Copy link
Author

By the way, I've noticed this PR breaks InfiniSim. Does that have to be adapted before things get merged in InfiniTime? I don't mind looking into it, it's just that the build setup seems a bit more involved (I went with the Docker method for InfinTime and that's super smooth).

@mark9064
Copy link
Member

Yeah it's good practice to get an InfiniSim PR up and waiting if a PR here will break it (annoyingly the CI has given up again so I can't easily see). I haven't used the docker method so I can't comment on that but provided you're on Linux getting a build environment up and running shouldn't be too bad.

@codingjourney
Copy link
Author

I've created PR #163 for InfiniSim. Necessary changes were trivial. Functionality in InfiniSim seems to work as intended.

@mark9064
Copy link
Member

mark9064 commented Nov 2, 2024

Great :)

@mark9064 mark9064 added this to the 1.16.0 milestone Nov 17, 2024
@mark9064 mark9064 added the enhancement Enhancement to an existing app/feature label Nov 18, 2024
@mark9064 mark9064 added the new feature This thread is about a new feature label Nov 18, 2024
if (lap) {
TimeSeparated laptime = ConvertTicksToTimeSegments(lap->timeSinceStart);
char buffer[16];
sprintf(buffer, "#%2d %2d:%02d.%02d\n", lap->number, laptime.mins, laptime.secs, laptime.hundredths);
Copy link
Member

@mark9064 mark9064 Nov 20, 2024

Choose a reason for hiding this comment

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

Maybe use snprintf with the buffersize fixed here? Since the number of laps can now exceed two digits the length is now variable and could overflow the buffer

Also, do you think the hour could be fit in the lap time here? I'm not sure how it'll look, but I think there might be enough horizontal space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature new feature This thread is about a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants