OSDev.org

The Place to Start for Operating System Developers
It is currently Mon Mar 18, 2024 10:25 pm

All times are UTC - 6 hours




Post new topic Reply to topic  [ 16 posts ]  Go to page 1, 2  Next
Author Message
 Post subject: Meaty Skeleton terminal_scroll bug
PostPosted: Wed Feb 24, 2021 9:02 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
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:
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:
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:
// 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.


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Fri Feb 26, 2021 7:21 am 
Offline

Joined: Tue Feb 23, 2021 10:25 pm
Posts: 2
Welcome to the community! While it looks like there’s an error, I think your reasoning is off.

Quote:
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.

_________________
https://git.tcp.direct/Zoarial94/ZoarialBareOS


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 3:52 am 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
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:

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.


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 4:22 am 
Offline
Member
Member

Joined: Wed Aug 30, 2017 8:24 am
Posts: 1590
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:
#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:
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:
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.

_________________
Carpe diem!


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 5:09 am 
Offline
Member
Member

Joined: Fri Nov 22, 2019 5:46 am
Posts: 590
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


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 2:30 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
These don't seem to be working either.

Originally, I tried these two:

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


Code:
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:
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:
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).


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 7:15 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
So, I managed to get it mostly working.

This is my code for terminal_putchar:

Code:
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:
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.


Attachments:
terminal_scroll.png
terminal_scroll.png [ 45.63 KiB | Viewed 16167 times ]
Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 8:14 pm 
Offline
Member
Member

Joined: Mon Mar 25, 2013 7:01 pm
Posts: 5069
I don't see why you're still trying to use a loop.
Code:
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 );


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 8:40 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
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.


Attachments:
terminal_scroll.png
terminal_scroll.png [ 46.13 KiB | Viewed 16156 times ]
Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 9:50 pm 
Offline
Member
Member

Joined: Mon Mar 25, 2013 7:01 pm
Posts: 5069
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.


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 10:50 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
Octocontrabass wrote:
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:
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.


Attachments:
terminal_scroll.png
terminal_scroll.png [ 44.36 KiB | Viewed 16138 times ]
Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sat Feb 27, 2021 11:09 pm 
Offline
Member
Member

Joined: Mon Mar 25, 2013 7:01 pm
Posts: 5069
Code:
        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.

dengeltheyounger wrote:
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.


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sun Feb 28, 2021 2:46 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
I changed the putchar function, but it did not seem to make any difference.
Code:
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.


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sun Feb 28, 2021 3:28 pm 
Offline
Member
Member

Joined: Wed Feb 24, 2021 8:52 pm
Posts: 25
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.


Top
 Profile  
 
 Post subject: Re: Meaty Skeleton terminal_scroll bug
PostPosted: Sun Feb 28, 2021 10:10 pm 
Offline
Member
Member

Joined: Wed Aug 30, 2017 8:24 am
Posts: 1590
dengeltheyounger wrote:
Too bad there is no emoji for facepalming.
The commonly accepted one is: m(

_________________
Carpe diem!


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 16 posts ]  Go to page 1, 2  Next

All times are UTC - 6 hours


Who is online

Users browsing this forum: No registered users and 2 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group