Skip to content
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

n64tool and -l / --size option #233

Open
rasky opened this issue Nov 30, 2021 · 0 comments
Open

n64tool and -l / --size option #233

rasky opened this issue Nov 30, 2021 · 0 comments

Comments

@rasky
Copy link
Collaborator

rasky commented Nov 30, 2021

n64tool underwent major surgeries in the past months which improved its stability a lot, and it's now a very mature tool. Unfortunately, there is one unresolved issue on the -l / --size option.

The option specifies the final ROM size, excluding the 4K header (which contains IPL3). It used to be mandatory but nowadays it's optional: if the user doesn't specify it, n64tool will create a ROM which is as small as possible, while still respecting several constraints (eg: minimum total size of the ROM file mandated by IPL3 is currently 1028Kib, aka 1Mb + 4K of header itself).

There might be an argument to transition this command line flag to actually refer to the total size of the ROM file, that is including header. We could either just change its behavior, or deprecate that option and introduce a new one that has this semantic (smoother transition for old code). Whatever migration plan we agree on, I would like to discuss whether it makes sense to switch this new semantic.

Letting the user specify a size including header has the following benefits:

  • If the user says --size 8M, they would get a 8 MiB file. Today, they get a 8196 Kib file (8MiB + 4KiB). This does not matter for emulators but obviously you cannot flash a 8196 KiB file on a 8Mb replica cartridge. They can of course truncate it afterwards, but they need to make sure that the truncated part of the file is only padding. Otherwise, they need to use --size 8188K during development to specify a size that will round up to 8 MiB after adding the header, which is surprising and not expected.
  • iQue Player requires ROM files to be multiple of 16 KiB (including header). When not forcing a specific size, n64tool can roundup to 16 KiB by itself. In case the user specifies a size, we would like to add a warning on stderr in case the specified size is not iQue compatible. That warning would trigger for all "round" numbers (eg: --size 4M would trigger a warning, as it generates a 4100 KiB file. Notice that the warning would have to explain the problem, because it is not sufficient to say "specified size is not multiple of 16 KiB", because the user specified 4M and could get confused).

The new approach has the following potential problems:

  • --offset specifies an offset in bytes excluding header. This is actually a good property to preserve because that offset also correctly maps to a memory address. For instance --offset 2M means "the data file will appear at 0x1020000 in the memory map". In fact, the option is documented as Next file starts at <offset> from top of memory. The new approach for --size would create an asymmetry, but maybe it is fine this way.
  • Since specifying a size was mandatory, it was normal and very common to specify -l 1M to mean "the minimum possible size for a ROM" (as that would creates a 1028 KiB file). A shorter file might create problems because not all ROM loaders might pad it correctly to let the IPL3 checksum test pass (though the ecosystem has evolved in this regard as well). By switching to the new semantic, -l 1M would actually emit an error and abort, as the specified file would be too short. The error would mention that the minimum size is 1028 KiB but maybe that breaks too many existing Makefiles. Also notice that for those cases, it would be now sufficient to drop the -l option altogether, as n64tool by default always pad to at least 1028 Kib anyway.
  • The migration might be a bit confusing. We should probably mark -l / --size as deprecated, emitting a warning (but still making them work), and then add a new option with a different name with the new semantic. Not even sure how we would call it, as the obvious name is unfortunately taken.

Notice that we've already changed behavior of n64tool once (we dropped --byteswap), so there's a precedent in doing it, but I am always instinctively against breaking backward compatibility.

Any comment or insight is appreciated before we try to reach a final decision on this

rasky added a commit to rasky/libdragon that referenced this issue Nov 30, 2021
This is required for a ROM to be loaded on iQue. It shouldn't matter
much for everybody else, and --size is respected anyway when provided,
so this patch only affects the autogenerated size.

Notice that, pending DragonMinded#233, this commit does not introduce a warning
for explicit sizes which are not multiple of 16 KiB. We can add it later
once we have decided the desired semantic for --size.
rasky added a commit that referenced this issue Nov 30, 2021
This is required for a ROM to be loaded on iQue. It shouldn't matter
much for everybody else, and --size is respected anyway when provided,
so this patch only affects the autogenerated size.

Notice that, pending #233, this commit does not introduce a warning
for explicit sizes which are not multiple of 16 KiB. We can add it later
once we have decided the desired semantic for --size.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant