stardot / b-em

An opensource BBC Micro emulator for Win32 and Linux
http://stardot.org.uk/forums/viewtopic.php?f=4&t=10823
GNU General Public License v2.0
112 stars 57 forks source link

CMOS gets mixed up between Integra-B and other machines #155

Closed ZornsLemma closed 3 years ago

ZornsLemma commented 3 years ago

I have a rough idea what's going on but let me try to illustrate the problem first.

*BOOT ?

I think the problem is that main_restart() does

    cmos_save(&models[oldmodel]);

but not all callers of main_restart() have set oldmodel up correctly. In particular, in the example above, when we hard reset the Integra-B machine, oldmodel still refers to the Master 128 and so we save the Integra-B CMOS as if it were the Master 128 CMOS. I base this conclusion on adding:

    fprintf(stderr, "SFTODO: main_restart() oldmodel=%d curmodel=%d\n", oldmodel, curmodel);

just above that cmos_save() line in main_restart() and watching the output as I play around with hard resets and model switches.

I am seeing this with e5adcafef6ac60137fd1d98bcc44974440a2e238 (current master) but think this has been happening for a while. (I don't know if Integra-B got added at some point in the last few months or I just noticed it for the first time in the last few months.)

SteveFosdick commented 3 years ago

I'll have a look.

SteveFosdick commented 3 years ago

Looking at the other places oldmodel is used and where main_restart is used it seems the general assumption in the code is that oldmodel is only different from curmodel when the model is being changed. This explains why it works for the first model change, i.e. in your test case changing from the Master 128 to the Model B with Integra, but then fails on the 2nd model change.

Here's one way to fix that: https://github.com/stardot/b-em/commit/4b911f7ff101eb88135e272c3bacfc40b400c667

Re-trying with your test case that now works for me.

ZornsLemma commented 3 years ago

Thanks, that certainly seems to fix this issue - much appreciated! I will keep using this branch and see if I spot any other problems.

SteveFosdick commented 3 years ago

Ok, I have merged this fix into master.