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

Fix synchronization for MC6847 video display generator on Dragon (PAL) machines #11743

Merged
merged 9 commits into from Dec 22, 2023

Conversation

dave-br
Copy link
Contributor

@dave-br dave-br commented Nov 13, 2023

@tlindner: FYI

Fixes: #11658

PROBLEMS:

  1. My prior commit (a9519eb) neglected PAL machines, and worsened the aspect ratio for Dragons

  2. Dragon timings have always been incorrect, leading to nasty shimmering while running some delicate tests

  3. My last fix improved coco_cart:drgnfire, but still left an extra purple outline at the bottom of the moat (NTSC CoCo).

DETAILED FIXES:

  1. 6847 implementation now supports differences between NSTC and PAL machines. Adds PAL-padding scanlines at the correct locations within the field and informs screen device of the correct dimensions when those scanlines are present.

  2. Improve cycle timings of HS & FS interrupts (both PAL and NTSC). Differentiate between PAL & NTSC on when FS is issued.

  3. Eliminate redundant frame timer, which was muddying the code. Frame now changes by virtue of the last scanline getting rendered. Scanline timings are accurate now, so that such as a thing is reasonable to do.

  4. There are 312, not 313, scanlines for PAL machines

  5. Dragons need to use a consistent crystal for all components. Dragon 6847 now bases its clock on the same 14.218_MHz_XTAL as used by SAM & CPU

  6. coco_cart:drgnfire: no more purple outline at the bottom of the moat on NTSC CoCo.

Communicate to screen device the PAL padding lines, and treat them properly when rendering and choosing whether to issue HS interrupt.
Tuning parameters to improve results of tmgtest_v1_3 test suite and Dragonfire
@dave-br
Copy link
Contributor Author

dave-br commented Nov 14, 2023

I see there are errors in non-Windows builds. Will take a look.

Added back new_frame to mc6847 that coco3 can override, but now just a simple virtual function, not a timer callback
coco3's gime passes the new pal param into the mc6847_friend_device ctor
adjusted coco3's field sync trailing edge scanline number down by 2 so demos work as before
adjusted tpfs to 262 to match the shared mc6847 class, so that FS happens on same scanline for each field
@dave-br
Copy link
Contributor Author

dave-br commented Nov 18, 2023

I added a commit to deal with coco3. It needed adjustments to coexist with the mc6847 changes I'd made in my previous pull request (a9519eb) as well as this pull request.

I confirmed the Sock Master coco3 demos now work as they did before my previous pull request.

Details:

  • coco3 had been overriding new_frame, which I'd removed as it and its associated timer were unnecessary for mc6847. My removal caused some build failures. I added back new_frame for coco3 to override, but now just as a simple virtual function called explicitly at the right time, and not as a timer callback
  • coco3's gime passes the new pal param into the mc6847_friend_device ctor
  • adjusted coco3's field sync trailing edge scanline number down by 2 so demos work as before
  • adjusted tpfs to 262 to match the shared mc6847 class, so that FS interrupt happens on same scanline for each field

The bottom line of the active video area was cut off (it was rendered as part of the bottom border instead).  This adjusts the screen update function to start the bottom border at the correct line.
@dave-br
Copy link
Contributor Author

dave-br commented Nov 24, 2023

Apologies for the late addition. I recently discovered that my previous pull request (a9519eb) introduced an off-by-one bug which cut off the bottom line of the active video area (it was rendered as part of the bottom border instead). I've just added a commit to this pull request to fix that line.

I've retested CoCo 1/2, Dragon, and CoCo 3, to ensure they're still ok after this.

Copy link
Member

@cuavas cuavas left a comment

Choose a reason for hiding this comment

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

This seems kind of gross with all the use of the conditional operator. Wouldn’t it be better to up counts for padding, etc. on construction, configuration or start?

Philisophically, if the 6847 doesn’t actually support PAL but the timings are messed with using additional glue logic, wouldn’t it be better to either make the 6847 device configurable or expose the signals that the glue logic uses to mess with the video timings?

From reading the updated comment, it seems like the PAL 6847 device represents something that doesn’t really exist as such.

…n the constructor, assigned to reusable const members
@dave-br
Copy link
Contributor Author

dave-br commented Nov 27, 2023

@cuavas :
I've turned the PAL conditional expressions into const members. This means they can't be initialized in ctor body with a single if (m_pal) { ... init everyone ... } / else { ... init everyone ... } ). So I'm still stuck with conditional expressions in the initializer list. But it's still arguably cleaner having them in one place. Also, some duplication was removed this way, specifically with m_lines_top_border & m_lines_until_vblank. While I was at it, I turned m_field_sync_falling_edge_scanline const as well.

RE 6847 & PAL... Yes, it's my understanding that there is no such thing as a PAL version of the 6847. For example, the Dragons apparently add circuitry that literally stops the 6847's clock and manually sends blank lines to the RF modulator during each field. Modeling this precisely would be tricky--right now the 6847 specifies the clock to the screen device, and it's got the extra padding factored in. Since the 6847 doesn't really know about padding, perhaps that belongs someplace else. And perhaps the screen device might need some extending (e.g., what pixel clock would make sense to specify when initializing the screen, and should that clock change back and forth somehow in each field as the 6847's clock gets suspended and resumed)?

Complicating matters is that the CoCo apparently had its own PAL versions released. I don't know how popular they were, as I think most PAL residents would have bought Dragons instead, but just guessing. In any case, PAL CoCos may have manipulated the NTSC 6847 in ways different from the Dragons (e.g., where in the field scan lines are added, how many are added, and what kind of clock was used). I haven't heard of anyone trying to experiment on physical PAL CoCos to see what's actually going on. Not to suggest that I'd necessarily know about it, though.

You wondered about making the 6847 configurable. Are you talking about end-user configurable (i.e., .ini and command-line)? I don't think that kind of power would be useful or safe in the hands of end users, as there is a limited number of cases to cover from real machines. If you're talking instead about extra constructor parameters to the 6847 class, that's quite sensible. But we'd need to know how PAL is implemented across the different Dragon & PAL CoCo variants to know what parts of the 6847 code to make extensible.

Sorry if I'm minsterpreting your suggestions, feel free to clarify if so.

@tlindner
Copy link
Member

tlindner commented Dec 9, 2023

Looks good to me.

@npwoods
Copy link
Contributor

npwoods commented Dec 15, 2023

Looks good to me too :-)

Also, you're "Coco Town" guy, right? I just saw https://www.youtube.com/watch?v=IFB1bSU0T1s and as the original author of much of the code you presented, I'm quite happy to see your work. When I rewrote the CoCo, 6847 and 6x09 CPU core about a decade ago, while I got closer to getting Dragon Fire working, as you saw I never got it perfectly.

@dave-br
Copy link
Contributor Author

dave-br commented Dec 15, 2023

Yep, that's me. And you must be Nathan Woods? It's really cool to hear from you. What you've done is incredibly impressive, to say the least. There was so much work that went in to emulating all of CoCo components, I don't know if I'd ever be able to internalize even half the code you wrote!

@npwoods
Copy link
Contributor

npwoods commented Dec 15, 2023

Yes that would be me - and thank you :-). I've been pretty busy with "real life syndrome" and I'm happy to that things are not atrophying in my absence - a benefit of working in the MAME world.

One piece of trivia - if I recall correctly when I was trying to get the CoCo timing working, the MAME core did not have the flexibility to customize the timing of the screen devices. So I was forced to ignore the beamx/beamy stuff that the screen devices surfaced in the debugger.

This is actually a very common pattern in MAME development - the MAME core aspires to be a common framework to support everything from Pong to SGI Workstations, and it is inevitable that not everything perfectly fit like a glove. But when the core does advance, work needs to be done to retrofit things using the newly kosher techniques..

@dave-br
Copy link
Contributor Author

dave-br commented Dec 16, 2023

Makes sense. And not only did my work benefit from an improved MAME core, but also Stewart Orchard's Dragon32/64 timing tests helped with the CoCo timing as well, Those tests are easier to tune to than the Dragonfire game, since they're so much simpler and precise. Not that all of his tests are working properly now, but a significant fraction of them are.

@npwoods
Copy link
Contributor

npwoods commented Dec 16, 2023

Yes I noted the Steward Orchard tests in your video and I contemplated how much easier my life would have been when I was trying to get the cycle exact video to work (2011?). I suppose that I should be as happy as far as I did; prior to that work the 6x09 emulation did not have any pretentions of supporting cycle exact timing.

In any case I'm really eager to see this work get over the finish line, not just to get the video timing right, but also to get a handle on how this hardware really worked. As you can probably tell from the code, I never really knew what was going on with "PAL 6847" and the code reflected this lack of awareness. If in practice Dragons (and perhaps PAL CoCos) did in fact suspend the CPU clock to support PAL there is probably a canonical (or "least worst") way to express that behavior in MAME (@cuavas?). Related to this, its worth noting that we never attempted to emulate the 6883 SAM's "dual speed" mode.

You wondered about making the 6847 configurable. Are you talking about end-user configurable (i.e., .ini and command-line)? I don't think that kind of power would be useful or safe in the hands of end users, as there is a limited number of cases to cover from real machines.

Agreed, it probably does not make sense to do this. If everything that could possibly be tweaked in the INI files was there, we'd probably have thousands or millions of entries.

@cuavas
Copy link
Member

cuavas commented Dec 22, 2023

You wondered about making the 6847 configurable. Are you talking about end-user configurable (i.e., .ini and command-line)? I don't think that kind of power would be useful or safe in the hands of end users, as there is a limited number of cases to cover from real machines. If you're talking instead about extra constructor parameters to the 6847 class, that's quite sensible. But we'd need to know how PAL is implemented across the different Dragon & PAL CoCo variants to know what parts of the 6847 code to make extensible.

I mean at machine configuration time, not user configuration.

I still think the fake PAL devices are gross. To implement it more realistically, you’d probably need to decouple the 6847 device from the screen somewhat, so you could call the line drawing function on the lines when it’s being clocked.

At this point it’s not really any worse than before, so I guess it can go in.

@cuavas cuavas merged commit b0c924a into mamedev:master Dec 22, 2023
0 of 5 checks passed
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.

MC6847 VDG improperly sync'd on Dragon computers
4 participants