Hi,
@@ -57,6 +58,7 @@ handle_10(struct bregs *regs) { debug_enter(regs, DEBUG_HDL_10); // don't do anything, since the VGA BIOS handles int10h requests
- sercon_10(regs);
}
Might as well remove handle_10 and call sercon_10 directly from romlayout.S.
Well, I expect this will change when adding support for parallel output to both vga and serial console.
+/****************************************************************
- serial console output
- ****************************************************************/
I think this code should go in a new c file and not modify serial.c.
Agree. Was thinking about that already as I saw the code grow ;)
[...]
+VARLOW u8 sercon_mode; +VARLOW u8 sercon_cols; +VARLOW u8 sercon_rows;
+VARLOW u8 sercon_row; +VARLOW u8 sercon_col;
I think the code should use the BDA equivalents of the above instead of declaring them in the private VARLOW space. Some old programs may rely on the equivalents in the BDA being updated.
Figured that meanwhile. syslinux is one example.
+VARLOW u8 sercon_cmap[8] = { '0', '4', '2', '6', '1', '5', '3', '7' };
For constant data (sercon_cmap) it would be preferable to use "static VAR16" (see kbd.c:scan_to_keycode[] and mouse.c:sample_rates[] as examples) and use GET_GLOBAL() instead of GET_LOW().
OK.
- SET_LOW(sercon_col_lazy, GET_LOW(sercon_col));
- for (pos = 0; pos < ARRAY_SIZE(sercon_char); pos++) {
SET_LOW(sercon_attr[pos], 0x07);SET_LOW(sercon_char[pos], 0x00);- }
+}
So, if I understand the above correctly, it's a buffer of recent updates used to reduce serial bandwidth (and unclutter serial logs) in the case where the application code issues a bunch of cursor moves / cell updates that are mostly redundant.
Yep. Typically happens for colored output. Apps use int10/09 to set character and attribute at the cursor position (but without moving the cursor). Then they move the cursor either using int10/02 (explicit set cursor position) or by printing the same character again using int10/0e (teletype, which prints character without updating attribute and moves the cursor forward).
+/* Read character and attribute at cursor position */ +static void sercon_1008(struct bregs *regs) +{
- regs->ah = 0x07;
- regs->bh = ' ';
+}
FYI, the sgabios code seems to indicate that sercon_1008() needs to be implemented for some programs to work properly. The sgabios code even implements a cache of recent writes to try to get it to work. It's ugly.
Didn't run into any issues yet, but also tested linux bootloaders only.
Maybe we can reuse the output buffer which we have anyway. Logic needs reworked a bit. We can't just clear characters after printing them out if we want be able to read them later, so we need a separate pending-updates bit. Also should be bigger I guess, maybe 80 chars so it can cover a complete line.
I'm finding it hard to understand how the "uncluttering" works in the above. I think it would be easier to understand if the vgabios interface code was separated from the "uncluttering" code.
In particular, I wonder if it would be simpler if the interface code was more similar to vgasrc/vgabios.c and it just translated requests to set_cursor_pos(cursorpos) and write_char(cursorpos, charattr) type calls. Then the write_char() code could check if the position was near previously written characters and buffer it if so - flushing if not. Thus the "uncluttering" could be mostly done in write_char(). The set_cursor_pos() implementation could be very lazy - it only needs to update the BDA with the new position. The sercon_check_event() code could send an explicit cursor move for the case where the cursor is idling at some position not after the last written character.
I'll have a look.
- default:
dprintf(1, "%s: ah 0x%02x, not implemented\n",__func__, regs->ah);There is a warn_unimplemented(regs) for this. Also, would be nice to implement a sercon_10XX(regs) to match other areas of the code.
Ok.
cheers, Gerd