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

Calculator App #375

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

Calculator App #375

wants to merge 48 commits into from

Conversation

Raupinger
Copy link
Contributor

@Raupinger Raupinger commented May 20, 2021

A calculator based on the Shunting-yard algorithm as described here. The basic functionality is in place but some work still needs to be done:

  • Second page of buttons for braces, pow etc.
  • Testing various edge cases
  • Haptic feedback for invalid operations
  • Clean up the UI
  • Make sure things like Precedence are handled correctly

this resolves #227

@Raupinger
Copy link
Contributor Author

Video

2588-10mB.mp4

@Raupinger Raupinger mentioned this pull request May 20, 2021
@Raupinger Raupinger force-pushed the Calc branch 3 times, most recently from 3fe992e to 5f4c65c Compare May 22, 2021 22:03
@Raupinger Raupinger changed the title WIP: Calculator App Calculator App May 23, 2021
@Raupinger
Copy link
Contributor Author

Raupinger commented May 23, 2021

Ok, I'm declaring this "ready for review" now. I cant find any bugs anymore and spent hours trying to reduce the memory usage. One obvious thing that could still be done is parsing the numbers while putting everything on the input stack. that would save n*(k)+31 bytes, where n is the number of digits a number has and k is the number of bytes a single digit on the stack takes up. in praxis it should be at the lower range of that. Overflows are not handled. The eval function has a lot of resemblance with Italian cuisine, so i wouldn't be surprised if there are still some errors in it. I couldn't test that the motor actually works but i don't expect any surprises there.

@Raupinger
Copy link
Contributor Author

video

@Dudemanjude
Copy link

I've found a bug that causes the watch to reset. If you just enter a period, and then hit equals, it resets. This happens no matter how many periods you add. For example, if you enter just "." and then hit equals, it will reset, and "...." and then equals will also reset. Another case of unwanted behavior happens when you put "1+.1" into the calculator, and hit equals. It says the answer is "2.1", while I would expect it to say "1.1". If you type ".2+.3", it results in "5", which is also unexpected.

The build that I'm running on my watch is a custom one that I compiled from your Calc branch, plus a custom Terminal watchface from a different repo. I'm a beginner when it comes to Git and compiling InfiniTime, so it's possible that my build is messed up, although everything else seems to work just fine.

Otherwise, the app is great, and I hope that once these bugs get sorted out, we can have a calculator built into one of the next versions of InfiniTime!

@Raupinger
Copy link
Contributor Author

Yeah, that sounds plausible. Currently there's no handling for periods that dont appear after some digits, so they just act as if nothing was there at all. The reset can be explained by trying to evaluate an empty expression. I should probably check for that and either interpret it as a zero or treat it as an invalid expression, vibrate and return

src/displayapp/screens/Calculator.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/Calculator.cpp Show resolved Hide resolved
src/displayapp/screens/Calculator.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/Calculator.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/Calculator.cpp Show resolved Hide resolved
@Raupinger
Copy link
Contributor Author

Thanks @jonvmey for all those reviews

@Raupinger Raupinger force-pushed the Calc branch 2 times, most recently from 7eecd9c to 1b18a63 Compare June 23, 2021 03:31
@Izaic
Copy link

Izaic commented Aug 29, 2021

Is this just waiting to be merged or?

@Raupinger
Copy link
Contributor Author

Its should be ready. Its just been long enough that I would probably have to rebase it again

@KoalaV2
Copy link

KoalaV2 commented Nov 12, 2021

Any updates on this?

@Raupinger
Copy link
Contributor Author

Any updates on this?

It's basically done. At the time JF was to busy to take a look at this and its just kind of been sitting here since. Doesn't help that I've not been active on Infinitime for a while now. Rebasing it doesn't look to bad assuming it doesn't run into memory limits again.

@fossdd
Copy link

fossdd commented Nov 21, 2021

i would really like that feature on InfiniTime

@Alpha-Titiwu
Copy link

I have successfully tested this nice calculator by rebasing it on the latest develop branch. It would be great, if it could be merged into the official codebase.
@Raupinger could you please resolve the merge conflicts?

@Vistaus
Copy link

Vistaus commented Dec 7, 2021

This would be a handy feature indeed. 🙂

@NeroBurner
Copy link
Contributor

rebased the Calc branch on current develop: https://github.com/NeroBurner/InfiniTime/tree/Calc_rebase

added the lv_simulation branch on top: https://github.com/NeroBurner/InfiniTime/tree/lv_simulation_Calc

Nice for local hacking:
Screenshot_20220226_220416

@Raupinger
Copy link
Contributor Author

Just a quick heads up: Ive had some incorrect results (like 0.3*0.3) and am still not super confident about my overflow checks. I would like to pull out the eval function and write some tests for it before this gets realeased.

@Raupinger
Copy link
Contributor Author

Also, thanks for rebasing

# Conflicts:
#	src/displayapp/Apps.h
#	src/displayapp/fonts/jetbrains_mono_bold_20.c
#	src/displayapp/screens/Symbols.h
#	src/displayapp/screens/settings/Settings.cpp
@Vistaus
Copy link

Vistaus commented Apr 2, 2022

Too bad this didn't get merged yet. I like that the main devs are active, but the main feature in 1.9.0 is a terminal watchface, really? I mean: it looks great and geeky, so I really dig it! :) But if I had to choose, I would've liked to see something that's actually useful, like this calculator app.

@medeyko
Copy link
Contributor

medeyko commented Apr 16, 2022

Just a quick heads up: Ive had some incorrect results (like 0.3*0.3) and am still not super confident about my overflow checks. I would like to pull out the eval function and write some tests for it before this gets realeased.

Had you an opportunity to raise your confidence in overflow checks? If not, could you please write more details?

@JF002
Copy link
Collaborator

JF002 commented Apr 17, 2022

Too bad this didn't get merged yet. I like that the main devs are active, but the main feature in 1.9.0 is a terminal watchface, really? I mean: it looks great and geeky, so I really dig it! :) But if I had to choose, I would've liked to see something that's actually useful, like this calculator app.

Please note that there are a lot of reasons why any specific PR is not merged yet : some are not finished or do not meet the project goals, priorities and/or code quality, they need time and attention from developers to review it and maintain it, they need too much memory and cannot be merged until we can find a way to free some ram/flash memory space,...

As you can see, there are currently ~100 open PR, and I'm really sorry I cannot give them the attention they deserve, but I (along with other core developers) work on this project on our free time, and we cannot possibly review and merge that much PR as soon as they are opened.
Also, remember that the PineTime is a very constrained hardware (in terms of processing power and memory). In the end, it'll be impossible to merge each and every features without hitting processing or memory limitations, so choices have to be made, unfortunately.

I'm sorry you don't find the terminal watchface as amazing as I do, but I'm pretty sure that If we merged this one instead, other people would have said that they don't need a calculator, and that they are sad the new watchface is not released yet.
Also, note that the terminal watchface is not the only addition in 1.9 : 49 PR and 100 commits were merged since 1.8. Maintaining software do not only consist in adding feature visible to the end user. It also consist in fixing bugs, improving the code that is running under the hood, improving performances and maintainability of the code,...

Now, regarding this PR, the first question I'll ask before going any further is : what is the added memory usage (flash and ram ) of this PR ?

@Danimations
Copy link

Just writing to express my support for this feature. I would definitely use it once it's available.

@NeroBurner
Copy link
Contributor

rebased the Calc branch on current develop: https://github.com/NeroBurner/InfiniTime/tree/Calc_rebase

Additionally I refactored the PR into three single commits to make rebases easier when the fonts were changed by other PRs:

  • fonts/README: add calculator symbol to font generation documentation
  • fonts: regenerate jetbrains_mono_bold_20 with calc symbol
  • Add Calculator screen as new App (the changes from @Raupinger )

I propose to replace the current branch content with the new Calc_rebase commits. To do that you can do the following steps (assuming origin is git@github.com:Raupinger/Pinetime.git)

git remote add neroburner https://github.com/NeroBurner/InfiniTime.git
git fetch neroburner Calc_rebase
git switch Calc
git reset --hard neroburner/Calc_rebase
git push --force-with-lease origin Calc

@Raupinger
Copy link
Contributor Author

Thanks!

@Riksu9000 Riksu9000 added the new app This thread is about a new app label May 2, 2022
@rev22
Copy link
Contributor

rev22 commented Jun 10, 2022

I tried to reduce the firmware size and ram usage by not using heap-allocated stacks, and removing the intermediate tree data structures, without changing the algorithm.

Some issues were solved, for example with displaying the result of 0.3x0.3, and with expressions like -(2^32)+1

Feel free to cherry-pick the commit: rev22@d1bedeb (updated)

It's based on NeroBurner's rebase above.

The binary size of the calculator changed from 13164 to 8628 bytes in my test builds with gcc-arm-11.2-2022.02 and -O2 -Os. I suppose a large part of it is the lv_btnmatrix component

@ItzSwirlz
Copy link

Any status on this besides a potential 1.10 rebase if needed? I'd love to see this merged

@MaWalla
Copy link

MaWalla commented Sep 18, 2022

I merged the rebase from @NeroBurner into the current develop state and resolved all conflicts. Hope I did it right! 😄

https://github.com/MaWalla/InfiniTime/tree/Calc_rebase

@minacode minacode mentioned this pull request Dec 8, 2022
18 tasks
@omginput
Copy link

Could someone please tell why this hasn't been merged yet?

@JustScott
Copy link
Contributor

I updated the code to work with v1.14, its available on my branch: https://github.com/JustScott/InfiniTime/tree/calculator_app.

While it's working in InfiniSim, I get the following error attempting to build the mcuboot-app directly:

/Pinetime/gcc-arm-11.2-2022.02-x86_64-arm-none-eabi/bin/../lib/gcc/arm-none-eabi/11.2.1/../../../../arm-none-eabi/bin/ld: region RAM overflowed with stack Memory region

I'm assuming this means the Calculator.cpp code needs some memory optimizations before the app can run on the Pinetime's hardware...which is a bit above my skill level at the moment, so if anyone would like to try this out feel free to use my branch!

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

Successfully merging this pull request may close these issues.

Calculator App