-
Notifications
You must be signed in to change notification settings - Fork 101
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
Refactor n64tool, add ed64romconfig binary #153
Refactor n64tool, add ed64romconfig binary #153
Conversation
Add support for long command-line flags and ROM configuration in the header
The magic rom configuration has been verified on EverDrive hardware. A warning has been added for the case of EEPROM + RTC which is not supported on real hardware (but works on most emulators). |
Why do you think the RTC and EEPROM can not work together in real hardware? Did someone test this already? I believe it could be possible. The chips are connected to the same PIF channel. But this is a single wire bi-directional data line. If I remember correctly it is using an open-drain configuration (each chip can only drive the line low). As the chips are using different commands (EEPROM: 0x00, 0x04, 0x05 / RTC: 0x06, 0x07, 0x08), they may work perfectly without any interference. But I never testet it with real chips. |
@networkfusion tested this on real hardware with an EverDrive X7 and was only able to get the RTC to work if EEPROM wasn’t used. If both EEPROM and RTC were enabled, EEPROM would be detected but RTC would not. |
Ah ok. So, at least the ED64 X7 doesn't support it. And real PCBs with both chips don't exist (to my knowledge). Would be a nice experiment to build one :) |
The test ROM in #152 will happily detect both EEPROM and RTC together on emulators. As you said, there's no reason from a software-side that they can't co-exist. I don't know enough about the hardware to say if there's any sort of conflict in actually building an EEPROM+RTC cart. I'm curious as to why the EverDrive X7 is implemented this way and if the 64drive hw2 has the same limitation. I'll ask Marshall and see if he has any insights. |
If I remember correctly from my testing, it should support both... |
Here is the test ROM that uses the header configuration for EEPROM16Kbit + RTC: https://cdn.discordapp.com/attachments/763114214688555039/862140409907380244/ctest_RTCEEPROM16k.zip And here is that test ROM running on @networkfusion 's EverDrive X7: |
Update: EEPROM + RTC works on 64drive hw2 |
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.
Thanks for this PR, I especially like that command line parsing is streamlined in this PR. My comments also contain suggestions for further cleanups.
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.
Great work! I think this is good to go
@@ -94,7 +94,7 @@ static void print_game_high_scores(void) | |||
static int read_game_high_scores(char * path) | |||
{ | |||
printf( "Reading '%s'\n", path ); | |||
const int result = eepfs_read(path, &game_high_scores); | |||
const int result = eepfs_read(path, &game_high_scores, sizeof(game_high_scores)); |
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 is an unrelated change, I think it should not be part of this PR
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.
It's a minor change; I included it because the example was broken, and I needed to test each example's Makefile. The size parameter of of eepfs_read and eepfs_write was a last-second addition to the PR that introduced this example.
if(save_type == SAVETYPE_NOT_SET) | ||
{ | ||
fprintf(stderr, "WARNING: RTC/Region-Free declared without save type; defaulting to 'none'\n"); | ||
save_type = parse_save_type("none"); |
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.
Seems like a convoluted way of writing save_type = SAVETYPE_DISABLED
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.
Oh, that's a relic from before I introduced the #define
constants -- you're right that it's a sub-optimal way to set that constant.
return -1; | ||
} | ||
|
||
write_file = fopen(arg, "r+b"); |
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.
It looks like this could get executed multiple times in a loop. Maybe it should instead be detected as a command line syntax error
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 if block directly above aborts the command if there are any more arguments that would cause the write file to be opened more than once, but I could make this more obvious
{ | ||
/* No way we can have just one argument or less */ | ||
print_usage(argv[0]); | ||
return -1; |
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.
I think in general on UNIX it is more customary to use positive error values for processes. I'd change this and the others to 1.
} | ||
if(save_type == SAVETYPE_NOT_SET) | ||
{ | ||
fprintf(stderr, "WARNING: RTC/Region-Free declared without save type; defaulting to 'none'\n"); |
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.
Do we really need this warning? By looking at the command line syntax, I would say it's pretty clear the intended behavior. If I asked for just RTC, I probably meant it.
@meeq feel free to open a new pr for any remaining changes you have. |
Based on final review notes from @rasky in DragonMinded#153
Changed - Improve fastpath of dfs_read (DragonMinded/libdragon#133) - Refactor n64tool (DragonMinded/libdragon#153, DragonMinded/libdragon#155) Fixed - Zero-initialize the token array to avoid -Werror=maybe-uninitialized (DragonMinded/libdragon#134) - Initialize arguments to main libdragon entrypoint (DragonMinded/libdragon#136) - SD support fixes and dragonfs fopen fix (DragonMinded/libdragon#137) - lib/include paths in tests Makefile (DragonMinded/libdragon#138) - Reenable test_timer_ticks for emulators (DragonMinded/libdragon#140) - n64tool: return error in case the seek offset required backward seek (DragonMinded/libdragon#144) - Add missing extern "C" in debug.h (DragonMinded/libdragon#146) - Ensure C++ global constructors are not garbage collected by ld (DragonMinded/libdragon#148) - Fix clipped RDP rectangle drawing (DragonMinded/libdragon#147) Added - restart_timer and new_timer_stopped functions (DragonMinded/libdragon#131) - dfs_rom_addr (DragonMinded/libdragon#133) - Implement EEPROM Filesystem and test ROM (DragonMinded/libdragon#125) - ed64romconfig binary (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - Support for RTC status/read/write commands (DragonMinded/libdragon#152)
Changed - Improve fastpath of dfs_read (DragonMinded/libdragon#133) - Refactor n64tool (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - It no longer support byte-swapping and only generates a z64 file. Fixed - Zero-initialize the token array to avoid -Werror=maybe-uninitialized (DragonMinded/libdragon#134) - Initialize arguments to main libdragon entrypoint (DragonMinded/libdragon#136) - SD support fixes and dragonfs fopen fix (DragonMinded/libdragon#137) - lib/include paths in tests Makefile (DragonMinded/libdragon#138) - Reenable test_timer_ticks for emulators (DragonMinded/libdragon#140) - n64tool: return error in case the seek offset required backward seek (DragonMinded/libdragon#144) - Add missing extern "C" in debug.h (DragonMinded/libdragon#146) - Ensure C++ global constructors are not garbage collected by ld (DragonMinded/libdragon#148) - Fix clipped RDP rectangle drawing (DragonMinded/libdragon#147) Added - restart_timer and new_timer_stopped functions (DragonMinded/libdragon#131) - dfs_rom_addr (DragonMinded/libdragon#133) - Implement EEPROM Filesystem and test ROM (DragonMinded/libdragon#125) - ed64romconfig binary (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - Support for RTC status/read/write commands (DragonMinded/libdragon#152)
Changed - Removed make, download, init, buildDragon, prepareDragon, and installDependencies NPM scripts - Update the necessary vscode and travis configuration - Update the readme to match - this also fixes #31 - Improve fastpath of dfs_read (DragonMinded/libdragon#133) - Refactor n64tool (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - It no longer support byte-swapping and only generates a z64 file. - Change test bench Makefile to reflect latest changes Fixed - Zero-initialize the token array to avoid -Werror=maybe-uninitialized (DragonMinded/libdragon#134) - Initialize arguments to main libdragon entrypoint (DragonMinded/libdragon#136) - SD support fixes and dragonfs fopen fix (DragonMinded/libdragon#137) - lib/include paths in tests Makefile (DragonMinded/libdragon#138) - Reenable test_timer_ticks for emulators (DragonMinded/libdragon#140) - n64tool: return error in case the seek offset required backward seek (DragonMinded/libdragon#144) - Add missing extern "C" in debug.h (DragonMinded/libdragon#146) - Ensure C++ global constructors are not garbage collected by ld (DragonMinded/libdragon#148) - Fix clipped RDP rectangle drawing (DragonMinded/libdragon#147) - Enable byte swap flag for the make action and update documentation accordingly - Skip the second parameter to the libdragon command as well - Enable --byte-swap flag for the make action Added - restart_timer and new_timer_stopped functions (DragonMinded/libdragon#131) - dfs_rom_addr (DragonMinded/libdragon#133) - Implement EEPROM Filesystem and test ROM (DragonMinded/libdragon#125) - ed64romconfig binary (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - Support for RTC status/read/write commands (DragonMinded/libdragon#152) - Generic libdragon NPM script
Changed - Removed make, download, init, buildDragon, prepareDragon, and installDependencies NPM scripts - Update the necessary vscode and travis configuration - Update the readme to match - this also fixes #31 - Improve fastpath of dfs_read (DragonMinded/libdragon#133) - Refactor n64tool (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - It no longer support byte-swapping and only generates a z64 file. - Change test bench Makefile to reflect latest changes Fixed - Zero-initialize the token array to avoid -Werror=maybe-uninitialized (DragonMinded/libdragon#134) - Initialize arguments to main libdragon entrypoint (DragonMinded/libdragon#136) - SD support fixes and dragonfs fopen fix (DragonMinded/libdragon#137) - lib/include paths in tests Makefile (DragonMinded/libdragon#138) - Reenable test_timer_ticks for emulators (DragonMinded/libdragon#140) - n64tool: return error in case the seek offset required backward seek (DragonMinded/libdragon#144) - Add missing extern "C" in debug.h (DragonMinded/libdragon#146) - Ensure C++ global constructors are not garbage collected by ld (DragonMinded/libdragon#148) - Fix clipped RDP rectangle drawing (DragonMinded/libdragon#147) - Enable byte swap flag for the make action and update documentation accordingly - Skip the second parameter to the libdragon command as well - Enable --byte-swap flag for the make action Added - restart_timer and new_timer_stopped functions (DragonMinded/libdragon#131) - dfs_rom_addr (DragonMinded/libdragon#133) - Implement EEPROM Filesystem and test ROM (DragonMinded/libdragon#125) - ed64romconfig binary (DragonMinded/libdragon#153, DragonMinded/libdragon#155) - Support for RTC status/read/write commands (DragonMinded/libdragon#152) - Generic libdragon NPM script
Enhanced
n64tool
with support for long command-line flags, better error messages, and support for 1M ROM size. Removed support for byte-swapped V64 ROM format.Created a separate
ed64romconfig
binary that can be run aftern64tool
(but beforechksum64
) that sets the EverDrive64 ROM Header Configuration:Refactored all of the
examples
Makefiles to follow standard conventions, add comments for special-cases, and use consistent patterns.Regarding byte-swapping
Premise
Big-endian (
.z64
) byte ordering is the canonical Nintendo 64 ROM format. Little-endian (.n64
) and byte-swapped (.v64
) formats are confusing and only exist for historical reasons that are no longer relevant for the overwhelming majority of use-cases. If there is any legitimate reason to prefer publishing a byte-swapped ROM instead of big-endian, I could not find it.Argument
libdragon has a responsibility to create authentic artifacts that target real Nintendo 64 hardware. Nintendo 64 ROM files are interpreted in a big-endian environment. There should be no question by homebrew developers as to which ROM format to use.
Developers should not be distributing ROMs that require "unmangling" in order to memory-map the data properly. Flash carts and emulators should not have to make assumptions about how to interpret a Nintendo 64 ROM file based on hard-coded magic numbers of the first 4 bytes of the ROM header.
The most developer-friendly approach is to deprecate the
.n64
and.v64
formats and strongly encourage everyone to use.z64
going forward.Counter-Arguments
.z64
to.v64
as part of their build process. When they go to share their work to others, they should publish the.z64
ROM file.n64tool
's byte-swap flag was responsible for two subtle easy-to-miss bugs while modifying the code.n64tool
does not need the additional complexity of byte-swapping. The feature is completely orthogonal to the other functionality ofn64tool
. There are plenty of other tools that solve the generalized problem of byte-swapping a file..z64
) instead, and I don't think the solution to continue generating V64 is difficult, onerous, or unreasonable.n64tool
will suggest the fix if the-b
flag is used, so affected developers will know exactly how to embrace this change.Compromise
The process of building a ROM file should always be in big-endian byte-order. Byte-swapping can be done as a post-processing step after the big-endian ROM file is finalized.
The refactored
examples
Makefiles still support theN64_BYTE_SWAP
flag, but the implementation has changed so that it is now a distinct target that depends on the.z64
build:If a developer really wants a
.v64
ROM, they still can easily create one, but it will no longer be handled byn64tool
.Backlash-anticipation
I understand that libdragon has its own legacy of using the V64 format, and that some individuals wished to continue supporting the byte-swapped format to prevent churn. I welcome anyone to provide opinions on how to improve this proposal.