I'm trying to write a kernel (an OS kernel) in C++ and I've encountered a really strange problem (I'm sure it'll be quite obvious what I'm doing wrong for anyone else, I just for the life of me can't find what's wrong)
I'm using a C++ class to represent a VGA console (The BIOS should load this at address 0xB8000) that can be accessed early in the OS bootsequence for debugging output.
(for more, read: http://wiki.osdev.org/Bare_Bones)
Here's my kernel's main function:
#include "Kernel.hpp"
extern "C"
{
void kernelMain ()
{
VGATerminal kernelTerm = VGATerminal ();
//kernelTerm.print("[");
//kernelTerm.setTextColour(VGATerminal::LIGHT_GREEN);
//kernelTerm.print("OK");
//kernelTerm.setTextColour(VGATerminal::WHITE);
//kernelTerm.print("]\t\tKernel gained control of the CPU.\n");
//kernelTerm.print("0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123465789\n");
}
}
(Right now the output is commented out, but it works as expected, using the right characters and colours in the right places)
Here's the VGATerminal constructor:
VGATerminal::VGATerminal ()
{
this->row = 0;
this->col = 0;
this->currentColour = generateVGAColour(WHITE, BLACK);
this->vgaBuffer = (uint16*) VGATerminal::VGA_MEMORY;
for (uint16 r = 0; r < VGATerminal::HEIGHT; r++) // Loop over rows
{
for (uint16 c = 0; c < 8; c++) // Loop over columns
{
this->vgaBuffer[(r * WIDTH) + c] = generateColouredChar(' ', this->currentColour);
//this->print(' ');
}
}
//this->row = 0;
//this->col = 0;
}
This code sends my OS into an infinite loop whenever the columns iterate past 7 (7 is fine, 8 sends it into an endless loop). (By 7 is fine, I mean it produces the expected behaviour of clearing the first 7 columns of the screen (although I can't really verify this, since before I clear the screen, it's not filled with text. The text that is on screen gets erased correctly though)
The loop starts somewhere at the beginning of the kernel (When i generate an ISO for my OS, it actually loops back to before GRUB opens, GRUB opens, I select my OS and it goes back to the beginning).
When I replace the this->vgaBuffer[...] = ... part with this->print(' ') everything works fine. (If I do this, I also need to reset this->row and this->col, that's why those two lines are commented out at the end)
Here's the VGATerminal::print(const char c) function:
void VGATerminal::print (const char c)
{
switch (c)
{
case '\t':
// First insert 1 space, this will force the terminal to insert atleast this
// (Otherwise, you might get something like this:)
// CATS\tDOGS Becomes CATSDOGS instead of CATS DOGS
// This happens because after writing CATS, the terminal is at a multiple of 4
// and decides it doesn't need to insert anything anymore
this->vgaBuffer[(this->row * WIDTH) + this->col] = generateColouredChar(' ', this->currentColour);
this->col++;
while ((this->col % 4) != 0)
{
this->vgaBuffer[(this->row * WIDTH) + this->col] = generateColouredChar(' ', this->currentColour);
this->col++;
if (this->col == WIDTH)
{
this->row++;
this->col = 0;
}
}
break;
case '\n':
this->row++;
this->col = 0;
break;
case '\r':
this->col = 0;
break;
default:
this->vgaBuffer[(this->row * WIDTH) + this->col] = generateColouredChar(c, this->currentColour);
this->col++;
if (this->col == WIDTH)
{
this->row++;
this->col = 0;
}
break;
}
}
Support for carriage returns and newlines might not be complete, I'd like to get rid of this bug first before testing those out, altough newlines seem to be working correctly.
uint16 generateColouredChar (const char c, uint8 colour) is actually a function (as opposed to a method):
uint16 VGATerminal::generateColouredChar (const char c, uint8 colour)
{
return (uint16) colour << 8 | (uint16) c;
}
uint8, uint16, ... are all aliases for their respective counterparts (uint8_t, uint16_t, ...) , created in another header as follows:
#include <stdint.h>
using uint8 = uint8_t;
using uint16 = uint16_t;
using uint32 = uint32_t;
using uint64 = uint64_t;
using uchar = uint16;
using int8 = int8_t;
using int16 = int16_t;
using int32 = int32_t;
using int64 = int64_t;
(I did this because it doesn't reduce readability IMO but it saves me having to type that annoying '_')
Edit:
Here's the complete VGATerminal class for future reference:
class VGATerminal
{
private:
constexpr static uint16 WIDTH = 80;
constexpr static uint16 HEIGHT = 25;
constexpr static int VGA_MEMORY = 0xB8000;
uint16 row;
uint16 col;
uint8 currentColour;
volatile uint16* vgaBuffer;
public:
/// Colour constants
constexpr static uint8 BLACK = 0;
constexpr static uint8 BLUE = 1;
constexpr static uint8 GREEN = 2;
constexpr static uint8 CYAN = 3;
constexpr static uint8 RED = 4;
constexpr static uint8 MAGENTA = 5;
constexpr static uint8 BROWN = 6;
constexpr static uint8 LIGHT_GREY = 7;
constexpr static uint8 DARK_GREY = 8;
constexpr static uint8 LIGHT_BLUE = 9;
constexpr static uint8 LIGHT_GREEN = 10;
constexpr static uint8 LIGHT_CYAN = 11;
constexpr static uint8 LIGHT_RED = 12;
constexpr static uint8 LIGHT_MAGENTA = 13;
constexpr static uint8 LIGHT_BROWN = 14;
constexpr static uint8 WHITE = 15;
VGATerminal ();
void clear ();
void setTextColour (uint8 colour);
void setBackgroundColour (uint8 colour);
void print (const char c);
void print (const char* str);
};
There's no obvious reason (in original post was not) why the 7/8 should differ.
How do you define VGATerminal::vgaBuffer? Did you mark it as volatile to avoid compiler optimizing out "useless" memory writes?
Compiler doesn't understand that writing value at 0xB8000 address has external effect outside of C++ language definition, by using volatile you give compiler hint, that it's not regular memory C++ variable write (the compiler is free to implement C++ variable any way it wish, even without actual memory write, as long as the code works as C++ language defines), but the actual memory read/write should be done, because it may cause external effects (in case of write), or the value may have been modified externally (like different thread code, or by OS, or by external device which has memory access too).
BTW as an assembly programmer it irks me to see such proper nested for loop just to clear screen:
VGATerminal::VGATerminal ()
{
this->row = 0;
this->col = 0;
this->currentColour = generateVGAColour(WHITE, BLACK);
this->vgaBuffer = (uint16*) VGATerminal::VGA_MEMORY;
const uint16 whiteSpace = generateColouredChar(' ', this->currentColour);
for (unsigned i = 0; i < VGATerminal::HEIGHT*VGATerminal::WIDTH; ++i)
this->vgaBuffer[i] = whiteSpace;
}
Although I think the optimizer may have modified the nested loops into single loop once you did use VGATerminal::WIDTH for columns, it's just old habit of asm programmer to write minimal code and avoid multiplications and multiple counters in cases where you are working with continuous memory block, and the logical separation of rows/columns is not important for the current task.