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

Upgrade to NES2.0 header #101

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

TakuikaNinja
Copy link

@TakuikaNinja TakuikaNinja commented May 25, 2024

This PR upgrades the header from iNES to the forwards-compatible NES2.0 in order to better describe the cart specification:

  • Add NES2.0 header support (closely matches the header from nointro for the MMC1 configuration)
  • Reflect the SAVE_HIGHSCORES flag in the header (previously, SRAM was always enabled in the header)
  • Add -i build flag for legacy iNES header support
  • Document workaround for NES2.0 header issues on PowerPak

With the BPS patch now being header-agnostic, this should help with adapting to the transition from iNES to NES2.0. (e.g. as seen in #19)

Copy link
Owner

@kirjavascript kirjavascript left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the contribution!

I left a few comments but I dont mind addressing them myself

src/header.asm Outdated
; https://github.com/bbbradsmith/NES-ca65-example

.segment "HEADER"

.include "constants.asm" ; for INES_HEADER

INES_MIRROR = 0 ; 0 = horizontal mirroring, 1 = vertical mirroring (ignored in MMC1)
INES_SRAM = 1 ; 1 = battery backed SRAM at $6000-7FFF
INES_SRAM = SAVE_HIGHSCORES ; 1 = battery backed SRAM at $6000-7FFF
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably add a HAS_SRAM flag for clarity

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that flag be a new addition, or should it replace an existing flag? I think replacing SAVE_HIGHSCORES would make the most sense here. The header should ideally be a set-and-forget kind of thing in my opinion.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be an additional flag so the saving highscores can be disabled separately if needed.

In a newer version we might decide to remove or replace it and leaving the name makes it easier to remove.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the HAS_SRAM flag but disabling it is currently lumped with the -s build flag.

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

Successfully merging this pull request may close these issues.

2 participants