heathkit/tlb.cpp: fix superset font issues #11652
Conversation
src/devices/video/mc6845.cpp
Outdated
| /* determine the visible area, avoid division by 0 */ | ||
| 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; | ||
| // properly handle horizonal display of 0 | ||
| uint16_t max_visible_x = m_horiz_disp ? m_horiz_disp * m_hpixels_per_column - 1 : 0; | ||
| uint16_t max_visible_y = m_vert_disp ? m_vert_disp * video_char_height - 1 : 0; |
There was a problem hiding this comment.
- Please restore the comment describing what this actually does, i.e. determine the visible area.
- Please put parentheses around the true expression in the ternary conditional operator – it makes it easier to interpret when reading the code.
Also, this will result in displaying a single column of pixels when m_horiz_disp is zero, and a single row of pixels when m_vertical_disp is zero (max_visible_x and max_visible_y are inclusive). Is that the intended behaviour?
Instinctively it feels wrong, as the 6845 works in character cells for the displayed area, so a special case where it displays a single column or row of pixels doesn’t seem right.
There was a problem hiding this comment.
Ideally, it would be 0 pixels, but since the variable is unsigned, having a max value of 0 is the lowest it can be set to. With the prior calculation if m_vert_disp is 0, the max becomes 0xffff.
This causes the check on line 453 to consider that the crtc params are invalid, and will not update the screen. So when the ROM sets it to 0, to trigger the screen saver, the screen is frozen, instead of being cleared. If there is a better way to handle this, let me know.
There was a problem hiding this comment.
You’re going to need to rework the code so that max_visible_x, max_visible_y, m_max_visible_x and m_max_visible_y are exclusive rather than inclusive (probably a good idea to rename them to something like visible_width/visible_height as the semantics are changing). That will allow it to deal with the zero width/height.
I don’t think changing the screen device’s visible area to a single row/column is a great idea, either. The system is only going to do this as a “blank screen” trick, and it will most likely program the same visible area again afterwards.
I think it would be better to detect zero width or height, and when it happens, try to leave the visible area unchanged if possible (you may need to limit it to the total width/height if they’re reduced while zero width and/or height is set). That would give a better user experience.
There was a problem hiding this comment.
Variables have been updated. I didn't see any user experience issues when the screen saver activated with the current code.
Since the calculation is no longer setting the variable values to 0xffff, which had previously preventing calculating the resolution at startup based on the class initialized values. I had to set m_horiz_char_total and m_vert_char_total to be less than m_horiz_disp and m_vert_disp to prevent the startup resolution screen to show bad values.
src/devices/video/mc6845.cpp
Outdated
| m_horiz_disp = m_vert_disp = 0; | ||
| m_horiz_disp = m_vert_disp = 0xff; |
There was a problem hiding this comment.
Is the intention of this to make it display something in the absence of any register writes?
At any rate, the vertical lines displayed is a 7-bit register on the MC6845 (table 2, about the 11th page, also see that it masks data in mc6845_device::register_w), so assigning it 0xff doesn’t make sense.
There was a problem hiding this comment.
This was needed, because with the new calculation at start up, the systems using the 6845 reports a bad screen resolution.
There was a problem hiding this comment.
This change was actually needed, so that the check on line 453 would consider the params invalid and not try to update screen, or calculate the resolution.
src/mame/heathkit/tlb.cpp
Outdated
| uint8_t heath_superset_tlb_device::crtc_reg_r(offs_t reg) | ||
| { | ||
| m_reverse_video_disabled = bool(BIT(reg, 3)); | ||
|
|
||
| return heath_tlb_device::crtc_reg_r(reg); | ||
| } |
There was a problem hiding this comment.
Since this is a read handler, you need to check machine().side_effects_disabled() before doing things with side effects, or it will make it impossible to debug.
There was a problem hiding this comment.
Updated, but I'm a little confused about this. I also updated the other read that had side effects. But if this prevents the code from running, the system is not going to be accurate.
There was a problem hiding this comment.
The idea is that you don't want anything that changes the machine state to happen due to the debugger's reads rather than the CPU's. For instance, on systems with read-to-trigger I/O locations (the Apple II is a classic example) I don't want to have opening a debugger memory view window change the video mode or click the speaker, and the side_effets_disabled() test prevents that while still allowing the system to run normally.
src/mame/heathkit/tlb.cpp
Outdated
| m_selected_char_set = (m_selected_char_set & 0x06) | (data & 0x01); | ||
| m_selected_char_set = (m_selected_char_set & 0x0A) | (data & 0x01); |
There was a problem hiding this comment.
Please use lowercase hex digits.
Fixes several issue with font selection when using the Superset ROM