4
\$\begingroup\$

I'm working on a 80x25 terminal that splits the screen into two. The top half gets the first buffer, the bottom half gets the second buffer. I'm concerned about readability and accessibility.

For accessibility, what I'm trying to do is hide as much of the implementation as possible so that other parts of the code don't have to rely on this class as much. For example, I'm able to plug in a third party printf implementation and only have to change it at one place, where it outputs the buffer. Therefore 90% of the class should be private. The user only uses printf for outputting, unless they need to change the buffer or color.

For readability, after adding the logic to handle two buffers, I noticed an explosion in buffers[current_buffer]. I want to be able to reduce this. As well as that, I've found myself making typos such as typing 'rows' instead of 'columns' and so on. By tackling readability, annoying bugs like this hopefully can be eliminated.

Should I make terminal a global object?

Terminal.hpp

extern Terminal terminal;

Terminal.cpp

// Define terminal in one translation unit.
Terminal terminal;

Example usage:

main.cpp

printf("Buffer 1.\n");
// Zero-indexed.
terminal.setbuffer(1);
printf("Buffer 2.\n");

printf.cpp

// Printf doesn't need to care about terminal's internals. Just copy
// to buffer and output result to screen.
void printf(const char* fmt, ...)
{
    va_list args;
    int i;
    va_start(args, fmt);
    i = vsprintf(terminal.getcurrentbuffer().buffer, fmt, args);
    va_end(args);
    terminal.getcurrentbuffer().buffer[i] = '\0';
    terminal.puts(terminal.getcurrentbuffer().buffer);
}

How should I handle access to the current buffer and setting it?

terminal.getcurrentbuffer().buffer is an eyesore and a handful to type. But I don't want to actually return a char* instead of a Buffer&. Secondly, terminal.puts(terminal.getcurrentbuffer().buffer); seems rather redundant, but currently I just write directly to video memory.

Terminal.hpp

    // Zero-indexed. So 0 == buffer1, 1 == buffer2, etc.
    void setbuffer(int new_buffer);
    Buffer& getcurrentbuffer();

private:
    Buffer buffers[2];

    int current_buffer;

Terminal.cpp

// Assumes that the user provided a non-zero index.
void Terminal::setbuffer(int new_buffer)
{
    current_buffer = new_buffer;
}

Terminal::Buffer& Terminal::getcurrentbuffer()
{
    return buffers[current_buffer];
}

What is a way to improve readability for loops like this?

void Terminal::scroll()
{
    uint16_t blank = make_vgaentry(' ', buffers[current_buffer].color
    );

    if (buffers[current_buffer].ypos >= buffers[current_buffer].rows) {

        for (int i =   buffers[current_buffer].ybase
                     * buffers[current_buffer].columns; 
                 i <  (buffers[current_buffer].ybase + buffers[current_buffer].rows - 1) 
                     * buffers[current_buffer].columns; i++)
        {
            video_buffer[i] = video_buffer[i + buffers[current_buffer].columns];
        }

        for (int i =  (buffers[current_buffer].ybase + buffers[current_buffer].rows - 1) 
                     * buffers[current_buffer].columns; 
                 i <  (buffers[current_buffer].ybase 
                     + buffers[current_buffer].rows) * buffers[current_buffer].columns; i++)
        {
            video_buffer[i] = blank;
        }
        buffers[current_buffer].ypos = buffers[current_buffer].rows - 1;
    }
}

I considered using a local variable copy but that might be expensive(?) I also considered making function stubs that return common calculations, like x + y * columns and such.


Full code dump.

Terminal.hpp

#ifndef TERMINAL_HPP
#define TERMINAL_HPP

#include <common.hpp> // stddef.h, stdint.h

class Terminal
{
    struct Buffer
    {
        char buffer[1024];
        int xpos;
        int ypos;
        int ybase; // What line the buffer starts at
        constexpr static int columns = 80;

        // This is how many lines each buffer gets.
        constexpr static int rows = 12;

        // Combined foreground and background color.
        uint8_t color;
    };
public:
    enum class Color
    {
        Black        = 0,
        Blue         = 1,
        Green        = 2,
        Cyan         = 3,
        Red          = 4,
        Magenta      = 5,
        Brown        = 6,
        LightGrey    = 7,
        DarkGrey     = 8,
        LightBlue    = 9,
        LightGreen   = 10,
        LightCyan    = 11,
        LightRed     = 12,
        LightMagenta = 13,
        LightBrown   = 14,
        White        = 15,
    };

    Terminal();
    ~Terminal();

    // Clears entire screen and puts separate in middle. Doesn't clear
    // invidiual buffer.
    void clrscr();

    void clrbuf(int buffer);
    void putch(uint8_t c);
    void puts(const char* s);

    // Zero-indexed. So 0 == buffer1, 1 == buffer2, etc.
    void setbuffer(int new_buffer);
    Buffer& getcurrentbuffer();

private:
    volatile uint16_t* video_buffer;

    Buffer buffers[2];

    int current_buffer;

    uint8_t make_color(Color fg, Color bg);
    uint16_t make_vgaentry(uint8_t c, uint8_t color);

    void scroll();
    void movecsr();
};

extern Terminal terminal;

#endif

Terminal.cpp

#include <terminal.hpp>
#include <system.hpp> // outb, inb
#include <common.hpp>
#include <cstring.hpp> // C library string functions

// Define terminal in one translation unit.
Terminal terminal;

// Constructors.

Terminal::Terminal()
    : video_buffer(reinterpret_cast<volatile uint16_t*>(0xb8000)),
      current_buffer(0)
{
    buffers[0].color = make_color(Color::White, Color::Black);
    buffers[1].color = make_color(Color::LightGreen, Color::Green);
    // Put buffer2's starting point after the separator.
    buffers[1].ybase = 13;
    clrscr();
    clrbuf(0);
    clrbuf(1);
    terminal.setbuffer(current_buffer);
}

Terminal::~Terminal()
{
}

// Public functions.

void Terminal::clrscr()
{
    for (int i = 0; i < buffers[current_buffer].rows * buffers[current_buffer].columns; ++i)
    {
        video_buffer[i] = make_vgaentry(' ', buffers[current_buffer].color);
    }

    // http://www.asciitable.com/
    // Set middle of screen with separator.
    for (int i = 0; i < buffers[current_buffer].columns; ++i)
    {
        video_buffer[i + 12 * buffers[current_buffer].columns] 
               = make_vgaentry(205, buffers[current_buffer].color);
    }
}

void Terminal::clrbuf(int buffer)
{
    for (int i = buffers[buffer].ybase * buffers[buffer].columns; 
             i <  (buffers[buffer].ybase + buffers[buffer].rows) 
                 * buffers[buffer].columns; ++i)
    {
        video_buffer[i] = make_vgaentry(' ', buffers[buffer].color);
    }
}

void Terminal::putch(uint8_t c)
{
    if (c == '\n')
    {
        buffers[current_buffer].xpos = 0;
        buffers[current_buffer].ypos++;
    } else {
        video_buffer[   buffers[current_buffer].xpos 
                     + (buffers[current_buffer].ybase + buffers[current_buffer].ypos)
                     *  buffers[current_buffer].columns] 
                       = make_vgaentry(c, buffers[current_buffer].color);
        buffers[current_buffer].xpos++;
    }

    if (buffers[current_buffer].xpos >= buffers[current_buffer].columns)
    {
        buffers[current_buffer].xpos = 0;
        buffers[current_buffer].ypos++;
    }

    scroll();
    movecsr();
}

void Terminal::puts(const char* s)
{
    size_t len = strlen((char*) s);
    for (size_t i = 0; i < len; ++i)
    {
        putch(s[i]);
    }
}

void Terminal::setbuffer(int new_buffer)
{
    current_buffer = new_buffer;
}

Terminal::Buffer& Terminal::getcurrentbuffer()
{
    return buffers[current_buffer];
}

// Private functions.

/*
+------------------+-----------------+
|    Attribute     |    Character    |
+------------------+-----------------+
| 7 6 5 4 3 2 1 0  | 7 6 5 4 3 2 1 0 |
| B BG    FG       | CP              |
+------------------+-----------------+
B  - Blink
BG - Background Color
FG - Foreground Color
CP - Codepoint

Although bits 4-7 may make up the background color.
*/

uint8_t Terminal::make_color(Color fg, Color bg)
{
    return static_cast<uint8_t>(fg) | 
           static_cast<uint8_t>(bg) << 4;
}

uint16_t Terminal::make_vgaentry(uint8_t c, uint8_t color)
{
    uint16_t c16 = c;
    uint16_t color16 = color;
    return c16 | color16 << 8;
}

// https://github.com/stupaq/sos-kernel/blob/master/kernel/lib/monitor.c#L40
// We add ybase to each place where ypos is required, so that the correct
// position is calculated for the respective buffer.
void Terminal::scroll()
{
    uint16_t blank = make_vgaentry(' ', buffers[current_buffer].color
    );

    if (buffers[current_buffer].ypos >= buffers[current_buffer].rows) {

        for (int i =   buffers[current_buffer].ybase
                     * buffers[current_buffer].columns; 
                 i <  (buffers[current_buffer].ybase + buffers[current_buffer].rows - 1) 
                     * buffers[current_buffer].columns; i++)
        {
            video_buffer[i] = video_buffer[i + buffers[current_buffer].columns];
        }

        for (int i =  (buffers[current_buffer].ybase + buffers[current_buffer].rows - 1) 
                     * buffers[current_buffer].columns; 
                 i <  (buffers[current_buffer].ybase 
                     + buffers[current_buffer].rows) * buffers[current_buffer].columns; i++)
        {
            video_buffer[i] = blank;
        }
        buffers[current_buffer].ypos = buffers[current_buffer].rows - 1;
    }
}

void Terminal::movecsr() 
{
    // The screen is 80 characters wide...
    uint16_t cursorLocation =  (buffers[current_buffer].ybase + buffers[current_buffer].ypos) 
                              * buffers[current_buffer].columns + buffers[current_buffer].xpos;
    outb(0x3D4, 14); // Tell the VGA board we are setting the high cursor byte.
    outb(0x3D5, cursorLocation >> 8); // Send the high cursor byte.
    outb(0x3D4, 15); // Tell the VGA board we are setting the low cursor byte.
    outb(0x3D5, cursorLocation); // Send the low cursor byte.
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ getcurrentbuffer is a bit of a pain to read anyway. I'd much rather see get_current_buffer or getCurrentBuffer. \$\endgroup\$ – cHao Sep 18 '14 at 7:42
5
\$\begingroup\$

Should I make terminal a global object?

No. This is an implementation detail and you (probably) don't want to impose this decision on client code; That is, client code should be able to decide at any point if application needs a global, a temporary or multiple instances used for various situations.

Other remarks:

terminal.setbuffer(1);

This is not setting a buffer, but selecting a buffer by index (consider the names selectbuffer, choosebuffer, setbufferindex and so on).

Terminal::Buffer is a structure. Consider creating it as a class (outside of Terminal) and not exposing it's data.

Currently you expose it's data to client code and (because of it) you end up with this client code:

terminal.getcurrentbuffer().buffer
//      ^^^^^^^^^^^^^^^^^^^^^^^^^^ <- should be implementation detail of terminal
// and not exposed publicly

for (int i = buffers[current_buffer].ybase [...] )
//           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ should be behind an iterator
// or class interface

but currently I just write directly to video memory.

Don't! The only scenario when you should expose the private data of a class is when the class stays valid no matter what client code does with the data (in other words, if I cannot memcpy 2 Gb of random data (for example) into the public buffer member of Terminal::Buffer, it should not be public. You should instead add an API to write to the buffer (and this API should check boundaries, validate contents and so on).

How should I handle access to the current buffer and setting it?

You should not give access to the current buffer. You should instead expose an API that allows client code to add data to the terminal. If client code needs to know there are buffers, client code should choose the buffer in the write operation.

Old code:

void printf(const char* fmt, ...)
{
    va_list args;
    int i;
    va_start(args, fmt);
    i = vsprintf(terminal.getcurrentbuffer().buffer, fmt, args);
    va_end(args);
    terminal.getcurrentbuffer().buffer[i] = '\0';
    terminal.puts(terminal.getcurrentbuffer().buffer);
}

New code:

void printf(const char* fmt, ...)
{
    char localBuffer[1024]; // this is not very flexible
    va_list args;
    int i;
    va_start(args, fmt);
    i = vsprintf(localBuffer, fmt, args);
    va_end(args);
    terminal.write(localBuffer, i); // << you should implement this
}

Your Terminal::write API shoud:

  • write to the current buffer (up to n characters)
  • manage zero termination
  • handle (or throw) errors

This API does not require client code to know about buffers.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the iterator idea. It didn't occur to me to write begin and end functions for buffer. \$\endgroup\$ – user25057 Sep 18 '14 at 9:50
  • \$\begingroup\$ vsnprintf would be much better, can be called once to get the buffer size, e.g. int sz = vsnprintf(nullptr, 0, fmt, args) and/or all this could be added as another method of terminal (e.g. print()). \$\endgroup\$ – user52292 Sep 18 '14 at 11:15
3
\$\begingroup\$

Should terminal be a global object?

It depends. I would think it is to be a singleton. One terminal per process is all you are going to get on most if not all OSs. A global object, as it is now, is not very nice. I suggest either making it a proper singleton class or letting the user instantiate one.

Overwriting printf?

Is your intention to overwrite printf to make it use your terminal? I'm not sure how portable this is, but I wouldn't recommend nevertheless. Programmers are expecting printf to write to stdout, by changing this behavior, you can cause a lot of confusion, plus, one can no longer redirect stdout output of the program to a file.

I suggest that instead you make writes to your terminal explicit. Define a Terminal::printf() member.

Naming convention:

void setbuffer(int new_buffer);
Buffer& getcurrentbuffer();

Some case differentiation or underscores will make names more readable. You should use either:

void setBuffer(int new_buffer);
Buffer& getCurrentBuffer();

Or

void set_buffer(int new_buffer);
Buffer& get_current_buffer();

Make methods that don't access class data static:

The methods:

uint8_t  make_color(Color fg, Color bg);
uint16_t make_vgaentry(uint8_t c, uint8_t color);

Are not accessing any member data, so they should be qualified as static:

static uint8_t  make_color(Color fg, Color bg);
static uint16_t make_vgaentry(uint8_t c, uint8_t color);

Improve Buffer:

Buffer should be made into a class and have some helper methods added to it. Instead of accessing the data by just doing buffer.buffer you should define an array access operator (operator []) that does debug bounds checking in buffer to avoid invalid indexing. Them make buffer a private member.

Some getXPos(), getYPos(), etc are probably also in order to avoid any accidental data modification when reading the buffer. Make those methods inline and you will have zero runtime overhead.

Empty destructor:

Your destructor is currently empty, so you don't need to define it. Let the compiler add the implicit one for you.

Raw constants:

Terminal::Terminal()
    : video_buffer(reinterpret_cast<volatile uint16_t*>(0xb8000)),
      current_buffer(0)
{

The 0xb8000 is the VRAM start address, I believe. It should them be a named constant, not a magic number:

static constexpr int VRAM_START_ADDRESS = 0xb8000;

Again, at the end, you have a few more hexadecimal constants:

outb(0x3D4, 14); // Tell the VGA board we are setting the high cursor byte.
outb(0x3D5, cursorLocation >> 8); // Send the high cursor byte.
outb(0x3D4, 15); // Tell the VGA board we are setting the low cursor byte.
outb(0x3D5, cursorLocation); // Send the low cursor byte.

Name those constants and let the code be self documented. It is much better than just the comments.

Add explanatory temp variables:

This loop and a few others:

for (int i = buffers[buffer].ybase * buffers[buffer].columns; 
         i <  (buffers[buffer].ybase + buffers[buffer].rows) 
             * buffers[buffer].columns; ++i)
{
    video_buffer[i] = make_vgaentry(' ', buffers[buffer].color);
}

Those very long lines are unclear and confusing. You should add some explanatory temp variables:

const auto & buf = buffers[buffer];
const int start  =  buf.ybase * buf.columns;
const int end    = (buf.ybase + buf.rows) * buf.columns;

for (int i = start; i != end; ++i)
{
    video_buffer[i] = make_vgaentry(' ', buf.color);
}
// It looks a lot like an iterator range now.
// Which would also be a very nice thing to have, BTW.

Now the loop is much more readable and less intimidating. The names I gave are just examples and might not be the best ones in this context. But the concept is very valid.

Use assertions and contracts:

// Assumes that the user provided a non-zero index.
void Terminal::setbuffer(int new_buffer)
{
    current_buffer = new_buffer;
}

Never assume that the user will be well behaved. Shield your code from invalid inputs with assertions:

void Terminal::setbuffer(int new_buffer)
{
    // Guarantee index is in a valid range
    assert(new_buffer > 0);
    assert(new_buffer < 2);
    current_buffer = new_buffer;
}

Or in this case, use unsigned int and keep just the second assert.

Miscellaneous:

enum class Color
{
    Black        = 0,
    Blue         = 1,
    Green        = 2,
    Cyan         = 3,
    Red          = 4,
    Magenta      = 5,
    Brown        = 6,
    LightGrey    = 7,
    DarkGrey     = 8,
    LightBlue    = 9,
    LightGreen   = 10,
    LightCyan    = 11,
    LightRed     = 12,
    LightMagenta = 13,
    LightBrown   = 14,
    White        = 15, // <--- Some compilers will issue a warning for this misplaced comma
};
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Nice review! I would just use auto& buf = buffers[buffer] before the for-loop. \$\endgroup\$ – user52292 Sep 18 '14 at 15:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy