InfiniTime icon indicating copy to clipboard operation
InfiniTime copied to clipboard

Calculator App

Open Raupinger opened this issue 3 years ago • 48 comments

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:

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

this resolves #227

Raupinger avatar May 20 '21 15:05 Raupinger

Video

https://user-images.githubusercontent.com/29229062/119008299-56f20f00-b992-11eb-973b-1702db2fb84c.mp4

Raupinger avatar May 20 '21 15:05 Raupinger

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 avatar May 23 '21 02:05 Raupinger

video

Raupinger avatar May 23 '21 02:05 Raupinger

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!

Dudemanjude avatar Jun 17 '21 02:06 Dudemanjude

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

Raupinger avatar Jun 17 '21 03:06 Raupinger

Thanks @jonvmey for all those reviews

Raupinger avatar Jun 18 '21 12:06 Raupinger

Is this just waiting to be merged or?

Izaic avatar Aug 29 '21 01:08 Izaic

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

Raupinger avatar Aug 29 '21 10:08 Raupinger

Any updates on this?

KoalaV2 avatar Nov 12 '21 15:11 KoalaV2

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.

Raupinger avatar Nov 12 '21 15:11 Raupinger

i would really like that feature on InfiniTime

fossdd avatar Nov 21 '21 18:11 fossdd

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?

Alpha-Titiwu avatar Dec 05 '21 11:12 Alpha-Titiwu

This would be a handy feature indeed. 🙂

Vistaus avatar Dec 07 '21 16:12 Vistaus

Ok. I've rebased it all and fixed another bug found in testing. For now I have put the Calculator at the Meter apps spot in the app list and moved Meter to the settings list. It would be great if some of you could give this branch a try and tell me about any bugs i missed. If you don't have a devkit and are willing to take the relatively minimal risk you can download the dfu file from githubs automated build and flash it OTA.

Raupinger avatar Dec 09 '21 11:12 Raupinger

Inputting a large number and pressing equals returns -2147483648. Inputting a large number that wraps on two lines has some weird behaviour at first where it takes at least three digits on the next line. This is because of an LVGL config. I don't know if there's an easy way to go around this, because this value probably shouldn't be changed. https://github.com/InfiniTimeOrg/InfiniTime/blob/85a25302bfac215f9ec7b993f6a2c21a20ee223b/src/libs/lv_conf.h#L506 You can make the button matrix slightly larger by reducing the outer padding to zero like was done in the Tile screen. The inner padding can adjust the button density. https://github.com/InfiniTimeOrg/InfiniTime/blob/85a25302bfac215f9ec7b993f6a2c21a20ee223b/src/displayapp/screens/Tile.cpp#L94

lv_obj_set_style_local_pad_all(btnm1, LV_BTNMATRIX_PART_BG, LV_STATE_DEFAULT, 0);
lv_obj_set_style_local_pad_inner(btnm1, LV_BTNMATRIX_PART_BG, LV_STATE_DEFAULT, 10);

Riksu9000 avatar Dec 09 '21 11:12 Riksu9000

I added a file to the PR that explains how to use the app.

Raupinger avatar Dec 09 '21 13:12 Raupinger

@Riksu9000

  1. I'm aware of the overflow problem but I currently don't intend to do anything about it. People are rarely gonna work with values in that range on their wrist and I felt like checking for it would be a lot of work. That said i just looked it up and its pretty easy for addition, substraction and multiplication while pow() should report range errors. So this could and should probably be fixed
  2. If anyone knows an easy fix for that, let me know, otherwise I don't think its that big a deal.
  3. Good idea. Ill add that.

thx

Raupinger avatar Dec 09 '21 13:12 Raupinger

That said i just looked it up and its pretty easy for addition, substraction and multiplication while pow() should report range errors. So this could and should probably be fixed

I plan on disabling those errors. There's probably some less ideal but equally okay solution there.

Avamander avatar Dec 09 '21 13:12 Avamander

Thanks, that single operator thing should be an easy fix. The overflow thing is a bit more work but I agree that it's worth the effort. My original plan for the plus signs spot was a sqrt but adding unary operators would have complicated things I left it a t that. Pi is a good idea. I'll clean up the code and fix the warnings.

Raupinger avatar Dec 10 '21 13:12 Raupinger

Ok, i've fixed the crash and added over/underflow checks. The code for the lather is neither particularly pretty or fast, but it works.

Raupinger avatar Dec 10 '21 15:12 Raupinger

I'd propose to introduce % (remainder) operator, with button on the position of + on the second buttons set. It is often implemented in physical calculators. Also it seems to be the right thing to introduce sin, cos, tan, arcsin, arccos, arctan, log, ln, exp, 1/x, n!, sqr, sqrt (maybe on a third buttons set) - these functions look cheap to implement, but turn the calculator into a scientific one, that may be used in more circumstances. Still, of course, the calculator would be handy anyway.

medeyko avatar Dec 14 '21 22:12 medeyko

I'd propose to introduce % (remainder) operator, with button on the position of + on the second buttons set. It is often implemented in physical calculators. Also it seems to be the right thing to introduce sin, cos, tan, arcsin, arccos, arctan, log, ln, exp, 1/x, n!, sqr, sqrt (maybe on a third buttons set) - these functions look cheap to implement, but turn the calculator into a scientific one, that may be used in more circumstances. Still, of course, the calculator would be handy anyway.

That would be way beyond the scope of this calculator, it's just supposed to be a simple calc for doing quick math on your wrist in a pinch. I also personally would prefer not to have too many functions, as the calculator app for BangleJS added all of those features that you've mentioned, and it means that it's really hard to accurately hit buttons on the calculator as a result because they are now so tiny.

Izaic avatar Dec 14 '21 23:12 Izaic

That would be way beyond the scope of this calculator, it's just supposed to be a simple calc for doing quick math on your wrist in a pinch.

I personally time to time calculate sqrt or sine exactly as a "quick math". Of course, I have a smart phone, but still, using a watch may be more covenient. Considering that the feature is cheap to implement, I don't see, why not.

the calculator app for BangleJS added all of those features that you've mentioned, and it means that it's really hard to accurately hit buttons on the calculator as a result because they are now so tiny.

I don't mean to make the buttons any smaller, there's no any need to shrink them. I listed 12 functions - it's 4 by 3 rectangle. So they may be introduced on a secondary buttons screen, activated by swipe (either replacing numbers on current secondary buttons screen or making second secondary screen activated by the opposite swipe), exactly in the already existing dimensions.

medeyko avatar Dec 15 '21 11:12 medeyko

I personally time to time calculate sqrt or sine exactly as a "quick math". Of course, I have a smart phone, but still, using a watch may be more covenient. Considering that the feature is cheap to implement, I don't see, why not.

it may be not cheap to implement @medeyko since it may need to include a math library for the other function , and since infintime is tight in space , it would make the pr unmergable because of the reaminder free fash availlable : math lib + this pr + firmware > firmware max flash size => not merged this pr (no cos sin...) + firmware< firmware max flash size => can be merged

it's better the keep it simple first , in order to make the merge easier for @JF002 .

trman avatar Dec 15 '21 21:12 trman

I absolutely agree that thing that make the merge difficult should be avoided at this stage!

However, I still disagree that my proposal falls under this category: math.h/cmath ALREADY #included in the calculator app. It also contains sine/cosine support for Cortex M4F.

As for the pow function, it is already used in the calculator app. Moreover, its documentation suggests to use x^(1/2) for calculation of a square root.

Factorial is a quite simple function. So no any additional math lib is required...

Therefore I still think that my proposal is cheap to implement. But ok, I would be happy to have the calculator app even in its current state.

medeyko avatar Dec 16 '21 18:12 medeyko

math.h/cmath ALREADY #included in the calculator app. It also contains sine/cosine support for Cortex M4F.

Compiler usually optimizes out functions that aren't used.

Moreover, its documentation suggests to use x^(1/2) for calculation of a square root.

This suggestion should not be followed before one verifies it's actually better.

In the end, the scope of the PR should be kept as-is, further feature requests should be placed in an appropriate issue.

Avamander avatar Dec 16 '21 19:12 Avamander

For reference #476 was done in order to remove usage of cmath's sine/cosine functions as that saved ~7kB of binary size. Accurate trig functions (ie. beyond the LUT now used for the analog watchface) are relatively expensive size-wise.

jonvmey avatar Dec 16 '21 19:12 jonvmey

  • [ ] Swiping left and right might also input characters. I'll look into this more as well.

The same sometimes happens on the app select screen. There's probably some improvement to Infinitimes gesture recognition to be made in general here

  • [ ] We could change the "+" to something else on swipe too. Maybe pi?

I like the idea of adding a modulo operator

  • [x] Maybe the results should be on a separate line, so it doesn't erase the input. I think it's unlikely that people perform calculations that don't fit on a single line on a watch.

That would have the added benefit of being able to display error messages instead of just vibrating. What would entering a new calculation look like with that?

Raupinger avatar Dec 16 '21 19:12 Raupinger

  • [x] I think it's very likely that many people will encounter the overflow problem by just playing around with it, so I think we should do something about it.

That fix involved a fair bit of trial and error. Id to see someone take a shot at breaking it again. The last overflow check when converting the float to an inte has the potential to hide cases where an overflow happens during calculation and isn't caught by the checks there. A calculator that avoids overflows 99.5% of the time is worse than one that doesn't do it at all, so I want to make sure we get this right.

Raupinger avatar Dec 16 '21 19:12 Raupinger

@medeyko btw. the compiler optimizing away unused library function is also the reason for the somewhat awkward float to string conversion seen in the calculator. Not using the library functions for that saves 6k in the binary.

Raupinger avatar Dec 16 '21 20:12 Raupinger