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

Add NES UNROM-512 mapper support, mapper defines to subtargets. #191

Merged
merged 7 commits into from
Sep 11, 2023

Conversation

asiekierka
Copy link
Contributor

All sub-modes are supported, as is both CHR-ROM and CHR-RAM.

This also fixes "bus conflict" behaviour in the UNROM mapper implementation; well, it doesn't exactly fix it, it papers around it - as per the NESdev wiki, programmers should not rely on the "bitwise AND" behaviour of the bus during a conflict. It also marks the submapper in the NES 2.0 header as a non-bus-conflict one.

I'm not sure how to best fix it properly - given the major ROM usage increase required to do so (256 bytes, fewer if one could vary it by linker script), there's some discussion opportunity to be had here, IMO. Probably best for a new issue.

@asiekierka asiekierka changed the title Add NES UNROM-512 mapper support. Add NES UNROM-512 mapper support, mapper defines to subtargets. Sep 10, 2023
@asiekierka
Copy link
Contributor Author

Fixes #173

mos-platform/nes-unrom-512/bank.h Outdated Show resolved Hide resolved
mos-platform/nes-unrom-512/bank.c Outdated Show resolved Hide resolved
mos-platform/nes-unrom-512/bank.c Outdated Show resolved Hide resolved
}

__attribute__((leaf)) inline char get_chr_bank(void) {
return get_bank_state(UNROM_512_CHR_BANK_MASK) >> UNROM_512_CHR_BANK_SHIFT;
Copy link
Member

Choose a reason for hiding this comment

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

Nice, this is a clean pattern.

/**
* @brief Define this in a .c file to use 2-screen vertical mirroring.
*/
#define UNROM_512_USE_2_SCREEN_VERTICAL \
Copy link
Member

Choose a reason for hiding this comment

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

Also, man this is a really nice pattern; we should have these for most settings in the SDK I think. NAR here; I made #193 for this.

mos-platform/nes-unrom-512/common.ld Outdated Show resolved Hide resolved
mos-platform/nes-unrom-512/prg-rom.ld Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ banked_call:
sta __rc17 ; __rc17 = requested PRG bank
Copy link
Member

@mysterymath mysterymath Sep 11, 2023

Choose a reason for hiding this comment

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

Ah, I hadn't noticed that the RC value is live across the call, right.

Since __rc20 is caller-saved, the value on entry to banked_call needs to be saved and restored too, à la lda __rc20; pha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! Should be fixed.

Copy link
Member

@mysterymath mysterymath Sep 11, 2023

Choose a reason for hiding this comment

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

Aaand this clobbers the value on entry in A; the store to __rc17 needs to happen first ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I've had my head in a whole other place...

Copy link
Member

@mysterymath mysterymath left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

@mysterymath mysterymath merged commit 339dcdc into llvm-mos:main Sep 11, 2023
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