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