Input fixes and VBlank ISR rewrite#282
Merged
alekmaul merged 42 commits intoalekmaul:developfrom Jun 10, 2024
Merged
Conversation
to free VBlank time that can be used by the `nmi_handler`
The functions that are called by the VBlank ISR have been moved to vblank.asm
These functions are called by the VBlank ISR and should not be called multiple times per frame.
The getpad1data loop did a 16-bit read of REG_JOYA, which also reads from REG_JOYB, preventing the getpad23data loop from reading joypad values. scanMplay5 now tests if a standard controller is connected to the multitap. This commit also optimises the multitap reading code by: * Using Joypad Auto-Read to read the first three controllers. * Using ScanPads to read the first two controllers. * Using an 8 bit A when reading the last two controllers. * Using the carry output of `rol` to determine when all of the controller bits have been read. As a consequence of the Joypad Auto-Read optimisation, the WRIO register must not be written to when `snes_mplay5` is set. I do not think this will be an issue, since `REG_WRIO` (nor 4201) is not defined in the header files nor the snes-examples directory. Tested by running the multiplay5 example in Mesen
This is safe as the input reading subroutines/macros are private to the vblank_isr section
During a lag frame: * oamMemory will not be transfered to the PPU OAM * the pads/mouse/scope will not be read nmi_handler will be called on every VBlank interrupt. The vblank_flag variable can be used to detect lag-frames in the nmi_handler.
Crash was caused by a missing `.BASE 0` in the `.vblank_bss` ramsection. This commit also fixes 2 more sections with the wrong .BASE.
Modifies WaitForVBlank() so it can be called with any DB register value. Also documents that WaitForVBlank() will preserve A/X/Y registers.
The speed_change jsr in `_MouseRead` can crash if `mouseSpeedChange` is in a different bank to `_MouseRead`. speed_change has been changed to a child label to illuminate that `_MouseRead` is calling a subroutine outside of vblank.asm.
mouse_read has been moved to vblank.asm and the same comments already exist in vblank.asm.
Saves 2 cycles and matches the _GetScope call in the VBlank ISR.
Writes to nmi_handler are not atomic and the VBlank ISR can crash if a VBlank Interrupt occurs in the middle of the nmi_handler write. To fix this crash, nmiSet() is now a function that will temporarily disable interrupts so nmi_handler can be safely written to. This commit also modifies consoleInit() to use nmiSet().
* Test for lag-frames * Remove oamMemory DMA transfer (now handled by the VBlank ISR) * Remove snes_vblank_count++ (now handled by the VBlank ISR)
These functions did not multiply the argument by 2 when converting the `value` argument (0-4) to an X index register value (0, 2, 4, 6, 8).
I have tested and confirmed `padsDown(i) == pad_keysrepeat[i]` before making this change.
padsClear() is not clearing the high byte of the pad variables. Using long addressing is faster then changing DB register. Also: * Added bounds testing to padsClear() * padsClear() can be called with an 8 bit Index
* Document pad arrays. * pads*() value parameter is an index, not an address. * Clarify pads*() parameter range
It is only used in the NMI ISR and should not be used for IRQ interrupts.
Fixes potential memory corruption in NMI ISR.
The VBlank ISR changes the Direct Page Register to `tcc__registers_nmi_isr`. If `tcc__registers_nmi_isr` is not page aligned, a 1 cycle penalty will be applied to all direct-page instructions in the VBlank ISR (including the `nmi_handler`).
`vblank_flag` can be used to determine if it is OK to read from `bgInfo` in `myconsoleVblank()`.
Contributor
Author
|
Fixed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR started as series of fixes to the input reading code and evolved to me rewriting and improving the VBlank ISR.
Continuing on from develop commit 555a85b,
scanPads()andscanMPlay5()has been inlined to the VBlank ISR and the input reading code has been moved to a newvblank.asmfile.Superscope and mouse reading code is mostly unchanged. I moved it to
vblank.asm, changed the labels and changed how the code was called (swapping ajslwithjsr).As discussed on Discord, the new VBlank ISR can detect lag frames. While I was testing I also discovered a few more bugs in the existing VBlank ISR, causing me to practically rewrite it.
I've also added a lag-frame counter. This u16 variable is user editable and is incremented on every lag-frame by the VBlank ISR.
The code was tested by running all of the examples on my 1-CHIP NTSC Super Famicom console.
Two examples remain untested on real hardware, could you arrange for these to be tested before merging this PR.
As the PVSnesLib changelog is handled by GitHub releases, I have written out the list of normal and breaking changes here:
Breaking Changes
Interrupts:
WaitForVBlank()should be unaffectedwaito pause execution will be treated as a lag-frame.See
interrupt.hdocumentation for more details.nmi_handleris called on every VBlank interrupt (ie, no lag-frame detection).oamMemoryto the PPU OAM on non lag-frames.nmi_handler's that contain a DMA transfer to OAM should remove it.nmiSet()is now a function.nmiSet()will disable IRQ interrupts, enable VBlank interrupts and enable Joypad Auto-Read.tcc__registers_irqhas been renamed totcc__registers_nmi_isr(it is not used by IRQ interrupts).Inputs:
WaitForVBlank()within the loop otherwise it will loop forever (See commit d450096).keysUp()andkeyDown()events during lag-frames.scanPads(),scanMPlay5(),mouseRead(),scanScope()scanPads()andscanMPlay5()has been inlined into the VBlank ISR.mouseRead()andscanScope()is now private and automatically called by the VBlank ISR.pad_keysrepeattopad_keysdownto better describe what it holds.padsDown(),padsUp()andpadsClear()argument has changed to matchpadsCurrent().padsDown(0)is unaffected.padsDown(1)now returns the down keys of the 2nd controller.padsDown()is now a macro.Changes
Interrupts:
interrupt.hdocumentation for more details.vblank.asmfile.WaitForVBlank()is used to detect lag frames. A lag-frame is when a VBlank Interrupt occurs and the S-CPU is not inWaitForVBlank().nmi_handler's can test for lag-frames using thevblank_flagvariable.See
myconsoleVblank()in the Mode1ContinuosScroll example.lag_frame_counternmiSet().RAMSECTIONs that would have movedtcc__registersout of address 0 now cause a wla-dx linker error.D.l != 0direct-page cycle penalty.Inputs:
nmi_handler)scanPads()andscanMPlay5()erroneously readingREG_RDIO.pad_keysnot cleared if input is not a standard controller.scanMPlay5()not reading controllers 2 & 3 (3a3db70).pad_keysif the input is not a standard controller.padsClear()not clearing the high-byte ofpad_keys*variables.padsClear()andpadsUp()pad_key*array size ininput.h