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.

Sign up for GitHub

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

Jump to bottom

Simple calculator #1483

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

Simple calculator #1483

wants to merge 52 commits into from

Conversation

minacode
Copy link
Contributor

@minacode minacode commented Dec 8, 2022 •
edited
Loading

This PR implements a simple fixed-point calculator. In contrast to #375 it aims to be simpler by not parsing whole equations but just evaluating one operation at a time over an accumulated result.

It takes an operation and a value (digit-wise) and applies the value to the result with the chosen operation whenever a new operation is chosen or = is pressed.

=: evaluate current input
<: removes the input charwise or reset the result if the input is zero.
(-): toggle the sign of the current input (thanks @FintasticMan)
+ - / * /: press the button to cycle through the operators and "no-operator".

It uses fixed-point numbers and checks for errors like too big results and zero division.

Known issues:

from @yusufmte's comment below:

  • Bug with crashing on large operations (thanks to @CCF100 too!)

  • Bug when typing a number larger than the interface allows

  • Bug with any use of the decimal point

  • Bug where if the result exceeds 12 digits, you will only see the last 12.

  • Bug when pressing ^, sometimes it will repeat whichever number you last placed

  • UI gripe / Suggestion (not a bug); backspace and extra clear button

  • = should repeat the last operation (thanks @Avamander!)

  • Check power overflow.

  • Show error messages.

  • Add power button toggle.

  • Fix button colors.

  • Set one check feature.

  • -3 * -5 results in "too large"

  • -6 repeatedly divided by -2 does not swap the sign right

  • number input is broken when value is negative

  • operator should only be removed if value is zero

  • 987654321*100 does not trigger a "too large" warning

  • implement the interface for optional apps

@joseph58tech
Copy link

I feel like the original design from #375 was simpler and cleaner, the multifunctional +/- and */ buttons might be too much of a hassle for regular use.

@FintasticMan
Copy link
Member

FintasticMan commented Dec 8, 2022 •
edited
Loading

About the printing of the value, I think that doing something like what's done here should be fine:

InfiniTime/src/displayapp/screens/BatteryInfo.cpp

Line 38 in 361e381

lv_label_set_text_fmt(voltage, "%1i.%02i volts", batteryVoltage / 1000, batteryVoltage % 1000 / 10);

Edit: looking at the code, seems like you're already using something like this.

I would say that it would be better to use fixed width integers instead of long ints, so that it's obvious what the min and max values are.

@minacode
Copy link
Contributor Author

minacode commented Dec 9, 2022

@joseph58tech, you mean the design with two keyboard layers?

@FintasticMan, I can change long int to int64_t, but the format string will still contain %ld which means "long decimal".

@FintasticMan
Copy link
Member

FintasticMan commented Dec 9, 2022 •
edited
Loading

You can either leave the format string as "%ld", or you can use fixed width format literals. This would mean that the format string would become "%" PRId64. To use those you need to include the <cinttypes> header. It's worth mentioning that it seems like in this codebase printing fixed width integers is usually just done with normal format specifiers.

@yusufmte
Copy link
Contributor

yusufmte commented Dec 11, 2022 •
edited
Loading

This looks amazing, thanks for making it! The UI looks really nicely designed with a good balance of button size and ability to do most of the major operations you would need. Personally I prefer the simplicity of this app though I see advantages in both. I also love the inclusion of ^ and using the same button for +- and */ to save space.

I would love to one of the calculator apps merged in an upcoming release and I'll probably be using it on my PT until then. It feels like the biggest thing currently "missing" from the PineTime to me. Along with the usefulness of having a calculator close by, a small part of that is probably from reminiscing about these calculator watches from childhood, haha.

image

@minacode
Copy link
Contributor Author

Thank you! 😊 Nice to hear that you want to try it already. This may help with finding any eventual bugs. I haven't tried the last two commits on my watch because I have another test build running right now.

@FintasticMan
Copy link
Member

Looks good, I've tried this on my watch and it works quite nicely. I do think that it is better to use floats rather than doubles for the pow calculation. Why did you switch to doubles?

@Ceimour
Copy link
Contributor

Ceimour commented Dec 12, 2022 via email

@minacode
Copy link
Contributor Author

I thought that double matches the 64-bit integers better than float. Is that correct? 🤔
The workaround is necessary because I wanted to have a power function and could not find one the works with the fixed point numbers. Speed should be no concern for one calculation.

@Ceimour
Copy link
Contributor

Ceimour commented Dec 12, 2022 via email

@Ceimour
Copy link
Contributor

Ceimour commented Dec 12, 2022 via email

@minacode
Copy link
Contributor Author

Ok, I will test both types on the watch. Thank you for the input!

@Ceimour
Copy link
Contributor

Ceimour commented Dec 13, 2022 via email

@Ceimour
Copy link
Contributor

Ceimour commented Dec 13, 2022 via email

@FintasticMan
Copy link
Member

I thought that double matches the 64-bit integers better than float. Is that correct? 🤔

While doubles are 64 bits wide, that doesn't make a difference for int > float and float > int conversion. 32-bit floats will definitely have enough precision for this calculation, and as Ceimour already said, the FPU in the nRF 52832 is for 32-bit floats only, so double calculations will be done much less efficiently.

@minacode
Copy link
Contributor Author

This says that there are 9 digits for float and 17 for double. 9 is not enough. That is only 6 integer digits.

@CCF100
Copy link

CCF100 commented Dec 19, 2022

Hey @minacode, if the variable used to store the calculated value overflows, the watch reboots...

@minacode
Copy link
Contributor Author

minacode commented Dec 19, 2022 •
edited
Loading

Thank you for testing this! It doesn't happen in the simulator so I wonder what makes the watch crash.
Does it happen with every operation or only with the power function?

Edit: I tested the last changes on the watch and it seems like a lot broke. Maybe we have to revert some of the changes.

@yusufmte
Copy link
Contributor

yusufmte commented Dec 19, 2022 •
edited
Loading

@minacode : I've been using the calculator for a little while now. Overall for most calculations it's working great, doing just what it's supposed to, and has been very useful-- gonna keep using it on my build and I would love to see it included in an upcoming milestone!

Basically it has worked flawlessly for me with any small integer operations (small meaning anything not huge, like less than 10^12). Deviating from small integers, there are a few bugs to iron out:

  1. Bug with crashing on large operations. As @CCF100 mentioned-- try feeding the calculator a very large operation (eg large number ^ large number), and InfiniTime will crash and reboot. I do agree this is probably because of overflow, as this consistently this happens with 2^54 but not with 2^53. It is not just the power function -- the same crash happens when reaching similar magnitudes by repeatedly doing 10*10. Suggested behavior: when an operation is about to cause overflow, the result label should just say "TOO LARGE." This would be fine, as in daily use most people wouldn't need to conduct operations that large.

  2. Bug when typing a number larger than the interface allows. Try repeatedly tapping "9" (or any digit)-- eventually the input label will start cycling between a jumble of random numbers, and as you keep tapping it, eventually it will show "-1". Not sure exactly what causes this. Suggested behavior: When the current number in the input field fills the field (looks like that happens as 12 digits), further digits input should be ignored, and some feedback should be sent to the user to indicate that this happened (eg short vibration as if to say "Nope, no more digits allowed.").

  3. Bug with any use of the decimal point. Try using the . (for example, type 5.1) and the number shown in the input field would be different (for 5.1, you would see 5.15663343). Not sure exactly what is causing this, but it has been consistent for me. However, the number actually stored in the input field is what you typed (5.1). For example, try 49^0.5 -- the result will, appropriately, be 7, even though the operation will look like you are doing 49^0.15663343.

  4. Bug where if the result exceeds 12 digits, you will only see the last 12. For example, if the result is 1234567890123, you will only see 234567890123. Suggested behavior: Either (a) the result label just says "TOO LARGE" if the result exceeds 12 digits, or (b) use scientific notation with E to denote numbers larger than 12 digits (for example, the result above would display as 1.2345678E12). I think solution (a) would be just fine, but solution (b) would ultimately be ideal.

  5. Bug when pressing ^, sometimes it will repeat whichever number you last placed. This one is tricky as it is not consistent, but when it starts happening (and I'm not quite sure what triggers it to start happening), it continues to happen consistently until the PineTime reboots. When pressing ^, instead of changing the operation to ^, it will repeat the digit you just typed. For example, if you tap 2 then ^, you will see 22 in the input field and no operation. If you keep tapping ^, it will keep repeating the same digit, then eventually clear the input field after a few taps.

  6. UI gripe / Suggestion (not a bug). This is somewhat subjective of a design decision and it might just be me, but when I see the < I sort of expect it to work as a BACKSPACE rather than a CLEAR. Along similar lines, I feel like a BACKSPACE would be quite handy for the sausage-fingered among us, as the buttons are quite small and it's easy to mistype :) Suggestion: I would keep the < button in it's place and change it to a BACKSPACE function, removing the last digit placed in the input field rather than clearing the whole thing. Then, I would use the space in the top-right corner (above the operation label) for a CLEAR button, and I would use the symbol C or even something like an eraser symbol from the FontAwesome library.

I hope this is helpful-- FYI these are all on PineTime hardware, I haven't tested any of these in the sim! If I have time in the upcoming days, I might poke around the code and see if I can help troubleshoot the causes for some of these.

@minacode
Copy link
Contributor Author

Thank you so much for this detailed feedback! You even gave examples to reproduce. This is very helpful.
I added your points to the description as "known issues" and will investigate them.

There seems to be a difference in behaviour between the simulator (x86) and the watch. I am currently diving in the details to get everything a little more safer. The format strings are way less forgiving on the watch it seems.

At first I ignored the issues around overflows but I get that the UX would be heavily improved by a controlled maximum value.

This is somewhat subjective of a design decision and it might just be me, but when I see the < I sort of expect it to work as a BACKSPACE rather than a CLEAR

This is a good idea. It would also be a lot more forgiving since it would not delete the result instantly. I don't know about a clear button and will think about it. Nulling the result could technically also be done via the = button.

@Avamander
Copy link
Collaborator

Nulling the result could technically also be done via the = button.

That's usually "repeat the last operation"

@minacode
Copy link
Contributor Author

Then this shall be it.

@yusufmte
Copy link
Contributor

yusufmte commented Dec 20, 2022 •
edited
Loading

Glad it is helpful-- thanks for your diligence!

At first I ignored the issues around overflows but I get that the UX would be heavily improved by a controlled maximum value.

I was thinking the same thing at first: probably very few users would need to perform calculations at the level needed to cause overflow. It's more the kind of bug that comes out of the user looking for ways to break the app. But out of principle it is best to foolproof it such that the user is incapable of causing a crash with any combination of inputs, and to make it clear when the user passes into too-large territory where the calculations or interface start to break down. 🙂

That's usually "repeat the last operation"

Agree! That would be a more expected behavior for =.

PS: This might be a good BACKSPACE symbol if you are interested! (But < works fine as well!)

@yusufmte
Copy link
Contributor

  1. Bug with any use of the decimal point. Try using the . (for example, type 5.1) and the number shown in the input field would be different (for 5.1, you would see 5.15663343). Not sure exactly what is causing this, but it has been consistent for me. However, the number actually stored in the input field is what you typed (5.1). For example, try 49^0.5 -- the result will, appropriately, be 7, even though the operation will look like you are doing 49^0.15663343.

Just wanted to add briefly -- In case it is helpful with troubleshooting this one: this bug also happens with division of non-divisible integers. For example, 52/53 and 1/2 both evaluate to 0.48957.

@minacode
Copy link
Contributor Author

minacode commented Dec 20, 2022 •
edited
Loading

Ok, so I changed some things in the last commits. It's not done yet, but we are getting there.

  • backspace is now beautiful 😊 and deletes in this order
    1. the operation (if it exists)
    2. a digit from the input value (if it exists)
    3. the result
  • the values are bounded and overflow is checked (but not reported yet). I also updated the number of decimal digits to 4 for the time being as 3 let some bugs slip through. I may go back to 3 because there are only 6 integer digits left now

All changes are only tested in the simulator for now.

And I have a question: How exactly do you expect the = button to behave?

Currently it calculates a new result and keeps the value and operation untouched. Therefore you can press it multiple times for repeated calculation. But this causes two issues:

  1. pressing any operation button afterwards will evaluate the calculation again (as it normally does as well). I think this would be unexpected.
  2. number input will extend the number that is already there.

One could use the backspace but it seems very inconvenient to me.
Another solution could be an internal "= was pressed"-flag that causes an immediate reset of the operation and number once anything other than = gets pressed.

@minacode
Copy link
Contributor Author

The last commit is an experiment for the UX. Instead of standard button presses you can now hold the touch and drag it accross the button matrix. The current button is colored with a different background. Releasing triggers the button handle. I hope that this help with pressing the correct buttons on the small screen.

If anyone has a good suggestion for the focus color I will add it. I am not good with colors 😀

@yusufmte
Copy link
Contributor

yusufmte commented Dec 21, 2022 •
edited
Loading

Awesome, thanks for your work on this-- love the changes!! Tested new build on PineTime hardware and can confirm that:

  1. Overflow no longer causes a crash-- any overflow seemingly evaluates to 0.0001. Remaining suggestion: Probably best if it's eventually reported as OVERFLOW or TOO LARGE, for accuracy, but I think this is great.

  2. Values bounding is working well and it is not possible to type a number larger than the interface allows. Remaining suggestion: It feels a bit odd that you can only type 6 digits before the decimal point, and 4 digits after. With room for 11 characters (including the decimal point), it feels like you should be able to type either n.nnnnnnnnn or nnnnnnnnn.n , for example. My suggestion would be to check for the total number of characters (digits before decimal point, decimal point itself, and digits after), and stop accepting inputs once the total is 11.

  3. Calculations with non-integer numbers have been working perfectly, and there is no more discrepancy between the number typed and the number in the register! Remaining suggestion: Similar to the suggestion in (2) above, it would be nice if the number of digits after the decimal point in the result extended to the end of the result area. For example, 1/9 currently evaluates to 0.1111. With room for 11 characters, I feel it should show 0.111111111. In other words, the behavior would be: (a) show all digits to the left of the . -- if there are more than 11, instead report the number as too large or use a scientific notation; (b) show as many digits as possible to the right of the ., based on how much room is left in the label. The reason for this is that it's clear to the user that the calculator will round when the label is full, but not necessarily if it gets cut off before that (so user might ask "are there only 4 digits here, or is it rounding?'). However feel free to disregard this one if it's overcomplicating things.

  4. The backspace is looking/working great! 🙂

And I have a question: How exactly do you expect the = button to behave?
Currently it calculates a new result and keeps the value and operation untouched. Therefore you can press it multiple times for repeated calculation. But this causes two issues:

  1. pressing any operation button afterwards will evaluate the calculation again (as it normally does as well). I think this would be unexpected.
  2. number input will extend the number that is already there.
    One could use the backspace but it seems very inconvenient to me.
    Another solution could be an internal "= was pressed"-flag that causes an immediate reset of the operation and number once anything other than = gets pressed.

The way = now is working is exactly as I would expect, with allowing multiple presses for repeated calculation!

Here's the way I would recommend addressing those two issues:

i. I agree this is confusing. I would make it so that pressing the operation buttons never causes a calculation-- only changes the current operation. The ONLY way to cause a calculation therefore would be pressing =, and so the user would never do it accidentally. If you did this, I would also modify the backspace so that it doesn't remove the operation (since you can change it freely), only: (a) a digit from the input value, then (b) the result. (I think that change to the backspace is an added bonus, because it's potentially confusing that the backspace would remove the operation first even though it may not be the last thing typed.)

ii. I like your solution-- when = is pressed and a calculation happens, if you type any digit after that, it should reset the current input label and place the digit freshly. (This also makes clearing more convenient.) However, I don't think the operation needs to be reset (only the number), especially if the tweak in (i) is made where the operation can be changed anytime.

  1. Unrelated: division by zero does nothing (tapping the = doesn't register). I think this is fine to stay that way, but if you eventually had reported flags like TOO LARGE or OVERFLOW, it would probably make sense to report this as well as something like UNDEFINED, for accuracy.

The last commit is an experiment for the UX. Instead of standard button presses you can now hold the touch and drag it accross the button matrix. The current button is colored with a different background. Releasing triggers the button handle. I hope that this help with pressing the correct buttons on the small screen.

This sounds like a great idea, excited to try it out!

(Of course, feel free to take or leave any of the suggestions above at your discretion! Just my thoughts but others might think differently and it is your app 🙂)

@minacode
Copy link
Contributor Author

Again thank you for the feedback! It means a lot 😊

  1. There needs to be an overflow check for the power function. I haven't done this yet.
  2. (and 3.) The current implementation builds on fixed point numbers. You basically suggest changing to floats. Maybe that is a thing, I don't know. I didn't use floats based on advice from the chat in the early development and will keep it like that for now. But I will change back to 3 decimals digits and 7 integer digits, because I guess that is the most practical format. 4 is just better for debugging.
  3. Nice! I like it as well 😌

@minacode
Copy link
Contributor Author

I did some updates:

  1. Power overflow is done.
  2. Error messages are done for "too large" and "zero division".
  3. The operation label is gone and instead the buttons are now colored. The numbers can be a lot larger now. I hope that this doesn't open any new bugs 😀

A gif probably says the most:
InfiniSim_2022-12-31_131450

There are some minor issues left that I will continue working on.

@yusufmte
Copy link
Contributor

yusufmte commented Jan 1, 2023 •
edited
Loading

Awesome updates! Thanks for your work 👍 Testing latest build on PineTime hardware and all the bugs number 1-7 reported above are resolved from my end ✔️ I love the design changes like the touch/drag coloring, and coloring the currently active operation! Very clever & sleek.

(Also, regarding the fixed vs floating point, that makes sense 🙂 I agree, I've become familiar with it and think fixed point is probably favorable for simplicity of design.)

minacode and others added 14 commits July 16, 2024 14:57
fixed negative number input
041369b 
Instead of always adding the new digit it now gets multipled with a sign, based on whether value is negative or not. Thus, the addition becomes a subtraction for negative numbers.
changes
f116a3e
fix after rebase
19eb8a9
format
44d1d7d
fixed display of negative numbers smaller than 0
a3a557f
@minacode @Boteium
only remove the operator if the value is zero
2502200 
Co-authored-by: SuIông N. <[email protected]>
lower maximum number size and number of digits
d356c40 
This is necessary to ensure correct calculations. Themaximum int64 has 19 digits. Therefore, 18 digits are safe to use. But in order to calculate exact multiplication we need twice the number of digits after the comma, so I decided to go with 12 digits before the comman and 3 after it.
update to optional apps
e609983 
This commit updates the codebase to use the new interface for compile-time optional apps.
fix build and format warnings
10346ef
reduce heap size
b734baf
implement new CMake interface
ab7ceb9
set heap size to 39
0f65f48
add missing icon
d2c178e
remove TODO
accd666
@minacode
Copy link
Contributor Author

This comment in the other calculator implementation is interesting. Saving it here for later.

minacode and others added 2 commits August 31, 2024 00:43
@minacode
Merge branch 'main' into simple-calculator
46efadb
add const
0d54f1e 
I dont 100% understand, how it works, but this seems to reduce the amount of needed memory, such that we do not have to reduce the size in the FreeRTOS config.
@minacode
Copy link
Contributor Author

minacode commented Sep 1, 2024

Turns out that adding this const to the button matrix does indeed change something, because we no longer need to reduce the FreeRTOS heap size.

Runs well on my watch for a day now.

@mark9064
Copy link
Contributor

mark9064 commented Sep 1, 2024

Nice!
Is constexpr possible or more correct here? I think it's preferred way when it's possible but I'm honestly not entirely sure...

@minacode
Copy link
Contributor Author

minacode commented Sep 1, 2024 •
edited
Loading

I don't know, but I will try.
What exactly? The first const? Both? 😄

@mark9064
Copy link
Contributor

mark9064 commented Sep 1, 2024

I think constexpr const char* should suffice?
AFAIK this means compile time evaluate this constant list of const char* (i.e constant strings)

@minacode
Copy link
Contributor Author

minacode commented Sep 1, 2024

The last commit did not change something, so I guess that was all we will get.

@mark9064
Copy link
Contributor

mark9064 commented Sep 1, 2024

Oh they're equivalent, it's that constexpr is nicer to use in general as it is a guarantee of compile time evaluation (if it cannot be evaluated at compile time compilation fails). The way you had previously is the way to do it in C, with constexpr added to C++ constexpr const char* is the more modern way from what I understand

@minacode
Copy link
Contributor Author

minacode commented Sep 1, 2024

So we just leave it like this, because more const -> more good? 😀

@mark9064
Copy link
Contributor

mark9064 commented Sep 1, 2024

The const immediately after constexpr is redundant (constexpr implies const) so I'd remove that personally but it doesn't make a difference in terms of semantics

@mark9064 mark9064 mentioned this pull request Oct 12, 2024
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mark9064 mark9064 mark9064 left review comments

@FintasticMan FintasticMan FintasticMan left review comments

@Boteium Boteium Boteium requested changes

@Riksu9000 Riksu9000 Awaiting requested review from Riksu9000

Assignees
No one assigned
Labels
new app This thread is about a new app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.