Meaty Skeleton terminal_scroll bug

[dengeltheyounger]

Hey, this is my first time on OSDev Wiki. I have been going through the meaty skeleton, and I noticed something odd about the terminal_scroll.

Basically, the for loop is structured in this manner:

Code: Select all

for(loop = line * (VGA_WIDTH * 2) + 0xB8000; loop < VGA_WIDTH * 2; loop++)

It appears that this for loop will never exit since loop is incremented rather than decremented.

I also noticed this in terminal_putchar:

Code: Select all

for(line = 1; line <= VGA_HEIGHT - 1; line++)
{
				terminal_scroll(line);
}

It looks as though terminal scroll is designed to take the current line and then copy it into the previous line.

Would this be a better fix for terminal_scroll based on the behavior of terminal_putchar?

Code: Select all

// Either this for a sanity check
// if (line == 0) ++line;
// Or this
// if (line == 0) return;
for (loop = line * (VGA_WIDTH*2) + 0xB8000; loop < (line-1)*(VGA_WIDTH*2) + 0xB8000; line++) { blah }

Going through the OSDev Wiki is my first foray into the inner workings of Operating Systems, and so I wanted to make sure that I had this all right.

[Zoarial94]

Welcome to the community! While it looks like there’s an error, I think your reasoning is off.

It appears that this for loop will never exit since loop is incremented rather than decremented.

From what it looks like, the for loop will never start. The variable loop essentially becomes a pointer off in memory (an absolute), but is then compared to (VGA_WIDTH * 2) as though it’s relative. This means loop will always be bigger. It looks like you caught on to this and your solution is almost right.

Instead of subtracting one from line, add one so that the loop can traverse the whole line and stop at the start of the next. By subtracting one, you have the same issue of never starting the loop.

I’m also new to the community and I haven’t spend too much time on my VGA console yet, but the Meaty Skeleton example feels kind of klunky. Maybe that’s by design to make it easier to understand, or maybe it needs a make-over. Either way, good catch.

[dengeltheyounger]

I actually did figure out what you mentioned. Although, that doesn't seem to be working either.

This is the body of the for loop:

Code: Select all

 
void *loop;
for (loop = (void *) (line*(VGA_WIDTH * 2)) + 0xB8000;
                loop > (void *) ((line+1) * VGA_WIDTH * 2) + 0xB8000; loop++) {
                c = *(char *) loop;
                *(char *) (loop - (VGA_WIDTH * 2)) = c;
        }
}

I did the cast to void star in order avoid compiler warnings. Regardless, it doesn't actually scroll anything.

Tomorrow, I'll look at it a bit more to see what I can do.

[nullplan]

Oh god. So much work just to avoid calling memmove()... Wait, that code is not even a memmove(), it's a memcpy(). And all just because the author failed to write C and wrote assembler instead. Here's how I would do it:

Code: Select all

#define SCREEN (uint16_t*)0xB8000
void terminal_scroll(int line) {
  memcpy(SCREEN + (line - 1) * VGA_WIDTH, SCREEN + line * VGA_WIDTH, VGA_WIDTH * sizeof(uint16_t));
}

There, that is it. That is what that code is trying to do: Copy the given line into the previous one. Done. No bloody loop, no bloody pointer casts, no assembler disguised as C. Just using the language somewhat as it was intended.

You could even model the screen as a two-dimensional array and let the compiler do the dirty work.

Code: Select all

typedef uint16_t screen_t[VGA_HEIGHT][VGA_WIDTH];
#define SCREEN (*(screen_t*)0xB8000)
void terminal_scroll(int line) {
  memcpy(SCREEN[line - 1], SCREEN[line], sizeof (SCREEN[0]));
}

The function is still terrible, since if it is called with an out-of-bounds argument (and 0 counts as out-of-bounds), it will fail horribly. It is only used in a single place, and there it is used to scroll the whole screen. So this could just be replaced with

Code: Select all

typedef uint16_t screen_t[VGA_HEIGHT][VGA_WIDTH];
#define SCREEN (*(screen_t*)0xB8000)
void terminal_scroll_whole_screen(void) {
  memmove(&SCREEN[0][0], &SCREEN[1][0], (VGA_HEIGHT - 1) * VGA_WIDTH * sizeof (uint16_t));
  memset(SCREEN[VGA_HEIGHT - 1], 0, sizeof (SCREEN[0]));
}

Now only someone needs to test this code I just came up with.

[PeterX]

Maybe it's my fault. That code which uses pointers directly is from me. (I was thinking in Assembler terms when I wrote those lines, that's why.)

The code which uses arrays and memcpy() is not from me.

Anyway it's good that so many eyes look at the code. So bugs are noticed! Thanks for finding this bug.

Greetings
Peter

[dengeltheyounger]

These don't seem to be working either.

Originally, I tried these two:

Code: Select all

memcpy(0xB8000 + (line-1)*(VGA_WIDTH*2), 0xB8000 + line*(VGA_WIDTH*2), sizeof(uint16_t)*VGA_WIDTH*2)

Code: Select all

memcpy(0xB8000 + (line-1)*(VGA_WIDTH), 0xB8000 + line*(VGA_WIDTH), sizeof(uint16_t)*VGA_WIDTH)

Neither of those resulted in scrolling.

I also tried this:

Code: Select all

void terminal_scroll(size_t line) {
        if (line < 1)
                return;

        typedef uint16_t screen_t[VGA_HEIGHT][VGA_WIDTH];
        #define SCREEN  (*(screen_t *)0xB8000)

        memcpy(SCREEN[line-1], SCREEN[line], sizeof(SCREEN[0]));
}

This didn't work either.

This is the terminal putchar function:

Code: Select all

void terminal_putchar(char c) {
        size_t line;
        unsigned char uc = c;

        if (c == '\n') {
                ++terminal_row;
                terminal_column = 0;
                return;
        }

        terminal_putentryat(uc, terminal_color, terminal_column, terminal_row);

        if (++terminal_column == VGA_WIDTH) {
                terminal_column = 0;

                if (++terminal_row == VGA_HEIGHT) {
                        for (line = 1; line <= VGA_HEIGHT - 1; ++line)
                                terminal_scroll(line);
                        terminal_delete_last_line();
                        terminal_row = VGA_HEIGHT - 1;
                }
        }
}

The issue seems to be with terminal_scroll (as opposed to some other function).

[dengeltheyounger]

So, I managed to get it mostly working.

This is my code for terminal_putchar:

Code: Select all

void terminal_putchar(char c) {
        unsigned char uc = c;

        if (c == '\n') {
                ++terminal_row;
                terminal_column = 0;
                return;
        }

        terminal_putentryat(uc, terminal_color, terminal_column, terminal_row);
        terminal_column++;

        if (terminal_column == VGA_WIDTH) {
                terminal_column = 0;
                terminal_row++;
        }

        if (terminal_row == VGA_HEIGHT) {
                terminal_scroll();
                terminal_row = VGA_HEIGHT - 1;
        }
}

This is my code for terminal_scroll:

Code: Select all

void terminal_scroll() {
        // Copy next line into previous line
        
        for (int i = 0; VGA_HEIGHT-1; ++i) {
                memmove(&terminal_buffer[i*VGA_WIDTH], 
                        &terminal_buffer[(i+1)*VGA_WIDTH], VGA_WIDTH);
        }
}

Will there be any unexpected bugs using this implementation? Also why does that h remain as seen in the picture? I was also testing an implementation of printf that accounted for integers.

[Octocontrabass]

I don't see why you're still trying to use a loop.

Code: Select all

memmove( &terminal_buffer[0], &terminal_buffer[VGA_WIDTH], sizeof(uint16_t) * VGA_WIDTH * (VGA_HEIGHT - 1) );
memset( &terminal_buffer[VGA_WIDTH * (VGA_HEIGHT - 1)], 0, sizeof(uint16_t) * VGA_WIDTH );

[dengeltheyounger]

Mainly because I'm new at this, and I'm still getting the hang of treating the entire VGA buffer as being like one long single dimensional array.

So, I did try that, and it only seems to delete the h. Why is it doing this? The code seems to be perfectly fine.

[Octocontrabass]

QEMU had (and maybe still has) an odd quirk where using HLT with interrupts disabled to halt the CPU also prevents the screen from showing the most recent updates. Try adding an infinite loop to the end of kernel_main() so it won't return to the HLT in the startup code.

[dengeltheyounger]

QEMU had (and maybe still has) an odd quirk where using HLT with interrupts disabled to halt the CPU also prevents the screen from showing the most recent updates. Try adding an infinite loop to the end of kernel_main() so it won't return to the HLT in the startup code.

That didn't help. The same problem is still there. I tried to make another print after scrolling (so that a second scroll occurred). Could it be that there's some quirk with QEMU that is causing this? Maybe there's a more suitable emulator? I doubt this is relevant, but this is the command that I'm running:

Code: Select all

qemu-system-i386 -kernel path/to/myos.kernel

I couldn't get qemu to run the grub iso from the meaty skeleton for some reason.

[Octocontrabass]

Code: Select all

        if (c == '\n') {
                ++terminal_row;
                terminal_column = 0;
                return;
        }

You return instead of handling scrolling, so the first character after a line break is printed beyond the end of the display.

I couldn't get qemu to run the grub iso from the meaty skeleton for some reason.

Do you have the "grub-pc-bin" package installed? The wiki page isn't very clear, but that package is required if you want the image to boot with QEMU's default BIOS.

[dengeltheyounger]

I changed the putchar function, but it did not seem to make any difference.

Code: Select all

void terminal_putchar(char c) {
        unsigned char uc = c;

        if (c == '\n') {
                ++terminal_row;
                terminal_column = 0;
                return;
        }
        // We don't want to actually print the newline
        else {
                terminal_putentryat(uc, terminal_color, 
                                        terminal_column,
                                        terminal_row);
                terminal_column++;
        }

        if (terminal_column == VGA_WIDTH) {
                terminal_column = 0;
                terminal_row++;
        }

        if (terminal_row == VGA_HEIGHT) {
                terminal_scroll();
                terminal_row = VGA_HEIGHT - 1;
        }
}

I have GRUB installed with the qemu use flag (I use Gentoo), in addition to xorriso as mentioned by the OSDev wiki.

[dengeltheyounger]

So, turns out I had somehow forgotten to remove the return statement... Now that I have removed it, all of it works. Too bad there is no emoji for facepalming.

[nullplan]

Too bad there is no emoji for facepalming.

The commonly accepted one is: m(