heathkit/tlb.cpp: Fix screensaver for superset slot device #11756
Conversation
There was a problem hiding this comment.
This needs more in-depth changes to the 6845 code.
When setting configuring the screen in mc6845_device::reconfigure_parameters, you need to:
- Check whether the configured vertical displayed (R6) or horizontal displayed (R1) is zero.
- If neither is zero, apply the -1 adjustment to the visible area before calling
screen().reconfigure(…)andm_reconfigure_cbas rectangle coordinates are inclusive. - If either the configured vertical displayed (R6) or horizontal displayed (R1) is zero, you should attempt to maintain the previously configured visible area. However if the vertical or horizontal total or vertical total has reduced, you may need to reduce the visible area so it doesn’t exceed the total area.
In mc6845_device::handle_line_timer, you need to check whether the configured horizontal displayed (R1) is zero, and if it is, don’t assert the DE output or schedule the DE off timer.
In mc6845_device::draw_scanline, you need to check whether the configured horizontal displayed (R1) is zero, and if it is, always set the update row’s de argument to zero.
src/devices/video/mc6845.cpp
Outdated
| if (postload || | ||
| if ((postload || | ||
| (horiz_pix_total != m_horiz_pix_total) || (vert_pix_total != m_vert_pix_total) || | ||
| (max_visible_x != m_max_visible_x) || (max_visible_y != m_max_visible_y) || | ||
| (visible_width != m_visible_width) || (visible_height != m_visible_height) || | ||
| (hsync_on_pos != m_hsync_on_pos) || (vsync_on_pos != m_vsync_on_pos) || | ||
| (hsync_off_pos != m_hsync_off_pos) || (vsync_off_pos != m_vsync_off_pos)) | ||
| (hsync_off_pos != m_hsync_off_pos) || (vsync_off_pos != m_vsync_off_pos)) && !maintain_visible_area) |
There was a problem hiding this comment.
You still want to reconfigure the screen if you’re maintaining the visible area, because the totals or sync positions could have changed.
src/devices/video/mc6845.cpp
Outdated
| if (maintain_visible_area) | ||
| { | ||
| if (visible_width > horiz_pix_total) | ||
| { | ||
| visible_width = horiz_pix_total; | ||
| maintain_visible_area = false; | ||
| } | ||
|
|
||
| if (visible_height > vert_pix_total) | ||
| { | ||
| visible_height = vert_pix_total; | ||
| maintain_visible_area = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
You shouldn’t need to clear maintain_visible_area when you clip this.
src/devices/video/mc6845.cpp
Outdated
| bool zero_horizontal_width = (m_horiz_disp == 0); | ||
| bool zero_vertical_height = (m_vert_disp == 0); | ||
| bool maintain_visible_area = zero_horizontal_width || zero_vertical_height; | ||
| uint8_t visual_adjustment = maintain_visible_area ? 0 : 1; |
There was a problem hiding this comment.
You’ll still need to inset the visible area rectangle if you’re maintaining the visible area since your width/height are exclusive while rectangle coordinates are inclusive – you don’t need visual_adjustment.
src/devices/video/mc6845.cpp
Outdated
| visarea.set(0 + m_visarea_adjust_min_x, max_visible_x + m_visarea_adjust_max_x, 0 + m_visarea_adjust_min_y, max_visible_y + m_visarea_adjust_max_y); | ||
| visarea.set(0 + m_visarea_adjust_min_x, visible_width + m_visarea_adjust_max_x, 0 + m_visarea_adjust_min_y, visible_height + m_visarea_adjust_max_y); |
There was a problem hiding this comment.
This is where you need to inset the visible area – it’s the rectangle right/bottom coordinates that are inclusive.
src/devices/video/mc6845.cpp
Outdated
| if (has_screen()) | ||
| screen().configure(horiz_pix_total, vert_pix_total, visarea, refresh.as_attoseconds()); | ||
| { | ||
| screen().configure(horiz_pix_total - visual_adjustment, vert_pix_total - visual_adjustment, visarea, refresh.as_attoseconds()); | ||
| } | ||
|
|
||
| if(!m_reconfigure_cb.isnull()) | ||
| m_reconfigure_cb(horiz_pix_total, vert_pix_total, visarea, refresh.as_attoseconds()); | ||
| { | ||
| m_reconfigure_cb(horiz_pix_total - visual_adjustment, vert_pix_total - visual_adjustment, visarea, refresh.as_attoseconds()); | ||
| } |
There was a problem hiding this comment.
You don’t inset the total width/height – those are counts, not inclusive coordinates.
src/devices/video/mc6845.cpp
Outdated
| uint16_t max_visible_x = m_horiz_disp * m_hpixels_per_column - 1; | ||
| uint16_t max_visible_y = m_vert_disp * video_char_height - 1; | ||
| uint16_t visible_width = m_horiz_disp * m_hpixels_per_column; | ||
| uint16_t visible_height = m_vert_disp * video_char_height; |
There was a problem hiding this comment.
If you’re maintaining the visible area, you want to start with m_visible_width and m_visible_height here to avoid getting a zero in the first place.
There was a problem hiding this comment.
OK, the logic is looking more-or-less sound at this point. However, given how widespread the 6845 family are, I’d like to hold off merging it until the start of the next release cycle (about a week from now).
Can you give a few 6845-based systems a basic check to see if they still seem to work before then?
src/devices/video/mc6845.cpp
Outdated
| visarea.set(0 + m_visarea_adjust_min_x, visible_width - 1 + m_visarea_adjust_max_x, 0 + m_visarea_adjust_min_y, | ||
| visible_height - 1 + m_visarea_adjust_max_y); |
There was a problem hiding this comment.
If you’re going split the line, please indent the continuation by two tabs, and group lines logically, e.g. put the X and Y coordinates on their own lines, like:
{
visarea.set(
0 + m_visarea_adjust_min_x, visible_width - 1 + m_visarea_adjust_max_x,
0 + m_visarea_adjust_min_y, visible_height - 1 + m_visarea_adjust_max_y);
}
src/devices/video/mc6845.cpp
Outdated
| uint16_t max_visible_x = m_horiz_disp * m_hpixels_per_column - 1; | ||
| uint16_t max_visible_y = m_vert_disp * video_char_height - 1; | ||
| uint16_t visible_width = zero_horizontal_width ? m_visible_width : m_horiz_disp * m_hpixels_per_column; | ||
| uint16_t visible_height = zero_vertical_height ? m_visible_height : m_vert_disp * video_char_height; |
There was a problem hiding this comment.
Please use parentheses for arguments to the conditional operator, like:
uint16_t visible_width = zero_horizontal_width ? m_visible_width : (m_horiz_disp * m_hpixels_per_column);
uint16_t visible_height = zero_vertical_height ? m_visible_height : (m_vert_disp * video_char_height);
src/devices/video/mc6845.cpp
Outdated
| set_de( m_line_enable_ff ? true : false ); | ||
| set_de( m_line_enable_ff && nonzero_horizontal_width ? true : false ); |
There was a problem hiding this comment.
Please use parentheses for arguments to the conditional operator, and since you’re editing the line anyway, change it to use “English” spacing around the partentheses, like:
set_de((m_line_enable_ff && nonzero_horizontal_width) ? true : false);| MC6845_BEGIN_UPDATE(heath_superset_tlb_device::crtc_begin_update) | ||
| { | ||
| bitmap.fill(rgb_t::black(), cliprect); | ||
| } |
There was a problem hiding this comment.
Is this actually necessary? Shouldn’t it get a call to heath_superset_tlb_device::crtc_update_row with de set to zero that will result in the rows being cleared if the screen saver is active?
There was a problem hiding this comment.
Without this, the screen is still not being erased. I'll try to determine why the update row code is not clearing, but right now the -log option to mame is not providing the expected logs.
There was a problem hiding this comment.
@cuavas The reason that heath_superset_tlb_device::crtc_update_row is not clearing it, is that the x_count being passed in is 0, since it uses the m_horiz_disp variable, which is R1 and that is being set to zero by the firmware.
Holding off until next cycle is fine. I've brought up the Super-80, ApricotPC, and Victor 9000, and they seem fine. |
|
This should be harmless, but now would be a good time to test a few systems with 6845-based CRTCs. |
|
A number of machines from the following drivers are showing exception or simply drop to command prompt when run after this commit. I've included all that I saw (there may be more in the drivers listed that I missed). Verified against a binary from this AM (23-12-23) as well as the action binaries for this commit and one prior to it: b128 - commodore/cbm2.cpp |
|
Is there specific software I need to track down to repo this? I tried to repo it with a recent branch (I happened to be on commit b816479). I ran b128 with 
|
|
Just tried with latest git. C:\MAME>mame b128 Exception at EIP=00007ff6ed9d48c0 ((anonymous namespace)::cbm2_state::crtc_update_row(bitmap_rgb32&, rectangle const&, unsigned short, unsigned char, unsigned short, unsigned char, signed char, int, int, int)+0x01d0): ACCESS VIOLATION
|
|
Strange, I just tried with the latest git, and not seeing any problems: I wonder if there is some difference between x86 and apple m1. |
|
It errors when it tried to write line 241 on the screen (y = 240, ma = 2400). I'm not familiar with this system, and the code in the machine config is far from clear, but I assume the screen should only have 240 lines. |
If you have a chance, could you try with changing this one line back so it initializes those variables to 0: https://github.com/mamedev/mame/pull/11756/files#diff-bd616883e4ae4ed2edf53526db4cd2c8f93ddbe1b5d8bea9abfe0e15856e0ba3R1101 and see if it still crashes? |
Problem still happens here. |
|
🤔 not sure how I'll be able to debug this since I can't repro on my mac and I don't have a wiindows dev machine. |
|
The key is the report "It errors when it tried to write line 241 on the screen (y = 240, ma = 2400)". Just add a printf or assert or whatever to that update function if y=240, because that means you're trashing memory off the end of the frame buffer. |
Added some mame/src/mame/commodore/cbm2.cpp Line 2521 in 8c32868 Lines 276 to 288 in 8c32868 |
|
Why I feel like this is very typical screen_device display blank setup, given that driver actually sets up 1142 x 261 later when it manages to initalize CRTC? |
|
The initialized CRTC is: so it line y=240 should still be fine(and it is on my Mac). I have no idea what is causing the Windows systems to crash. |
|
It's not fine lol, CRTC sets up the configuration while (very likely) the vpos beam is between 262~312, causing the crash. |
It's at least fine on the Mac 🙈 So would the mame/src/devices/video/mc6845.cpp Lines 512 to 515 in 8c32868 Do you think my PR just changed the timing to expose this existing bug? |
|
No, look again: M6845 config screen: HTOTAL: 1143 VTOTAL: 262 VIS_WIDTH: 720 VIS_HEIGHT: 200 HSYNC: 882-971 VSYNC: 224-239 Freq: 60.106990fps The callback is being told to draw visible content 40 scanlines after the end of the visible screen. So you are far into trashing memory at that point, and it's working on your system purely by accident. Memory trashing bugs can be very OS and compiler specific as to if they crash on a given build. Even single-driver vs full builds can be different on if what gets trashed will crash. |
I don't see that. I had it print y and de values for the first screen drawn after values became valid and they look fine. Here is the portion of the logs: |
|
You are not logging |
|
Some drivers have a line that says if de == 0 return; The cbm2 callback doesn't have this check, so perhaps it always worked by luck before. Because DE is 0, there should be no attempt to write to the screen. Just to be sure, you should check the schematic and make sure that the DE line inhibits output of data (sync pulses will still go through). |
It should still be ok to write when de=0. This is the approach Vas had suggested to address fixing an issue when line 25 was turned off, basically write the background color when de is off - mame/src/mame/heathkit/tlb.cpp Lines 440 to 466 in b8fa9a0 |
ok, I added screen().vpos() and I saw it consistently at 261 for all values of y, which should be ok, since the vtotal is 262 and if it is rendering the full screen at once, being at the max value seems fine. I did add |
|
Did the issue with |
|
Sorry I dropped the ball on this one. But I wasn't able to reproduce it. I didn't see any problems with the value of |
Checked the schematics and it shows no connection for the DE line 
|
…nfiguration. (mamedev#11756)"" This reverts commit 064d676.
…ion. (mamedev#11756)" Issues causing other systems to crash were never resolved. This reverts commit 5e1b719.
To enable screensaver, the superset ROM sets the horizontal characters displayed (R1 of the 6845) to 0.
The existing 6845 code would subtract 1 from this, causing the max_visible_x to be set to 0xffff. This value was considered invalid, and no further calls to update the screen were made. This caused the current text on the screen to be frozen during the screensaver, instead of cleared. If there is better way to handle this let me know. I had thought about on a valid -> invalid transition of the 6845 params, screen could be called to clear itself, but the PR change seemed more true to what the real device is probably doing.