stella-emu / stella

A multi-platform Atari 2600 Emulator
https://stella-emu.github.io
GNU General Public License v2.0
610 stars 112 forks source link

Old BUS demos not working correctly #707

Open thrust26 opened 4 years ago

thrust26 commented 4 years ago

Some old BUS demos from SpiceWare are not working as described and on real hardware.

https://atariage.com/forums/topic/258191-bus-stuffing-demos/?do=findComment&comment=3676378

128bus_20170120.zip

sa666666 commented 4 years ago

Yep, someone was discussing making use of versioning, so old BUS ROMs would work. It would be really nice to get these working again, since I look on this as a regression (they did work in an older version of Stella). I suspect the best approach is to find the last time they worked in Stella, and then see what code changed. Some stuff in CartBUS obviously changed, but I think some was in Thumbulator too.

thrust26 commented 4 years ago

Darrell plans to update the demos to the latest BUS driver, but maybe we are faster. 😈

sa666666 commented 4 years ago

This is the commit that broke the ROMS: https://github.com/stella-emu/stella/commit/490724c07978317e74c6491d5084fa58056fd805. Don't know how feasible it is to add support; perhaps we should wait for @SpiceWare to update the ROMs??

thrust26 commented 4 years ago

Yes, probably better. The changes are too large and we do not really understand how the code works.

SpiceWare commented 4 years ago

I seem to recall @sa666666 had decided we wouldn't support the preliminary BUS driver since only some tech demos were done with it. That was a few years ago, so I could have misremembered it.

@thrust26 - if there's a specific demo you're interested in I could take a look at it this week.

thrust26 commented 4 years ago

@SpiceWare The one posted above would be nice.

SpiceWare commented 4 years ago

Will do.

sa666666 commented 4 years ago

No, that's probably what I said. While it would be nice to have the ROMs working, it seems like a lot of work to change the scheme and never have it used again. Probably better to spend the time on the ROMs themselves. Of course, only you @SpiceWare can work on that part, so it all falls to what you want to do.

thrust26 commented 4 years ago

Yes, changing the ROMs, that's what I meant too.

SpiceWare commented 3 years ago

Forgot how primitive the old build process was 1) run dasm 2) copy the echoed out #define statements and paste them in a c header file. 3) compile C code 4) run dasm again

Decided it would be best to migrate it to the new build process, seen in the CDFJ tutorial, that uses a makefile to do everything. The c header file is now automatically created by parsing the dasm symbol file for all symbols beginning with an underscore character.

Took a while, but finally got the first image to show. Over the next week or two I'll migrate the rest of the 128 pixel demo routines over, clean up the code, then post it in the BUS subforum of the Harmony/Melody club.

Screen Shot 2020-10-25 at 7 35 51 PM
sa666666 commented 2 years ago

Looking back at this again, it seems that Stella is freezing on those old BUS ROMs. It would be nice if we could detect those old ROMs, and just display a message saying they're not supported.

sa666666 commented 2 years ago

@SpiceWare, has this been resolved, or will you have time to work on this soon? Just attempting to clean up the Stella Github issues list.

SpiceWare commented 2 years ago

Sorry, RL has kept me away from Atari projects for so long I'd forgotten about this and #745

Looks like I'd started porting the BUS Demos to the last version of the driver. I'll try to resume work on that this weekend. I'll also look into detecting if a ROM is using an obsolete BUS driver so an unsupported message can be shown.

SpiceWare commented 2 years ago

I've reconsidered porting the old demos to the latest BUS driver - I decided it's to much effort to update all of those old projects to the new toolsets we're using for something that we've pretty much abandoned due to the hardware compatibility issues.

Instead I've decided to update Stella to also support the older version of BUS.

Did run into a problem, I got the latest source for Stella and built it - it fails to run the RPG demo, which it should be able to. It crashes in PlusROM.hxx on this line:

bool isValid() const { return myIsPlusROM; }

with this error:

Thread 1: EXC_BAD_ACCESS (code=1, address=0x18)

Not familiar with the Plus ROM stuff.

thrust26 commented 2 years ago

The PlusROM initialization was missing in CartBUS.cxx. I committed a fix, please try again.

SpiceWare commented 2 years ago

Thanks! Made some good headway this evening. So far I've identified 3 versions of the BUS driver in the demos that are linked to in the first post of Bus Stuffing Demos:

  1. RPG driver (newest driver)
  2. Draconian driver (oldest driver)
  3. all others

and have updated Stella to detect and run them.

Still need to update the debugger tabs for BUS, clean up the code, then remember how to submit the changes. It's been so long since I last did anything with github that I just issued the following in Terminal to get the latest source: git clone https://github.com/stella-emu/stella.git

Screen Shot 2022-06-06 at 6 51 50 PM Screen Shot 2022-06-06 at 6 52 36 PM Screen Shot 2022-06-06 at 6 49 44 PM
thrust26 commented 2 years ago

Thanks! Made some good headway this evening. So far I've identified 3 versions of the BUS driver in the demos that are linked to in the first post of Bus Stuffing Demos:

  1. RPG driver (newest driver)
  2. Draconian driver (oldest driver)
  3. all others

and have updated Stella to detect and run them.

Still need to update the debugger tabs for BUS, clean up the code, then remember how to submit the changes.

That sounds good. 👍

It's been so long since I last did anything with github that I just issued the following in Terminal to get the latest source: git clone https://github.com/stella-emu/stella.git

I don't work with GitHub for committing my changes too. Commandline or e.g. TortoiseGit are perfect.

thrust26 commented 2 years ago

@SpiceWare Is bus stuffing really dead? Or are you still working on a solution?

SpiceWare commented 2 years ago

I'm not working on a solution, it's beyond my skill set. Chris was the one working on the BUS driver, with help from Fred. On Friday I was talking with Chris about Boom!, he mentioned: "I don't think we will get stable bus-stuffing any time soon, so I was glad to be able to reboot the game with CDFJ."

Both Chris and John have rebooted their BUS projects to use CDFJ. My most recent changes to Stella were to update CDFJ with the optional ability to override LDX # and/or LDY # for Fast Fetcher use. That change was for John's conversion of his BUS project, Elevator Action, to CDFJ.

Last week's conversation with Chris was the tipping point for me to decide that it didn't make sense to spend a lot of time on the BUS projects. I decided to update Stella as that will take significantly less time than updating all the BUS projects.

SpiceWare commented 2 years ago

I just found 2 old demos that were posted but do not work with my changes, which adds the BUS support from Stella 5 to the most recent BUS support that was added in Stella 6.

They do have Stella screenshots of 4.8 pre_BUS_TEST.  I tracked down source for that, but it doesn't run them. Not surprised as BUS was undergoing a lot of changes at the time.

I'll spend a little time to see if I can figure out how to get those to work. If I'm not able to it's not a major loss as they were never supported in a released version of Stella, and both have newer versions that do work with my current changes.

sa666666 commented 2 years ago

@SpiceWare, we are planning on the 6.7 release for Saturday, June 11. Do you think you can get this stuff included if I push the release to Monday, June 13 instead?

thrust26 commented 2 years ago

@SpiceWare Agreed, if there are newer ROMs available,which work with your changes, then I wouldn't invest too much time.

SpiceWare commented 2 years ago

@sa666666 There's not much left if I'm not going to worry about those 2 old demos, so I'll be able to get the changes checked in by tomorrow night.

I've already shifted the BUS version numbers by 1. BUS0 is now used for the older demos and shows an unsupported message in the debugger.

Is there another way to flag a ROM as unsupported so Stella shows a message when you attempt to load the ROM?

Screen Shot 2022-06-09 at 12 57 17 PM
thrust26 commented 2 years ago

Is there another way to flag a ROM as unsupported so Stella shows a message when you attempt to load the ROM?

Maybe setMessageCallback would work for you.

SpiceWare commented 2 years ago

Thanks! I'll look into setMessageCallback this evening.

sa666666 commented 2 years ago

@SpiceWare, can you also post/send all the BUS ROMs you've tested?

thrust26 commented 2 years ago

He doesn't have to post them, just commit them within the test folder.

sa666666 commented 2 years ago

Yes, committing them is fine too.

SpiceWare commented 2 years ago

Will commit the additional test ROMs with the exception of the 3 I'm not free to hand out. Should I include my notes text file about the identified versions?

I'd contacted Chris and John this morning about my BUS update for Stella:

John sent me the BUS version of Elevator Action to test, but he doesn't want it released until after he's revealed the CDFJ reboot. After that he'll post the BUS version in the Bus Stuffing Demos topic.

Haven't heard back from Chris yet, though I did find a couple early versions of Boom! that he'd previously sent me via PM. I don't think he's publicly released those.

sa666666 commented 2 years ago

Sure, only commit what you have permission for. As for the notes, you can commit that to the BUS ROMs folder as well, I guess.

thrust26 commented 2 years ago

@SpiceWare Do the ROMs you cannot post get autodetected by Stella? Else, maybe add the properties (in Stella.pro) too.

SpiceWare commented 2 years ago

I just checked Stella.pro, "Cart.Type" "BUS" is not in it so all the BUS ROMS have been autodetected by Stella.

sa666666 commented 2 years ago

Yes, BUS ROMs are autodetected with the following code:

  // BUS ARM code has 2 occurrences of the string BUS
  // Note: all Harmony/Melody custom drivers also contain the value
  // 0x10adab1e (LOADABLE) if needed for future improvement
  static constexpr uInt8 bus[] = { 'B', 'U', 'S'};
  return searchForBytes(image, size, bus, 3, 2);
sa666666 commented 2 years ago

@SpiceWare, I've decided to push the release until Monday, June 13, as I'd like to get your changes included in it.

SpiceWare commented 2 years ago

@sa666666 I believe I've finished my changes. I'd done the command

git clone https://github.com/stella-emu/stella.git

in Terminal, so can't submit my changes from my current workspace. Tomorrow I'll get my GitHub fork up to date, get a fresh copy of the source from my fork, then move the changed files over so I can submit the changes.

SpiceWare commented 2 years ago

Since the release has been pushed to Monday I've looked at the original BUS demos some more.

During lunch I was able to make some changes that prevent Stella from immediately "crashing" the older demos into the debugger. Big part of that was due to the newer (supported) BUS drivers being 2K in size, while the older (unsupported) drivers are 3K. This difference caused the 4K banks of 6507 code to be misaligned by 1K for the older BUS demos.

Additionally the new BUS routines support banks 0-6, and defaulted to bank 6, while the older BUS only has banks 0-5.

Hopefully I can get the the older demos working this weekend. If not I'll check in what is working and leave the older BUS versions as "unsupported" for the time being.

thrust26 commented 2 years ago

@SpiceWare Sounds good. But maybe you want to do a pull request before you start working on the older demos again. Then we could already review and test the existing code.

Later you can either update the pull request (if we did not pull) or create a new one.

SpiceWare commented 2 years ago

I've submitted the changes. It's not 100% but is far better than what was there before. For the oldest demos (identified as BUS0) I was able to get 1 of them working, while the others no longer "crash" into the debugger.

We have unexpected company in town from Mexico, so I won't have much additional time to spend on it this weekend.

Screen Shot 2022-06-11 at 6 01 38 PM
sa666666 commented 2 years ago

@SpiceWare, thanks for working on this.

SpiceWare commented 2 years ago

You're welcome. Looks like a check failed though.

https://ci.appveyor.com/project/sa666666/stella/builds/43831654

sa666666 commented 2 years ago

Yes, that's fine. It's because the newly added file wasn't added to Visual Studio. I'm working on that now.

thrust26 commented 2 years ago

@SpiceWare Thanks for your updates. I hope my Pull request instructions helped a bit. Please let me know if something is still not 100% clear.

I wonder why all ROMs are silent. Is there still something missing?

SpiceWare commented 2 years ago

Only BUS3 has working audio. I have some additional test ROMs but they name clashed so I didn't add them.

I'd deleted my Fork then recreated it before submitting my changes. I just attempted to submit the BUS3 ROMs, I'd renamed them to have a bus3_ prefix, but no luck as I'm in that weird state of: This branch is 2 commits ahead, 8 commits behind stella-emu:master.

One of those that I'm ahead is my BUS changes, which are in stella-emu:master so I don't understand why that's considered "ahead". The other is the bus3_ ROMs.

I tried doing a Fetch and Merge but there are conflicts that must be resolved via Command Line. I've not done that before, and don't have time to look into at the moment as I'm getting ready to head over to my folks'.

Anyway here's the additional BUS3 ROMs. Audio and speech both work.

bus3_ROMs.zip

sa666666 commented 2 years ago

I've added these ROMs. @SpiceWare, can we close this one now, or will you continue to work on it after the upcoming release?

thrust26 commented 2 years ago

I'd deleted my Fork then recreated it before submitting my changes. I just attempted to submit the BUS3 ROMs, I'd renamed them to have a bus3_ prefix, but no luck as I'm in that weird state of: This branch is 2 commits ahead, 8 commits behind stella-emu:master.

Which means, your local branch has 2 commits which are not synced to the master. And the master has 8 commit, which are not synced to you local branch.

One of those that I'm ahead is my BUS changes, which are in stella-emu:master so I don't understand why that's considered "ahead". The other is the bus3_ ROMs.

Git is not tracking the individual changes, but the commits.

I tried doing a Fetch and Merge but there are conflicts that must be resolved via Command Line. I've not done that before, and don't have time to look into at the moment as I'm getting ready to head over to my folks'.

Yes, that's a bit more complicated. I am using TortoiseGit here, as I don't like to learn command line commands in 2022.

sa666666 commented 2 years ago

Before doing a commit, always do a git pull to bring your local repo up to date with the remote one. Then do the commit/push. That will fix the error you're seeing.

SpiceWare commented 2 years ago

@sa666666 I'll look at it some more - I tracked down a later version of Stella's source that generates audio for test1_music.bin, one of the BUS2 demos. The audio is a bit distorted, but can still clearly make out the music.

Based on this from bus.h for the older 3K BUS driver, BUS0 did not have audio routines implemented so I should remove the Music registers in the debugger for BUS0.

UNUSED0     DS 1    ; $14    AMPLITUDE?
UNUSED1     DS 1    ; $15    WAVEFORM0?
UNUSED2     DS 1    ; $16    WAVEFORM1?
UNUSED3     DS 1    ; $17    WAVEFORM2?
UNUSED4     DS 1    ; $18    NOTE0?
UNUSED5     DS 1    ; $19    NOTE1?
UNUSED6     DS 1    ; $1A    NOTE2?
UNUSED7     DS 1    ; $1B