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

mmc3: using __mapper_detail breaks scanline_irq example #21

Open
claui opened this issue Sep 19, 2023 · 3 comments · Fixed by #22
Open

mmc3: using __mapper_detail breaks scanline_irq example #21

claui opened this issue Sep 19, 2023 · 3 comments · Fixed by #22

Comments

@claui
Copy link
Contributor

claui commented Sep 19, 2023

In mmc3.fab, commit 344fd87 introduces a usage of the __mapper_detail pseudo-register.

This breaks the scanline_irq example, which uses mapper 189 together with mmc3.fab.

Given how similar mapper 189 is to MMC3, I would have expected 189 to support the __mapper_detail pseudo-register, too. However, mapper 189 seems to be missing in the switch statement for detail_size:

nesfab/src/mapper.hpp

Lines 164 to 171 in bde8daf

switch(mt)
{
case MAPPER_MMC1:
case MAPPER_MMC3:
return 1;
default:
return 0;
}

That causes the following error when building the scanline_irq example:

$ nesfab scanline_irq/scanline_irq.cfg
../../../../../../../usr/share/nesfab/lib/mapper/mmc3.fab:24:7: error: Mapper 189 lacks __mapper_detail.
 24 |     {&__mapper_detail}(bank_select) // Shadow register used by the runtime.
            ^^^^^^^^^^^^^^^

With the exception of bankswitch_addr, shouldn’t MAPPER_189 take the same switch branch as MAPPER_MMC3 throughout mapper.hpp?

claui added a commit to claui/nesfab that referenced this issue Sep 19, 2023
@pubby pubby closed this as completed in #22 Sep 19, 2023
pubby added a commit that referenced this issue Sep 19, 2023
`scanline_irq` example: use MMC3 to work around issue #21
@pubby pubby reopened this Sep 19, 2023
@pubby
Copy link
Owner

pubby commented Sep 19, 2023

This is a good issue.

MMC3 got __mapper_detail because its PRG banking is fragile without. Mapper 189 doesn't need it for PRG banking, but it could still make use of it for CHR banking.

Anyway, I think it's reasonable to add __mapper_detail to 189 too. Down the line perhaps a better solution can be found.

@pubby
Copy link
Owner

pubby commented Sep 19, 2023

Pushed some changes to b1.2. I'll leave this open until 1.2 drops.

@claui
Copy link
Contributor Author

claui commented Oct 22, 2023

Works fine with v1.2.

The only thing I noticed is that the scanline_irq and mmc3 examples are now 100% identical. So it may be a good idea to lose one of them.

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 a pull request may close this issue.

2 participants