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

Add new machine Tandberg TDV-2115L with accurate addressing in Display Logic driver #11843

Merged
merged 11 commits into from
Jan 2, 2024

Conversation

Frodevan
Copy link
Contributor

@Frodevan Frodevan commented Dec 16, 2023

The Tandberg TDV-2115L is a terminal based on the Tandberg Display Logic module, and is the most basic member of the TDV-2100 series of terminals and computer products from Tandberg.

Background: I recently dumped the addressing PROMs on the Tandberg TDV-2100 series Display Logic module, and the aim of this PR is to use this to add proper emulation of the vram addressing. As a result, a feature which was previously simulated (page roll), is now emulated the same way it is operating in hardware.

In addition to this, due to the more robust vram addressing, the conditional checks for the cursor have been changed to work like the real hardware.

Summary:

  • Add vram address-encode PROM
  • Properly emulate a roll-counter used for page up/down
  • Increase accuracy of cursor positioning

The two additional ROMs can be found here:
https://github.com/Frodevan/Tandberg/tree/main/TDV-2100/ROM-Files/Loose%20cards/960310%201-19%20Display%202

@cuavas
Copy link
Member

cuavas commented Dec 16, 2023

You may as well close the older PR since all the work in it is in this PR anyway. It will be less work to deal with it as a single unit.

src/mame/tandberg/tdv2100_kbd.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_kbd.cpp Show resolved Hide resolved
src/mame/tandberg/tdv2100_kbd.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.cpp Outdated Show resolved Hide resolved
src/mame/tandberg/tdv2100_disp_logic.h Outdated Show resolved Hide resolved

// LF, Move cursor to next line
case 0x0a:
if((m_cursor_row&0x18) == 24 && auto_roll_up)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be clearer if you keep it in hex when doing comparisons, and most of the code has more white space which seems more readable to me. if (((m_cursor_row & 0x18) == 0x18) && auto_roll_up)

Copy link
Contributor Author

@Frodevan Frodevan Dec 16, 2023

Choose a reason for hiding this comment

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

Given what this logic is responsible of, I thought it would be more readable to have it the way I did. 0x18 masks which lines are used for the comparison, so it's useful to see the individual bits used. 24 is the row-number the comparison targets, and this describes a parametrical value. Of course the target uses all the lines of the mask by nature, but it can be useful to be able to directly read out which row is targeted without having to convert from hex.

@Frodevan Frodevan changed the title Accurate addressing for Tandberg TDV-2100 series Display Logic driver Add TDV-2100 with Accurate addressing in Display Logic driver Dec 18, 2023
@Frodevan Frodevan changed the title Add TDV-2100 with Accurate addressing in Display Logic driver Add new machine Tandberg TDV-2115L with Accurate addressing in Display Logic driver Dec 18, 2023
@Frodevan Frodevan changed the title Add new machine Tandberg TDV-2115L with Accurate addressing in Display Logic driver Add new machine Tandberg TDV-2115L with accurate addressing in Display Logic driver Dec 18, 2023
@Frodevan
Copy link
Contributor Author

All review comments have now been addressed, so please mark any conversation as resolved if sufficiently fixed. Otherwise, please let me know so I can continue working on this.

Comment on lines +344 to +345
m_write_kstr_cb(0x80|m_char_buffer);
m_write_kstr_cb(m_char_buffer);
Copy link
Member

Choose a reason for hiding this comment

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

Is there potential for issues to be caused by the lack of any time between these two calls? How long would the top bit be asserted for in reality?

Copy link
Contributor Author

@Frodevan Frodevan Dec 20, 2023

Choose a reason for hiding this comment

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

On real hardware this line is driven by the Any-Key-Down signal from the AY-3-4592, and eventually gated with the key-inhibit signal and the key-repeat logic (74LS123 trigger arming a 555). The AY-3-4592 will also internally gate this "for 2 clock cycles" if a key is pressed when any other keys are being held.

This strobe goes straight to the clock pin of an edge-triggered D-flipflop. The output of this flip-flop feeds logic that schedules a write of the keyboard data to RAM. This scheduling is done to avoid any potential conflict with Rx on the UART or eventually RAM access by the the CPU in CPU-mode, and also to hold any access if the next row of characters is buffered for the display logic. All of that synchronization is not important here since RAM access is atomic and instantaneous.

The display logic module here still implements the edge-trigger, and due to this the timing is not important as long as the msb trigger strobe returns to 0 before the next keystroke.


PORT_START("X12")
PORT_BIT(0x01, IP_ACTIVE_HIGH, IPT_KEYBOARD) PORT_NAME("NORM") PORT_CODE(KEYCODE_RCONTROL) PORT_CHAR(UCHAR_MAMEKEY(RCONTROL))
PORT_BIT(0x02, IP_ACTIVE_HIGH, IPT_KEYBOARD) PORT_CODE(KEYCODE_3) PORT_CHAR('3') PORT_CHAR(0x00a3)
Copy link
Member

Choose a reason for hiding this comment

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

Please put the character that’s specified by codepoint in a comment to make it clearer, since it isn’t anywhere else on the line. (I think it’s a pound currency symbol £? But normal people don’t memorise codepoints.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

devcb_write_line m_write_transk_cb;
devcb_write_line m_write_break_cb;

int m_column_counter;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is registered for save states, it should be an explicitly sized integer type for portability (e.g. uint8_t if it’s big enough).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 110 to 115
int m_frame_counter;
bool m_video_enable;
bool m_vblank_state;
int m_cursor_row;
int m_cursor_col;
int m_page_roll;
Copy link
Member

Choose a reason for hiding this comment

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

Please use explicitly sized integer types for data members that need to be registered for save states for portability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 609 to 613
for(int dot = 0; dot < 8; dot++)
{
bitmap.pix(chr_y_pos + line, chr_x_pos + dot) = palette[BIT(data, 7 - dot)<<extra_intensity];
}
bitmap.pix(chr_y_pos + line, chr_x_pos + 8) = palette[BIT(dot_nr8, 0)<<extra_intensity];
Copy link
Member

Choose a reason for hiding this comment

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

You’re better off avoiding the repeated calls to bitmap.pix here – the compiler may or may not optimise away the repeated line address calculation. You can lift the invariant with something like:

				auto *pix = &bitmap.pix(chr_y_pos + line, chr_x_pos);
				for(int dot = 0; dot < 8; dot++)
				{
					*pix++ = palette[BIT(data, 7 - dot)<<extra_intensity];
				}
				*pix = palette[BIT(dot_nr8, 0)<<extra_intensity];

Alternatively, you could index the pixel pointer rather than incrementing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, although that sort of optimization does sound a little extreme to me. Then again, I am no expert about the inner workings of C++ compilers.

Comment on lines 869 to 872
void tandberg_tdv2100_disp_logic_device::break_w(int state)
{
m_break_key_held = state;
}
Copy link
Member

Choose a reason for hiding this comment

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

If the break key generates a break condition on the RS232 TxD output, shouldn’t this immediately call m_rs232->write_txd(0) if the break input is asserted, and restore the output from the AY51013 if it’s deasserted?

With the way you’ve got it now, no break condition will be generated in response to the break key if the AY51013 doesn’t shift anything out. Similarly, if the break key is released while a break condition is being generated, it won’t end until the AY51013 shifts something out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed now.

Comment on lines +489 to +490
uint32_t tandberg_tdv2100_disp_logic_device::screen_update(screen_device &screen, bitmap_rgb32 &bitmap, const rectangle &cliprect)
{
Copy link
Member

Choose a reason for hiding this comment

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

For what it’s worth, because of the way this function always draws entire characters, it won’t work properly if something triggers a partial update on a screen line that isn’t the first row of a character. The previous rows of the first line of characters intersecting the clipping rectangle will get redrawn.

If you do want to cater for cases like this, do you know how the display logic deals with attributes for each row of a line of characters? For example does it latch the attributes when it starts drawing the first row of a line of characters, then reuse that value when it starts each subsequent row of the line? Or does it have an line-sized attribute buffer it fills on the first row, or does it use some other approach?

Copy link
Contributor Author

@Frodevan Frodevan Dec 20, 2023

Choose a reason for hiding this comment

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

The attribute is latched anywhere through the scan, and reset only on VSync (not per line). The attribute is latched at an character defining an attribute change. In the original hardware, the current attribute at the end of a row is backed up as a checkpoint when moving to the next row, to restore it at each line-start so that the attributes are consistent for all lines in the row. Here I get around that by drawing whole characters one at a time.

I have already accounted for variable drawing areas since each time screen_update is called, I scan from upper left (0,0) to get the attribute for the first character in the rectangle section before any drawing is done. The attribute variable is in other words not shared between individual calls to screen_update. In this way, overlapping draws will not mess up the attribute.

Copy link
Member

@cuavas cuavas Dec 20, 2023

Choose a reason for hiding this comment

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

You’re missing the point. You’re drawing all the rows of pixels in a character in one go before moving to the next character horizontally.

It isn’t possible for the hardware to do this since the raster display scans an entire line of pixels before moving to the next line. After drawing the first row of pixels in a character, it draws the first row of pixels in the next character to the right, and so on. Then on the next line it draws the second row of pixels for each character in the same line of characters, and so on.

For this to work, it needs to be able to restore the attribute state for the beginning of the line of characters each time it starts drawing a line of pixels that doesn’t correspond to the first row of pixels in a line of characters. Usually this is done either by latching the attribute state at the beginning of screen line corresponding to the first row of pixels in a line of characters, then restoring that state at the start of subsequent rows of pixels for the same line of characters. However, other approaches are also possible, such as keeping a line width attribute buffer and filling it while drawing the first row of pixels in a line of characters.

So I have already accounted for this since each time screen_update is called, I scan from upper left (0,0) to get the attribute for the first character in the rectangle section before any drawing is done.

Think about what happens when video RAM or registers change part way through a frame. This is what causes visual artefacts like “tearing”. The portion of the raster that has already been drawn will be unaffected, but the part that has yet to be drawn will use the new values (assuming the RAM/registers aren’t double-buffered).

In MAME, we deal with this using partial updates. When something that affects video display changes, you trigger a partial update before setting the values so the parts of the screen that would be drawn before the change takes effect won’t see the change.

Consider what would happen on a partial update with your code as written now:

  • The code draws all rows of any line of characters that intersects the clipping rectangle. Supposed a partial update is triggered after three rows of a line of characters has been drawn. With your current implementation, those three rows will be redrawn when the remaining rows of that line of characters are drawn.
  • On every update, you scan attributes from the start of video RAM. If attribute characters are changed in the part of the frame that has already been drawn in a partial update, they will still be applied in subsequent updates for the same frame.

Copy link
Contributor Author

@Frodevan Frodevan Dec 21, 2023

Choose a reason for hiding this comment

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

For this to work, it needs to be able to restore the attribute state for the beginning of the line of characters each time it starts drawing a line of pixels that doesn’t correspond to the first row of pixels in a line of characters. Usually this is done either by latching the attribute state at the beginning of screen line corresponding to the first row of pixels in a line of characters, then restoring that state at the start of subsequent rows of pixels for the same line of characters.

Sorry for not going in too much detail, but this is exactly what the hardware does. The attribute is latched in a backup register at the end of the last line of each row of characters, and recovered at the start of each line in the next row. See "Attribute Memory/Latch" on pdf page 33/35:
https://www.technikum29.de/de/geraete/Siemens_6610/Siemens_6610_PDFs/Siemens_6610_Wartungskurs_Reg_I_Kap_0_Display_Logic_I_und_II.pdf

Think about what happens when video RAM or registers change part way through a frame. This is what causes visual artefacts like “tearing”. The portion of the raster that has already been drawn will be unaffected, but the part that has yet to be drawn will use the new values (assuming the RAM/registers aren’t double-buffered).

The hardware double-buffers display RAM one character row at a time, and halts any other access when the buffer is swapped out (also happens during last line of a row of characters). See "Refresh Memory", page 21/23 in the pdf linked to.

On every update, you scan attributes from the start of video RAM. If attribute characters are changed in the part of the frame that has already been drawn in a partial update, they will still be applied in subsequent updates for the same frame.

I don't see any good way around this, unless I have some guarantee drawing will always be fully sequential and will never skip any part of the screen (even across multiple partial updates).

Never mind, after sleeping on it I came up with an idea that might work.

Copy link
Contributor Author

@Frodevan Frodevan Dec 21, 2023

Choose a reason for hiding this comment

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

Ok, I have made it so that it initially only scans attribute between the end of the previous draw rectangle and the start of the current draw rectangle.

Comment on lines 632 to 644
void tandberg_tdv2100_disp_logic_device::rs232_rxd_w(int state)
{
if(!(m_data_terminal_ready && m_rx_handshake))
{
state = 1;
}
bool ext_echo = BIT(m_sw_rs232_settings->read(), 3);
if(!(ext_echo && m_data_terminal_ready))
{
state = (state && m_uart->so_r());
}
m_uart->write_si(state);
}
Copy link
Member

Choose a reason for hiding this comment

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

If m_data_terminal_ready or m_rx_handshake changes state or the external echo switch is flipped, shouldn’t that AY51013’s SI input be update immediately rather than waiting for the next state change of the external RxD input?

You’ll need to keep the input state so you can update the AY51013’s SI input when the other things that affect it change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the logic in the RS232 handling has now been streamlined, to not duplicate the same responsibility across multiple functions. This should be sorted out by this change.

Comment on lines 681 to 684
if(!(ext_echo && m_data_terminal_ready))
{
m_uart->write_si(state);
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t match with the logic in rs232_rxd_w. In that function, you’re setting SI low if either the SO output is low and internal echo is enabled, or the RxD input is low. This will set SI high if the SO output is low and internal echo is enabled even if the RxD input is low at that time.

Copy link
Contributor Author

@Frodevan Frodevan Dec 20, 2023

Choose a reason for hiding this comment

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

The RxD and SO+Echo are AND'ed together for SI, but this particular condition was not originally considered because this case is only valid if data-collision happens, and you are likely to get a framing error in both implementations cases anyways.

But, I can add it.

edit
Fixed with the fix for the other RS-232 issue.

@Frodevan
Copy link
Contributor Author

@cuavas With the previous PRs I should have sorted out the RS-232 discrepancies, and altered the attribute scanning to make it consistently sequential no matter when or where a portion of the screen is drawn.

For the screen drawing algorithm, I responded with a detailed description of the hardware logic, and I would argue that the discrepancies is so minimal that it is negligible. The consequence of implementing this exactly as-is in the hardware is a lot of complications to the code, and the only differences in functionality would be at worst a 1-frame extra delay before a change would be visible. Other differences would be very minor conditional delays for certain operations, to handle collisions with the swap of the display buffer. Nothing that incorporated this module depended on that level of timing-accuracy.

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.

OK, I think this looks like most things are covered. It could probably do with named constants for config port values, a few int changed to more appropriate types. But I think it can be merged now.

@cuavas cuavas merged commit 16d2bfe into mamedev:master Jan 2, 2024
5 checks passed
einstein95 pushed a commit to einstein95/mame that referenced this pull request Mar 2, 2024
* tandberg/tdv2100_disp_logic.cpp: Encapsulated TDV-2100 series terminal display logic with accurate addressing.
* tandberg/tdv2100_kbd.cpp: Added TDV-2100 series keyboard simulation.

New working machines
-----------------------
Tandberg TDV-2115L
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.

None yet

5 participants