pubby/nesfab

mmc3: using `__mapper_detail` breaks `scanline_irq` example

claui opened this issue · 3 comments

claui commented

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?

pubby commented

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 commented

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

claui commented

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.