mame icon indicating copy to clipboard operation
mame copied to clipboard

heathkit/tlb.cpp: fix gp19 50-line text mode

Open mgarlanger opened this issue 1 year ago • 23 comments

Uncommented code in mc6845.cpp which fixes the heathkit gp19 device when operating in 50 line mode. I've made the uncommented code conditional requiring the system wanting to use it to set a flag since the existing comment mention that some systems did not function properly with it, and that was why it had been commented out.

This fixes both issues seen with 50 line mode:

  • only 25 lines being displayed on the screen
  • character descenders being cut-off

mgarlanger avatar Aug 24 '23 13:08 mgarlanger

To be honest, I don’t particularly like this. It gives two sets of “wrong” behaviour. What actually goes wrong with the systems that don’t work when this is enabled? Is it caused by the horizontal timing being completely screwed up by all lines being rendered every field? How do the counters work in interlaced mode?

My gut feel is that more adjustments need to be made to implement interlaced mode to make it look like the lines are being drawn at the correct time.

You can see my adventures implementing enough of interlaced mode to appease the OS on the Apple JMFB cards:

* https://github.com/mamedev/mame/blob/mame0257/src/devices/bus/nubus/nubus_48gc.cpp#L750

* https://github.com/mamedev/mame/blob/mame0257/src/devices/bus/nubus/nubus_48gc.cpp#L841

* https://github.com/mamedev/mame/blob/mame0257/src/devices/bus/nubus/nubus_48gc.cpp#L1003

In that case, the CRTC always counts in half-lines, so non-interlaced mode needs the counts from the screen_device adjusted.

I suspect the 6845 family will require counts, flags and interrupts to be adjusted for interlaced mode so that the system doesn’t get confused by seeing twice the horizontal frequency it expects.

I'm not sure what was wrong with those systems when it failed. The comment identifying the systems is from 2013 and this commit - https://github.com/mamedev/mame/commit/45fa51294da48244e0195bbd1d0eaad57079ee8c I would need to track down the ROMs to test the failed systems.

I'll take a look at those files you linked and see if I can make sense out of the changes. I'd be willing to help with whatever changes are needed in the 6845, but would likely need a lot of direction, since I don't know much about the 6845 and am still learning the ways of MAME.

This doubling of scan lines that Robbbert identified in his commit linked above, at least displays text properly on the GP-19, and it would be nice to get this merged until we can come up with a better implementation for the 6845.

I still need to track down why only the top half of the screen is displayed in graphics mode for the GP-19.

mgarlanger avatar Aug 24 '23 23:08 mgarlanger

Unlike interlace, which using half-lines, the interlace/video mode only uses integer line numbers, they are just spaced half the distance as regular sync lines as shown here. Screenshot 2023-08-27 at 1 13 13 PM

mgarlanger avatar Aug 27 '23 18:08 mgarlanger

It gives two sets of “wrong” behaviour.

I read over the MC6845 datasheet again, and I don't think the is the wrong behavior, but is just incomplete. Based on this text in the datasheet(page 12 - http://bitsavers.trailing-edge.com/components/motorola/_dataSheets/6845.pdf):

3. For interlace sync and video mode only, the number
(Nvd) programmed into the vertical display register (R6)
must be one half the actual number required.

So this doubling is correct/required, but is not the complete implementation. What's missing is only doing odd or even lines on each scan (note: for the "interlace sync and video" mode every line is an integer and doesn't need the half lines like the plain "interlace" mode requires).

I've tried changing the code to add the part of only doing even/odd halves each scan, but so far have ended up with either a black screen or mame hanging at 100% CPU usage. There must be something I'm missing related to the scan.

mgarlanger avatar Sep 03 '23 17:09 mgarlanger

I tested the Apricot PC and Victor9k with and without the code commented. The victor9k did not show anything on the screen with the code uncommented. The Apricot PC had this with the code commented: Screenshot 2023-09-04 at 6 33 09 PM With the code uncommented, it duplicated some of the graphics: Screenshot 2023-09-04 at 8 05 30 PM

mgarlanger avatar Sep 05 '23 02:09 mgarlanger

@cuavas After further investigation into this issue, the reason this change broke some systems, was that different versions/manufacturers of the chip implemented R6 in different ways. Some required the value written in R6 to be half the desired number of lines and others required it to be the actual number of lines. I could not find any note in the HD6845S datasheet about setting R6 to half the desired lines, as I had mention above, that it was in the MC6845 datasheet. And after further investigation, a Synertek manual - https://archive.org/details/Synertek-DataBook1983OCR/page/n323/mode/2up confirmed that different vendors chips implemented R6 in different ways.

Screenshot 2023-09-05 at 8 30 47 AM

I've updated this PR to reflect that the code to double the vertical lines is dependent on the chip, and set the values for the chips that I could find the datasheet for.

mgarlanger avatar Sep 05 '23 13:09 mgarlanger

If the Apricot PC and Victor 9K are good with this change I think it's doable unless Vas sees something.

rb6502 avatar Sep 08 '23 20:09 rb6502

This is still wrong. Whatever is attached to out_hsync_callback will get twice as many calls as it should in interlaced mode because it’s still assuming the screen device is always configured in terms of lines, not half-lines. It’s papering over a nasty hack.

To actually work, it needs to:

  • Always double the vertical resolution in interlaced mode.
  • Only update alternate lines in interlaced mode, on the correct half-line.
  • Adjust how the horizontal sync callback is generated in interlaced mode so it isn’t double the line rate.
  • Adjust the values read from registers so it reports lines or half-lines as appropriate.

cuavas avatar Sep 11 '23 19:09 cuavas

Vas, did you read what he wrote? Some 6845 variants are documented by the manufacturer to count in half-lines in interlace mode and some count in full lines. There is not a one-size-fits-all solution because the chips are different. This is getting rid of a hack now.

rb6502 avatar Sep 11 '23 21:09 rb6502

To clarify, "interlace" mode(0x01) is handled differently than "interlace and video" mode(0x03). With interlace(0x01), it does use half-lines. But with mode 3 - "interlace and video" it uses full lines, and alternates scan out of odd/even lines for each refresh(see the screenshot of the manual I posted above). The difference lies in what the chip expects to be programmed in the R6 "Vertical Displayed" register. The MC6845R (, HD6545R and SY6545-1) expect 1/2 of number of lines on the screen (the number that would be displayed every refresh). While other chips including the SY6545R, SY6545, HD6545S, and SY6545E) expect the full number of lines(even though half will be draw on each refresh). This is why the systems that use the HD6545S like the victor9k works. This just sets the screen in a consistent manner based on what the crtc is expecting.

Updating alternate lines(either odd/even for mode 3, or 1/2 lines for mode 1) is another piece of the puzzle that needs to be fixed. But even if that is fixed, the changes in this PR will still be needed so that the differences between chips is taken into account.

mgarlanger avatar Sep 11 '23 23:09 mgarlanger

This is still wrong. Whatever is attached to out_hsync_callback will get twice as many calls as it should in interlaced mode because it’s still assuming the screen device is always configured in terms of lines, not half-lines. It’s papering over a nasty hack.

For the "interlace and video" mode (0x03), it does use lines, not half-lines. They are whole number and it alternates odd/even lines when refreshing the screen. See the screen capture above to see how the manual shows how this mode is different that just plain "interlaced".

Yes, with my change, out_hsync_callback will have twice as many calls as it should, but that is already happening with the "working" systems - apricot PC and victor9k, because their CRTC is being programmed with double the number of lines that would be scanned on a refresh. They are using the "HD6845S", which expects R6 to be set to the total number of (character) lines. The original MC6845R is expecting it expecting R6 to be set to half the number of (character) lines. This is why only half the screen is being shown for the GP-19 when in 50-line mode - the rom is programming it to 25 lines per the requirements in the datasheet.

To actually work, it needs to:

* Always double the vertical resolution in interlaced mode.

This should not happen when in 'interlace and video' mode for the chips that expect to be programmed with the full number of lines in R6. It is only needed in the chips that expect R6 to be programmed with half the number lines to display.

Screenshot 2023-09-14 at 8 29 51 PM Screenshot 2023-09-14 at 8 30 23 PM
* Only update alternate lines in interlaced mode, on the correct half-line.

For "interlace mode" (0x01), that is needed, and needs to be done on half-lines. But for "interlace and video" mode(0x003), it needs to alternate odd/even lines.

* Adjust how the horizontal sync callback is generated in interlaced mode so it isn’t double the line rate.

This looks like it needs to be done, but this shouldn't be a blocking issue, and the systems using chips that expect the full number of lines in R6 like Apricot PC and Victor 9K are already at double the line rate without any major issues. And even if that change is made, the fix in the current PR still needs to be made.

* Adjust the values read from registers so it reports lines or half-lines as appropriate.

The R6 register is read-only, at least on the original MC6845. If some of the other chips allow reading, that would need to be looked at. And since R6 is the number of character lines, and the value being written is stored, I don't think any adjustment is needed.

mgarlanger avatar Sep 15 '23 01:09 mgarlanger

Sorry for the red herring about position readback, I forgot the MC6845 doesn’t have readable display counters besides the address outputs.

But, looking at this, I still can’t see it working properly for the basic MC6845.

There are three modes: normal, interlaced sync, and interlaced sync/video. In normal mode, things are simple – the vertical totals apply to the whole frame.

In the two interlaced modes, the vertical values are per-field. We can confirm this by looking at the values used to generate a PAL signal on a Commodore PET: R4=33, R5=7, R9=8. This produces ((33 + 1) × (8 + 1)) + 7 - 0.5 = 312.5 lines per field for a total of 625 lines per frame.

The difference between the interlaced modes is:

  • In interlaced sync mode, the row address is incremented by 1 each line.
  • In interlaced sync/video mode, the row address is incremented by 2 each line, and offset by 1 for odd fields.

Now MAME’s screen device does nothing to help you with interlaced modes. You have to set the screen’s refresh rate to the field rate, and set the total vertical size to the number of lines per frame (which will inevitably be an odd number). When you read the position back from the screen device, the Y coordinate will effectively be in half-lines, and the X coordinate will be doubled, and I believe offset by half a line during even fields.

So for this to work properly, you need to configure the screen device’s total lines per frame with double the programmed vertical total minus 1.

But now that the screen device is working in fields and half-lines rather than frames, you need to adjust how you update the screen because only half the lines should be updated each field. In mc6845_device::screen_update, you’ll need to track which field is being drawn, and only call draw_scanline for either odd or even lines. This is important, because whatever the driver configures for m_update_row_cb may have side effects.

Also, the code that sets up the timers will need to be checked and adjusted to fire DE and VSYNC at the right times taking half lines into account. Without doing this, the MC6845 device and screen device will drift out of sync, and you’ll get tearing.

cuavas avatar Oct 04 '23 14:10 cuavas

I agree that this change is not the complete fix, but this change is part of the fix. The only reason that Apricot PC and Victor 9k is showing full screen, is because it is using the HD6845S, which is expecting the total number of display lines to be programmed in R6, and that is what the ROM is programming it for. But the MC6845 expects half the number of lines to be programmed in R6, that is what the ROM is doing, and that is why only half the screen is being shown.

So for this to work properly, you need to configure the screen device’s total lines per frame with double the programmed vertical total minus 1.

As shown in the datasheet for Synertek, which I linked to above, only the chips that expect half the number of lines to be programmed should have the R6 value double. HD6845S can not have the lines doubled because it is already programmed for the full number of lines.

This PR is the first step, it provides the consistency for R6 whether the chip expects the total number of lines, or half the total number of lines to be programmed. Can this partial fix be merged now so that all versions of the 6845 behave the same, and I can work on addressing the other concerns in a later PR?

mgarlanger avatar Oct 05 '23 02:10 mgarlanger

I tried to have alternating odd/even lines scanout in mc6845, but had bad results. There are quite a few checks currently in the code for the interlace/video mode (0x03), and it is not clear which ones should be there and which ones are hacks to get the ApricotPC and Victor 9k working.

mgarlanger avatar Oct 05 '23 05:10 mgarlanger

I tried having mc6845_device::screen_update alternate odd/even lines and the screen is totally messed up. It appears the first line of text on the screen is drawn properly, but then the rest of the lines of text have the odd scan lines of the first line, plus the even scan lines from that line of text. Attached is a video showing the behavior. On the terminal I have abcdef on the first line and abcdefabcdef on the second line. No other characters were typed on the screen. The first line looks ok. On the second line, the first 6 characters look ok, because it is getting the odd scan lines from line 1 and the even scan lines from line 2. The next 6 characters only show the even scan lines since there is no data on the first line. Then the remaining text lines on the screen all show the odd scan lines from line 1.

https://github.com/mamedev/mame/assets/8291090/a16d5be6-8e42-423b-ae10-b19b7ddcfc7a

mgarlanger avatar Oct 07 '23 06:10 mgarlanger

But now that the screen device is working in fields and half-lines rather than frames, you need to adjust how you update the screen because only half the lines should be updated each field. In mc6845_device::screen_update, you’ll need to track which field is being drawn, and only call draw_scanline for either odd or even lines. This is important, because whatever the driver configures for m_update_row_cb may have side effects.

I tried that, and the video in the previous comment is what I see when only calling draw_scanline for either odd or even. Since that isn't working, I moved the check from screen_update into draw_scanline and have it not make the call to the call to m_update_row_cb(). This resulted in problems of the screen not erasing, both when trying to do a full screen erase, or overwriting characters with the space bar.

I then tried with setting DE to zero if it was a line that should not be draw during that frame. This seems to basically work. There is quite a bit of flickering, which actually is similar to what I see on the real H19 with the GP19 add-on when in 132x50 character mode. @cuavas let me know if this approach seems reasonable and you want me to push the change. Attached are videos for the H19 with gp-19, and the victor 9k. Surprisingly, I didn't see any change in the screen for the ApricotPC, it still seemed rock solid without any flickering.

https://github.com/mamedev/mame/assets/8291090/164780cb-f25d-4754-8268-bbafb87efa60

https://github.com/mamedev/mame/assets/8291090/1b4560e4-94bf-44f8-bc75-09aed8dcf222

mgarlanger avatar Oct 08 '23 03:10 mgarlanger

Created issue for 6845 interlace mode - https://github.com/mamedev/mame/issues/11644 let me know if more details are needed.

mgarlanger avatar Oct 20 '23 05:10 mgarlanger

@cuavas my latest commit only calls 'm_update_row_cb' on alternate updates to the display when in the "interlace and video" mode. Functionality seems to work for the H19 with the GP19 option, the Apricot PC, and the Victor 9k.

mgarlanger avatar Oct 22 '23 02:10 mgarlanger

@cuavas I am seeing tearing with this change. I looked at the DE and vsync timers, but couldn't figure out what they were doing, or how to adjusted them for the alternating lines, that you had mentioned in a previous comment (https://github.com/mamedev/mame/pull/11504#issuecomment-1747051947). I'd appreciate any guidance you could provide on how those need to be adjusted.

mgarlanger avatar Nov 12 '23 16:11 mgarlanger

To get the best possible results for this, and to make it less susceptible to issues caused by different parts of the device code getting out of sync, this is going to need some more invasive changes to the 6845 device code. I know that probably isn’t what you want to hear, but I’m confident we can get through this, and we’ll end up with multiple issues fixed all at once. That’s the beauty of MAME’s modular device architecture – you fix it properly once, and everything using the device benefits.

The most fundamental thing is that the current implementation relies on the timers not getting out of sync with the screen device if it’s configured to use one. This is fundamentally error-prone, and it’s liable to drift due to error accumulation and precision limitation effects with time and frequency values. When the device has a screen configured, it should be setting the timers in terms of screen().time_until_pos(...) to ensure that it’s always in sync. Similarly, it should update its counters in terms of screen().vpos() and screen.hpos() if it has a screen rather than relying on checking elapsed(). This is the most invasive change, but it will need to be done sooner or later.

However, it will still need to support setting timers based on the configured clock frequency alone since it’s supposed to work without a screen device as well. It might be best to get interlace support working in this configuration before reworking the code to set the timers off the screen device.

But getting to the immediate issue of why you’re seeing tearing, it’s because the timers are getting out of sync from the screen device due to not taking interlacing into consideration.

The most basic thing that’s broken is that you’re flipping m_odd_frame in screen_update. You need to remember that you don’t necessarily get a single screen update call per frame. The screen device can be configured to generate an update callback for every scan line (common for systems that frequently update video RAM during horizontal blanking), and a screen update can be triggered at any desired time by another device in the system (often done when software writes registers that affect drawing). On top of this, you need to remember that the 6845 device is supposed to work without a configured screen device, in which case it won’t get screen update callbacks at all. It’s probably best to update this flag in the line timer callback. (It should also be called m_odd_field, too.)

Further to that, you should only be calling draw_scanline on alternate screen lines in interlaced modes. It isn’t just the actual drawing callback that should only be called on alternate lines, all the effects of updating a line should only happen once per line, not per half line in interlaced mode. You need to move the checks for drawing alternate lines up from draw_scanline to screen_update.

You need to realise that the 6845 device’s current assumption that a vertical sync transition always coincides with the start of a line is not valid in interlaced modes. Look at the timing diagrams in figure 13 in the MC6845 datasheet (14th page). During an even field, the vertical sync transition is offset by half a line. You’ll need an additional timer to trigger the vertical sync transition at the correct position in the line during even fields.

You can also fix up the hacky adjustments for interlaced modes in handle_line_timer. In interlaced modes, you need to make it increment the raster counter by 2 on each line, and account the “extra” split line between even and odd fields (shown hatched in the diagram in the datasheet). Not accounting for this line is the current source of your severe drift between the 6845 and screen devices that results in the tearing you see.

You can check that your timers aren’t getting out of sync with the screen device if one is configured by logging screen().vpos() and screen().hpos() in handle_line_timer as well as the vertical counters (yes, this will generate a very large amount of logging and shouldn’t be enabled by default). You should see vpos() incrementing cleanly by 1 in non-interlaced modes and by 2 in interlaced modes, and hpos() should be consistent and not drift rapidly. If you get drift, you aren’t calculating the timings correctly.

cuavas avatar Nov 18 '23 18:11 cuavas

Thanks, I'll look at all that info, and try to make the appropriate adjustments.

mgarlanger avatar Nov 18 '23 20:11 mgarlanger

Changed this to a draft, since it is no longer even working.

mgarlanger avatar Nov 24 '23 21:11 mgarlanger

Seem to be getting closer. Added the half-line timer needed for vsync. Systems are no longer having hard-lockups. ApricotPC startup display looks good. Victor9000 is showing just minor corruption. But h19's gp19 still not displaying anything (but it is not a hard lockup as it was prior to the last commit.

mgarlanger avatar Dec 03 '23 08:12 mgarlanger

The call to screen().reset_origin(); is what is causing the hard-lockups, because it keeps reconfiguring the screen due to some mismatch, but I'm not sure how to address that.

mgarlanger avatar Dec 05 '23 01:12 mgarlanger