WP 34S and 31S bugs and fixes

[Bit]

This thread can be used to discuss those bugs found in, and fixes proposed for, the WP 34S and 31S that don't warrant having their own dedicated thread.

The first ones:

1. When tracing is enabled (flag T) on the 34S emulator or the infrared-enabled build, the backspace operation is turned into CLx because print_trace() calls process_cmdline(). This means you cannot delete individual digits you just typed.

2. On the 34S if you attempt to perform an operation with a partially entered fraction in the command line, you get an error message as expected, but the same error is set again when xeq() tries to recover and calls process_cmdline_set_lift() in the "if (Error != ERR_NONE)" code branch.

Here's how to reproduce the problem:
[1] [.] [2] [.] [+]
You get an error as expected, but if you clear the error and continue with another operation:
[EXIT] [+]
Then you get another bogus error.

I've committed two patches to SVN but I'd prefer if someone else reviewed them, too.

[Marcus von Cube]

Bit,

thanks for fixing these. The patches look fine to me. I've just committed a new build to SF.

[pascal_meheut]

The Qt emulators have been rebuilt and uploaded. I'll update the iOS version later.

[Bit]

Thank you, Marcus.

Another issue: The documentation says that the algorithm that PRIME? uses is believed to work up to 9e18, which is roughly 2⁶³, the limit above which the isPrime() function fails with a domain error.

However, because of integer overflows, this is what actually happens (in double precision mode and only considering the integer part of a number):
- Negative numbers are reported as not primes. But odd negative numbers whose absolute value has bit 63 set also produce a domain error. Compare -2⁶⁴+1 and -2⁶⁴-1.
- Between 0 and 2⁶³: The algorithm in isPrime() is applied, errors are not produced.
- Even numbers above 2⁶³ with bit 63 set: Reported as not primes, see e.g. 2⁶³+2.
- Odd numbers above 2⁶³ with bit 63 set: A domain error is produced and PRIME? returns true, try 2⁶³+1.
- Above 2⁶³ if bit 63 is not set: PRIME? returns the result of the input modulo 2⁶⁴, so for example the even number 2⁶⁴+2 is reported as a prime.

I've committed a patch that results in the following more consistent behavior:
- All negative numbers are reported as not primes and errors are never produced.
- The original algorithm in isPrime() is applied to numbers between 0 and 2⁶³-1 and errors are never produced.
- Numbers greater than or equal to 2⁶³ always produce an error and PRIME? returns true.
(The handling of the two infinities and NaNs didn't change: They're always reported as not primes and no error is produced.)

I think it makes sense to treat the same way all numbers for which the algorithm isn't supposed to work, i.e. produce an error rather than have a special case only for numbers divisible by 2 but not for those divisible by 3, 5 and so on. I'm not sure if PRIME? should return true or false after an error, so I left that unchanged (it returns true).

The documentation states that PRIME? uses the absolute value of numbers but the original code explicitly (it didn't look like a typo) tested the sign to exclude negative numbers. I didn't change that and I suggest we keep this behavior because it makes sense and just update the documentation. But it'd be easy to use absolute values if that's what's desired. Please comment.

[Marcus von Cube]

I've rebuilt the while thing again.

[pascal_meheut]

So did I.

[Bit]

The 31S doesn't appear to make use of the quartz crystal (there are no stopwatch or set time functions) but it supports the ON+C key combination, and then it hangs and needs a reset if a crystal isn't installed. I've added a compile time option to disable the crystal completely, and enabled it by default on the 31S.

Someone please let me know in case the 31S does need the crystal in some way I didn't notice.

[Marcus von Cube]

(11-24-2014 02:21 AM)Bit Wrote:  Someone please let me know in case the 31S does need the crystal in some way I didn't notice.

Leaving the crystal out on the 31S is safe.

For the 34S, I wanted to avoid having another set of flash images to maintain, one which lacks crystal support completely. That's why ON+C+C is present. You may save a few bytes with the crystal-less image but I doubt it's worth the hassle.

[Bit]

(11-24-2014 04:43 PM)Marcus von Cube Wrote:  For the 34S, I wanted to avoid having another set of flash images to maintain, one which lacks crystal support completely. That's why ON+C+C is present. You may save a few bytes with the crystal-less image but I doubt it's worth the hassle.

Marcus, you gave me an idea: I've added a DISABLE_XTAL build option to the Makefile of the 34S to make it easier to produce such a build in case someone would like to. It creates a new binary named calc_noxtal. It saves about 800 bytes compared to the plain build, which could be worth it in some cases.

[Bit]

I've found another issue related to prime numbers. NEXTP increments a number in a loop but if the value is large enough, there aren't enough digits in the mantissa for the number to actually increase. For X ≥ 2⁶³, the recently fixed PRIME? command produces an error. But in single precision mode if 9999999999999937 ≤ X < 2⁶³, the XROM code gets stuck in an infinite loop and the calculator has to be reset.

I've updated NEXTP, and now it gives a domain error instead of hanging in such cases. If it's invoked in single precision mode (or on the 31S), it fails if the input is greater than or equal to 9999999999999937 (the largest prime that can be accurately represented in single precision mode). Without that, it'd produce incorrectly rounded values when returning from double precision mode via xOUT. For example, NEXTP 10¹⁷ would return 10¹⁷, which is neither the next number, nor a prime. The new version uses xIN that helps properly recover from errors reported by PRIME? and handle truncating in integer mode. The only library function that invokes NEXTP is PF, which doesn't use xIN, so there are no conflicts due to nesting.

This issue highlighted another problem: XROM code couldn't be interrupted. Other similar bugs in XROM may exist or could be introduced later, so it'd be useful if there was a more graceful way of dealing with them than a hardware reset. Therefore I created a feature to interrupt XROM code safely by holding down the EXIT/ON key for a while. A variable (OnKeyTicks) in volatile RAM indicates how long the EXIT key has been held down and xeq() generates an error ("Interrupted") above a certain threshold. It's a compile time option and I recommend that it be enabled in all builds, but I left it disabled by default until it's approved by one of the original developers.

Pascal, I've added support for this feature in the Qt versions. They seem to work on Linux but please have a look.

To make testing easier for those who can't compile binaries, I've attached 34S and 31S builds that interrupt XROM code if the EXIT key is held down for 1 second, and they include the old NEXTP implementation that locks up if X = 10¹⁷ in single precision mode. (The binaries were compiled from vanilla r3700 sources without any of my other patches.)

[Paul Dale]

(11-26-2014 05:24 AM)Bit Wrote:  This issue highlighted another problem: XROM code couldn't be interrupted.

The current behaviour is intentional. XROM code that doesn't stop is a bug which will be fixed. That doesn't mean we shouldn't discuss this option...


Thanks for the other bug fixes,

Pauli

[pascal_meheut]

(11-26-2014 05:24 AM)Bit Wrote:  Pascal, I've added support for this feature in the Qt versions. They seem to work on Linux but please have a look.

I had a quick look and it seems ok. Except for the use of _ in camelcase naming (increment_OnKeyTicks and update_OnKeyTicks) but this is minor.

[Bit]

(11-26-2014 05:54 AM)Paul Dale Wrote:  The current behaviour is intentional. XROM code that doesn't stop is a bug which will be fixed. That doesn't mean we shouldn't discuss this option...

Pauli, here's where I come from: I'm planning to give some 31S calculators to a few scientists among my acquaintances. If the device ever locks up and you need a pin to reset it, it'll look like it's broken and they'll be annoyed. On the other hand, if some operation seems to take forever but you can easily interrupt it with the EXIT key, that's a minor issue in comparison.

It may make little difference to you and I who understand the nature of occasional bugs of this sort but the user experience for most people is significantly better if error recovery is smooth. The cost in firmware size and code complexity is minimal because the standard error handling code does most of the job.

[Bit]

(11-26-2014 07:02 AM)pascal_meheut Wrote:  I had a quick look and it seems ok. Except for the use of _ in camelcase naming (increment_OnKeyTicks and update_OnKeyTicks) but this is minor.

Thanks for looking into it. I'll update those names if you tell me what you'd like to call increment_OnKeyTicks_adapter() considering that the other adapter functions use lower case.

[pascal_meheut]

The convention I used is to follow the same for C code: _ to separate the words, lowercase.
For C++ methods, I used a common convention in object-oriented code: no _, first character in lower case, each word starting with an uppercase character.
Because increment_OnKeyTicks is a C++ method of QtKeyboard, it should be incrementOnKeyTicks (and update_OnKeyTicks should be updateOnKeyTicks).

I know I'm a bit anal about it but it helps code reading.

[Bit]

(11-26-2014 07:32 PM)pascal_meheut Wrote:  The convention I used is to follow the same for C code: _ to separate the words, lowercase.
For C++ methods, I used a common convention in object-oriented code: no _, first character in lower case, each word starting with an uppercase character.
Because increment_OnKeyTicks is a C++ method of QtKeyboard, it should be incrementOnKeyTicks (and update_OnKeyTicks should be updateOnKeyTicks).

I know I'm a bit anal about it but it helps code reading.

Consistency is naming is very useful, no need to apologize.

The mentioned two names are clear but what about the adapter function? It's in C++ code but declared extern "C" and on top of that its name refers to a C variable in camelcase (following the convention already used for some similar variables in C code). Would you like to call it increment_on_key_ticks_adapter or something else?

[Paul Dale]

(11-26-2014 07:32 PM)pascal_meheut Wrote:  I know I'm a bit anal about it but it helps code reading.

I suspect I'm the most guilty for inconsistently naming things through the code base


- Pauli

[pascal_meheut]

(11-26-2014 07:56 PM)Bit Wrote:  The mentioned two names are clear but what about the adapter function? It's in C++ code but declared extern "C" and on top of that its name refers to a C variable in camelcase (following the convention already used for some similar variables in C code). Would you like to call it increment_on_key_ticks_adapter or something else?

As it is a C function, I would named it using the _ convention, so increment_on_key_ticks_adapter indeed.
In the same file, you have is_key_pressed_adapter or get_region_path_adapter for instance.

(11-26-2014 10:01 PM)Paul Dale Wrote:  I suspect I'm the most guilty for inconsistently naming things through the code base

I did not notice and found the project to be well-written and the code easy to understand when I started working on it.
I've seen much worse.

[Paul Dale]

(11-26-2014 10:07 PM)pascal_meheut Wrote:  I did not notice and found the project to be well-written and the code easy to understand when I started working on it.

Thanks. I suspect my name choices varied a bit depending on what bit of open source software I was tasked with at my day job. I also jumped between camel case and underscores a bit -- e.g. most of the mathematical functions are decNumberXXX or cmplxXXX whereas a lot of the rest of the code is underscored. The decNumber naming follows that library, although we did wrapper a lot of the common functions using dn_xxx to save some bytes.

Quote:I've seen much worse.

I think we've all see worse and I've been guilty of writing worse too


- Pauli

[Bit]

Pascal, I've changed the function names as you requested.

The new NEXTP implementation still got stuck in a loop for positive infinity in double precision mode, I've fixed that, too.