Change clc to sec in math operations - #906
Conversation
Analysis done.
|
I can't really notice a behavior difference with this change. It only affects shuffle, and shuffle seems really busted both with and without this change. Can you say more about what your fix does and how you verified it? |
|
Well, this is like a "DivideBy11". |
|
FYI I just pushed a related fix 116e687 - the random number seed was... not random. Oops! Anyway, with that change the shuffle function seems to work, as opposed to always being stuck on the same number. |
|
Okay, from staring at the code for a bit, I think the original code at https://github.com/a2stuff/cdremote was indeed flawed.
I'll make some comments in the review. |
inexorabletash
left a comment
inexorabletash
left a comment
There was a problem hiding this comment.
Thanks for your patience. Since this was based on the original 40 year old code, I wanted to be really sure the fixes were valid, and understand why it had mostly worked before.
| @@ -2284,7 +2284,7 @@ PRNGMathLoop1: | |||
| PRNGMathLoop2: | |||
| cmp HexTrackCount0Base | |||
| bmi ExitMathLoop2 | |||
There was a problem hiding this comment.
Can you change this to bcc ? having this be signed doesn't help, we want an unsigned modulus.
| ldx #$00 | ||
| DivideBy10: | ||
| cmp #$0a | ||
| bmi TenOrLess |
There was a problem hiding this comment.
Can you change this to bcc ? having this be signed doesn't help, we want an unsigned modulus.
| ;;; ============================================================ | ||
|
|
||
| ;; TODO: Analysis - WTF is going on here?? This *seems* like an attempt to convert from BCD to binary, but it's... not. It's kinda wonky. | ||
| .proc Hex2BCDSorta |
There was a problem hiding this comment.
The Sorta can be removed from the proc name
| bmi TenOrLess | ||
| inx | ||
| clc | ||
| sec |
There was a problem hiding this comment.
Add a comment that this changed from the original code at https://github.com/a2stuff/cdremote which was buggy: it had clc and so produced invalid BCD results for some inputs.
| cmp HexTrackCount0Base | ||
| bmi ExitMathLoop2 | ||
| clc | ||
| sec |
There was a problem hiding this comment.
Add a comment that this changed from the original code at https://github.com/a2stuff/cdremote which was buggy: it had clc and so could result in extra iterations to find a good track.
|
done |
|
Thank you! |
2 participants
Analysis done.
See also
.macpack generic.