NES: FDS Accuracy Improvements#133
Conversation
Passes latest FDS-Mirroring-Test Disk registers behave closer to hardware in FDS-4023-Test
$4023.D0 = 0 forces output to $7F Always return good battery status in $4033.D7
Eventually needed by Tonkachi Editor, though it still fails to access disks for editing
Clarify external connector implementation Clarify comment for $4024 IRQ handling Move $4025 IRQ handling into SetFdsControlReg()
Allows audio to play properly in FDS-Audio-Registers
|
Here's how the register view currently looks: |
| //TODO $4025 bit 5 is unknown, all known software sets it to 1 | ||
|
|
||
| _diskReady = (value & 0x40) == 0x40; //TODO $4025 bit 6 is CRC enable, not disk ready flag | ||
| _diskIrqEnabled = (value & 0x80) == 0x80; |
There was a problem hiding this comment.
These are all bools, so you don't need to do these comparisons on them; they'll be true if the right hand side is nonzero and false if 0.
| void Fds::SetFdsControlReg(uint8_t value) | ||
| { | ||
| _motorOn = (value & 0x01) == 0x01; | ||
| _resetTransfer = (value & 0x02) == 0x02; |
There was a problem hiding this comment.
Your wiki documentation has the drive motor on bit 1 and nothing on bit 0. I'm referencing your material as I review this PR and when it's not consistent, I don't know which one is right. The original documentation has the motor on bit 1 (matching your own documentation) and the transfer reset on bit 0, so I'm guessing you have these inverted here.
Looks like this was actually already backward in the original code (I didn't realize at first that this was just moving code). Definitely something to double check, though.
There was a problem hiding this comment.
The bits being swapped here results in their meanings also being inverted (it should be bit 0 = 1: scan media, bit 1 = 1: stop motor). I'll keep the current bool names and simply invert the register read/write logic for now so we don't break savestates, but we might want to revisit their naming once we reach the point of reimplementing the disk loading to use the CRC controls and byte transfer flag properly.
| void ProcessCpuClock() override; | ||
| void UpdateCrc(uint8_t value); | ||
|
|
||
| void SetFdsControlReg(uint8_t value); |
There was a problem hiding this comment.
For stuff like this being moved into a new function, make sure to consult with Sour about whether you need to use __forceinline to avoid a performance impact.
There was a problem hiding this comment.
I've reverted that function anyway, since the values set when $4023.D0 = 0 are constant.
| case 0x4033: | ||
| //Always return good battery | ||
| return _extConWriteReg; | ||
| return _extConWriteReg | 0x80; |
There was a problem hiding this comment.
For clarity, I suggest either adding a note that 'good battery' is indicated by bit 7, or using a named constant here.
2 participants
This is a redo of #85 which avoids changing the current audio implementation.
Working
Not addressing
_soundRegEnabledto reflect its purposeTest programs