-
-
Notifications
You must be signed in to change notification settings - Fork 790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GBA hardware extensions #2251
base: master
Are you sure you want to change the base?
GBA hardware extensions #2251
Conversation
…on that allows a game to access a swap memory it can write to or read from.
…he way parameters are passed.
…o disable extensions. Merged HardwareExtensionsView into SettingsView, and finished it in the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of stylistic issues/nitpicks, and some questionable design choices need to be addressed before this is mergeable. I haven't given everything a deep look-over yet as a result.
include/mgba/core/config.h
Outdated
// Extensions | ||
bool hwExtensions; | ||
uint16_t hwExtensionsFlags[HWEX_FLAGS_REGISTERS_COUNT]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be in the core opts since they're specific to one core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed them.
include/mgba/core/core.h
Outdated
size_t (*hwExtensionsSerialize)(struct mCore*, void** sram); | ||
bool (*hwExtensionsDeserialize)(struct mCore*, const void* sram, size_t size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably make more sense to have a "generic" extdata serialize that you specify via passing in an extdata flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
include/mgba/core/serialize.h
Outdated
@@ -16,6 +16,7 @@ enum mStateExtdataTag { | |||
EXTDATA_SAVEDATA = 2, | |||
EXTDATA_CHEATS = 3, | |||
EXTDATA_RTC = 4, | |||
EXTDATA_HW_EXTENSIONS = 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extensions should probably have a larger ID, I'm trying to reserve low IDs for things that are standard on systems. Maybe 0x80.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 0x1F? To make it consistent with the SAVESTATE_HW_EXTENSIONS define.
Since flags in mCoreLoadStateNamed and mCoreSaveStateNamed are declared as int, so I shouldn't exceed 32 bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SAVESTATE_HW_EXTENSIONS
define is fine--note that there's one for metadata already, which is everything over 0x100, so you don't necessarily need to worry about this one exceeding that bit count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Changed the enum to 0x80 and left the define as it was.
include/mgba/feature/commandline.h
Outdated
char hwExtensions; | ||
char hwExtensionsFlags[HWEX_EXTENSIONS_COUNT]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above about configuration. Passing in via a -C flag is probably a better idea, at least for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also removed this.
include/mgba/internal/gba/gba.h
Outdated
@@ -123,6 +124,8 @@ struct GBA { | |||
bool debug; | |||
char debugString[0x100]; | |||
GBADebugFlags debugFlags; | |||
|
|||
struct GBAHardwareExtensions hwExtensions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably group this with the other components closer to the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Moved it under the SIO.
src/gba/hardware-extensions.c
Outdated
case REG_HWEX_ENABLE_FLAGS_0: | ||
case REG_HWEX_ENABLE_FLAGS_1: | ||
case REG_HWEX_ENABLE_FLAGS_2: | ||
case REG_HWEX_ENABLE_FLAGS_3: | ||
case REG_HWEX_ENABLE_FLAGS_4: | ||
case REG_HWEX_ENABLE_FLAGS_5: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of making these registers generic? Why not just have a fixed address per extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason. This way they occupy 1 bit instead of 16. But yeah, I could change it so it works the same as REG_HWEX_ENABLE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/gba/hardware-extensions.c
Outdated
case REG_HWEX_ENABLE: | ||
return 0x1DEA; | ||
case REG_HWEX_VERSION: | ||
return REG_HWEX_VERSION_VALUE; | ||
case REG_HWEX_ENABLE_FLAGS_0: | ||
case REG_HWEX_ENABLE_FLAGS_1: | ||
case REG_HWEX_ENABLE_FLAGS_2: | ||
case REG_HWEX_ENABLE_FLAGS_3: | ||
case REG_HWEX_ENABLE_FLAGS_4: | ||
case REG_HWEX_ENABLE_FLAGS_5: { | ||
uint32_t index = (address - REG_HWEX_ENABLE_FLAGS_0) >> 1; | ||
return *GetHwExIOPointer(gba, address) & gba->hwExtensions.userEnabledFlags[index]; | ||
} | ||
|
||
default: | ||
return *GetHwExIOPointer(gba, address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case
statements are unintended, only their content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
include/mgba/core/serialize.h
Outdated
@@ -16,6 +16,7 @@ enum mStateExtdataTag { | |||
EXTDATA_SAVEDATA = 2, | |||
EXTDATA_CHEATS = 3, | |||
EXTDATA_RTC = 4, | |||
EXTDATA_HW_EXTENSIONS = 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SAVESTATE_HW_EXTENSIONS
define is fine--note that there's one for metadata already, which is everything over 0x100, so you don't necessarily need to worry about this one exceeding that bit count.
@@ -0,0 +1,16 @@ | |||
/* Copyright (c) 2013-2021 Jeffrey Pfau |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should still be merged into extensions.h
--there's no strong reason to keep it separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -13,6 +13,7 @@ set(SOURCE_FILES | |||
cheats/parv3.c | |||
core.c | |||
dma.c | |||
extra/extensions.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extras go in the EXTRA_FILES
section below, but the feature will need to be able to be ifdef'd out for that to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I ifdef'd under MINIMAL_CORE or do I create a new define?
…from core options. Renamed said options in the gba core. Renamed "More RAM" to "Extra RAM". Removed flags registers and added one register to specifically enable the extra RAM extension.
…sions serialization functions with more generic extdata serialization functions.
…hree more commands to the extra RAM extension: init, resize and destroy.
As we were discussing on discord, here's the draft PR for GBA hardware extensions.
Whats is currently implemented is: