Slot-2 addon support (v1.0)

[Buenia0]

This pull request adds support for the DS Rumble Pak (packaged with Metroid Prime Pinball, compatible with several DS games), the Guitar Grip (required for the Guitar Hero: On Tour games), and the DS Memory Expansion Pak (required for the DS Browser).

Located in the emulator settings dialog is a drop-down menu (below the "Console type" drop-down) for choosing the desired Slot-2 addon.
The "Rumble Pak" option emulates the DS Rumble Pak, the "Guitar Grip" option emulates the Guitar Grip used by the Guitar Hero: On Tour games, the "Memory Expansion Pak" option emulates the DS Memory Expansion Pak, and the "None" option emulates no Slot-2 addon.

The Guitar Grip implementation comes with assignable hotkeys for the Guitar Grip's four colored buttons, which can be configured in the "Addons" tab of the input config dialog.

I intend on solving most, if not all, of the add-on related issues in mainstream melonDS, which is why this proposed Slot-2 add-on implementation constitutes the initial version of many. As such, if this pull request is merged, expect support for more add-ons, such as the "Tony Hawk" motion pak, the "Slide Controller", and others, to be added in future versions.

I'm opening this pull request early for any feedback related to coding style and UI design.

Here is a demo of the Memory Expansion Pak implementation:

https://drive.google.com/file/d/1l1PTeNzFacDgqN5hr5-PmjXi2T_rE_iu/view?usp=sharing

Demos for the other emulated add-ons are available upon request.

[RSDuck]

skimmed over it and looks good to me

@rzumer do you have some thoughts on this?

[rzumer]

One thing I'd like to ask before I get around to reviewing the actual code is to squash formatting fixes into their relevant commits (or fix them "pre-emptively" in an interactive rebase).

Also for reference, this closes #99, #73, and #384.

[Buenia0]

Sure, I'd be happy to squash the formatting fixes into their relevant commits! The fixes should be done within the next few hours!

[rzumer]

Thank you for the PR, for the most part it looks good, clean and coding style is consistent. I have some style nitpicks and a couple of more substantial issues. I invite @Arisotura to also take a look at code style and design choices.

[rzumer]

Please add an inline comment about why the argument is 16 (= 1 frame in ms).

This length of time also seems arbitrary, and I'm not sure that melonDS steps one frame at a time. We might want to reduce this number to increase accuracy if not.

[RSDuck]

theoretically it's possible to have frames shorter or longer than 1/60s

[rzumer]

I assume it would also "stutter" (send pulses too slowly) if the emulation runs slow due to system performance issues. In that case having an on/off toggle for rumble and resetting it regularly if there's no more pulse requested might be best.

[Buenia0]

Just extended the millisecond argument to 48, just in case the emulation suffers from performance issues.

[rzumer]

That's still arbitrary and only works up to a certain threshold of performance degradation, on top of being of questionable accuracy. I'll let @Arisotura comment on how large emulation steps can get.

[rzumer]

As mentioned earlier, just sending a single-frame pulse is not necessarily the best implementation. For rumble to persist correctly even when the emulator is lagging, the rumble should have an on/off toggle, or at least send a long pulse (maybe several frames worth), and explicitly stop rumbling after some undetermined amount of emulation time (like one frame but maybe less) has passed without the cartridge being signalled to rumble.

Unless it's been documented that a single rumble instruction sends a 16ms pulse anyway.

[Buenia0]

Currently working on a fix for that. Will let you know when it's ready.

[rzumer]

After thinking about it some more I'm not sure my approach is good either. Since the rumble cartridge does not use a motor and just sends pulses at some frequency determined by the software, constantly rumbling to account for lag will not be accurate anyway. Pulse timing will probably be slightly off depending on the emulation step size.

I'd like to test different implementations and see which compares most to the "real thing" or at least my ezflash if I can find it, but I won't have access to my hardware for a few weeks.

[Buenia0]

No worries, rzumer. Take your time...

[Buenia0]

Hello, rzumer. Have you been able to access your hardware and test the different Rumble Pak implementations yet? If so, what were your findings? I just need to know that so I can determine how to properly implement the Rumble Pak's pulse timing!

[Buenia0]

Thanks for the input! I'll fix the formatting issues ASAP!

[Buenia0]

Alright, it's past 9 pm where I live right now. I'll continue work on this PR tomorrow, okay?

[Buenia0]

In the meantime, I'll temporarily mark this as a draft and work on fixing the remaining issues over the next few weeks, if that's okay with you...

[SA-X0]

Seems the other branches don't like this one, recent build again Slot-2 addon support is gone.

[rzumer]

There are still some unresolved comments, make sure to expand collapsed discussions. You should be able to mark ones that were dealt with as resolved to collapse them by default.

[Buenia0]

Alright, just resolved most of the unresolved comments. I'm still waiting on the rest until we have more information on those fronts. Hope this gets merged soon!

[Buenia0]

I think I know why the Ubuntu aarch64 build is failing. It's because of an installation issue with the grub-efi-amd64-signed package. I have no idea what's causing that error, so if anyone has any ideas as to why that's the case, please let me know!

[RSDuck]

what still bugs me, is that your changes aren't really integrated @rzumer's previous slot-2 work. It would probably be the best if the solar sensor and the gba cart were integrated into your scheme of slot2 additions (both from the code, but also from the UI).

That opens of course more questions, the solar sensor for example is automatically inserted for the associated games. Should that be performed with other peripherals as well? Yesterday we also had some discussions about refactoring Slot1 and Slot2 devices as polymorphic classes.

Please tell us what you think! If you want to do it, should further refactorings go right into this PR or be made with separate one?

[rzumer]

That opens of course more questions, the solar sensor for example is automatically inserted for the associated games. Should that be performed with other peripherals as well?

I think you're conflating the GBA cartridges that enable the sensor controls when inserted, and the DS card that samples the sensor. There's no sensor "inserted" with the DS card alone. Detecting GBA cartridges that can vibrate and enabling rumbling when they are inserted would be another example of that. In most cases it doesn't make much sense to automatically insert a peripheral based on the DS card.

[RSDuck]

I think you're conflating the GBA cartridges that enable the sensor controls when inserted, and the DS card that samples the sensor. There's no sensor "inserted" with the DS card alone. Detecting GBA cartridges that can vibrate and enabling rumbling when they are inserted would be another example of that. In most cases it doesn't make much sense to automatically insert a peripheral based on the DS card.

oh sorry, you're right. Also good point regarding GBA games which can rumble, so ideally Slot-2 peripherals and GBA games should be separated (from a code standpoint, so that the functionality can be reused).

[rzumer]

oh sorry, you're right. Also good point regarding GBA games which can rumble, so ideally Slot-2 peripherals and GBA games should be separated (from a code standpoint, so that the functionality can be reused).

I think so too, but a GBA cartridge is still a slot-2 device, so it's not intuitive for it to be separated from the others in the interface.

[WaluigiWare64]

I think I know why the Ubuntu aarch64 build is failing. It's because of an installation issue with the grub-efi-amd64-signed package. I have no idea what's causing that error, so if anyone has any ideas as to why that's the case, please let me know!

@Buenia0 fixed this with PR #764.

[Buenia0]

Thanks so much, WaluigiWare64!

[WaluigiWare64]

There seems to be extra whitespace at the end of this line.

[Buenia0]

Thanks for reminding me, WaluigiWare64! I'll fix it as soon as I can!

[rzumer]

Just as a reminder there are still unaddressed issues:

• Slot-2 cartridge type is still hardcoded integers only defined in a comment, should be enum values
• GBA cartridges are still separate from all other slot-2 cartridges, which leads to unintuitive/confusing behaviour

[poudink]

how do you want slot-2 stuff to be "merged"? like a drop down menu the emu settings menu where you can choose between using a gba rom or a slot 2 accessory?

[rzumer]

My idea for a first implementation would be something like that, but I'm open to alternatives.

• GBA cartridge as an option in the slot-2 dropdown
• GBA cartridge either settable from the settings menu, or cartridge type set to GBA automatically when a new GBA ROM is successfully loaded from the Open... menu or drag-and-drop, with an OSD message to reflect that
• GBA cartridge/ROM unloaded/reset if it is swapped for another peripheral

This reminds me slot-2 cartridge type should be stored in save states if it is not there already.

[RSDuck]

how do you want slot-2 stuff to be "merged"? like a drop down menu the emu settings menu where you can choose between using a gba rom or a slot 2 accessory?

yes, correct me if I'm wrong, but I think currently you can select both a gba rom as well as a slot 2 peripheral at the same time and it's only a matter of how the things are ordered in the code what will happen.

[Buenia0]

Just as a reminder there are still unaddressed issues:

• Slot-2 cartridge type is still hardcoded integers only defined in a comment, should be enum values
• GBA cartridges are still separate from all other slot-2 cartridges, which leads to unintuitive/confusing behaviour

Currently working on resolving those right now, rzumer! Should have something ready soon!

[kyandora]

@rzumer Are all values in GBACart(_SRAM)::DoSavestate actually used? I've been reworking GBACart trying to fit it in with the rest, but I can't fully wrap my head around the save states.

[rzumer]

@rzumer Are all values in GBACart(_SRAM)::DoSavestate actually used? I've been reworking GBACart trying to fit it in with the rest, but I can't fully wrap my head around the save states.

What is confusing you about it?

Save states store emulator state to restore them correctly when loaded. For the GBA cartridge the behaviour is a bit special because we can't reasonably store the full GBA cartridge ROM in the save state, both because it can be excessively large and because it can make sharing save states difficult from a legal standpoint. We also can't reasonably store a file path to a GBA ROM, because they can move around at any time and it would be annoying for a save state to not work as expected due to an associated GBA ROM having moved somewhere else. Save states are associated with DS games, so there is no such issue with DS ROMs moving around (they need to be loaded in advance anyway).

The solution I came up with was to only store the GBA cartridge header and any save data. This allows DS titles that detect inserted cartridges to identify the inserted game, and for titles that read save data (like Pokémon games) the functionality remains intact, though if anything is written to the GBA save data after loading a save state, it won't be stored back onto the disk.

Now that I think about it there are some titles that read from the GBA cartridge ROM, so this is not a perfect solution. I posted an issue about it at #796.

[Buenia0]

Just as a reminder there are still unaddressed issues:

• Slot-2 cartridge type is still hardcoded integers only defined in a comment, should be enum values
• GBA cartridges are still separate from all other slot-2 cartridges, which leads to unintuitive/confusing behaviour

Currently working on resolving those right now, rzumer! Should have something ready soon!

Quick update: a (WIP) pull request by a new contributor should fix those issues:

Buenia0#2

[Buenia0]

Quick update on this pull request: I'm currently finishing up my fourth semester of college, and am currently swamped with final projects and exams. As such, I won't be able to work on this pull request for a few weeks. But I assure you, once that semester of college is behind me, work on this pull request will pick up the pace! Have a belated Happy Thanksgiving!