rust-osdev/vga

Creating a ScreenCharacter using TextModeColor

StackDoubleFlow opened this issue ยท 7 comments

It would be easier for a vga writer to store a TextModeColor instead of storing both the foreground color and the background color. This will also make it easier to convert the current blog_os vga to use this crate.
Something like this for example:

pub struct VgaWriter {
    column_position: usize,
    color_code: TextModeColor,
    text_mode: Text80x25,
}

The issue with this would be when writing a character.

impl VgaWriter {
    fn write_byte(&mut self, byte: u8) {
        let row = self.text_mode.get_height() - 1;
        let col = self.column_position;
        self.text_mode.write_character(row, col, ScreenCharacter {
            character: byte, 
            color: color_code,
        });
        self.column_position += 1;
    }
}

The above code will not compile because character and color is private.
There are a couple options to for this:

  1. We could publicize character and color
  2. We could create an alternative function to ScreenCharacter::new that takes a TextModeColor instead of two Color16Bits

Thanks for this crate and hopefully I'm not misunderstanding all of this. I'm also open to contribute if it helps.

Thanks for this crate and hopefully I'm not misunderstanding all of this. I'm also open to contribute if it helps.

Thanks for the feedback and you're not misunderstanding anything. I mainly wanted to get everything working, so I'm open to improvements. Changing ScreenCharacter::new to take a TextModeColor seems like it would be more useful then taking 2 Color16Bit. It might make sense to also add TextModeColor::set_foreground(foreground: Color16Bit) and TextModeColor::set_background(background: Color16Bit) in case someone wants to modify an existing TextModeColor.

I was also working on switching to this crate in my version of blog_os and I'm pretty sure I'm going to need to add fn read_character(&self, x: usize, y: usize) -> ScreenCharacter; to TextWriter in order for you to be able to read characters to shift the screen during new_line.

@phil-opp Do you have any thoughts on this? Not saying you're going to switch blog_os to use this crate, but I'd love your feedback on this also if you have some spare time.

@StackDoubleFlow I added a few things related to your suggestions. Would you mind pointing your project to the dev branch here to test it out? This is the code I used in my os to test it. I use a different crate for the lazy initialization and spinlock, but it should be pretty similar.

pub static WRITER: Lazy<Spinlock<Writer>> = Lazy::new(|| {
    let text_mode = Text80x25::new();
    text_mode.set_mode();
    Spinlock::new(Writer {
        column_position: 0,
        color_code: TextModeColor::new(Color16Bit::Yellow, Color16Bit::Black),
        text_mode: text_mode,
    })
});

pub struct Writer {
    column_position: usize,
    color_code: TextModeColor,
    text_mode: Text80x25,
}

impl Writer {
    pub fn write_byte(&mut self, byte: u8) {
        match byte {
            b'\n' => self.new_line(),
            byte => {
                if self.column_position >= self.text_mode.get_width() {
                    self.new_line();
                }

                let row = self.text_mode.get_height() - 1;
                let col = self.column_position;

                self.text_mode.write_character(
                    col,
                    row,
                    ScreenCharacter::new(byte, self.color_code),
                );
                self.column_position += 1;
            }
        }
    }

    fn write_string(&mut self, s: &str) {
        for byte in s.bytes() {
            match byte {
                // Printable ASCII byte or newling.
                0x20..=0x7e | b'\n' => self.write_byte(byte),

                // Not part of the printable ASCII range.
                _ => self.write_byte(0xf3),
            }
        }
    }

    fn new_line(&mut self) {
        for row in 1..BUFFER_HEIGHT {
            for col in 0..BUFFER_WIDTH {
                let character = self.text_mode.read_character(col, row);
                self.text_mode.write_character(col, row - 1, character);
            }
        }
        self.clear_row(BUFFER_HEIGHT - 1);
        self.column_position = 0;
    }

    fn clear_row(&mut self, row: usize) {
        let blank = ScreenCharacter::new(b' ', self.color_code);
        for col in 0..self.text_mode.get_width() {
            self.text_mode.write_character(col, row, blank);
        }
    }
}

Also you'll need to use the newest version of bootloader in order for the memory ranges vga uses to be mapped.

bootloader = { version = "0.9.0", features = ["map_physical_memory"] }

I pointed my project to the dev branch and anything seems to be working perfectly! It seems to be very easy to convert the blog_os code to use this crate.

Glad it works. I'll update the docs and try to upload this version to crates in the next day or so.

@phil-opp Do you have any thoughts on this? Not saying you're going to switch blog_os to use this crate, but I'd love your feedback on this also if you have some spare time.

I don't really have any strong opinions on this. Either way seems fine to me. Regarding blog_os: I think we will keep the custom code in the VGA post as is because it is the first time that we actually write some Rust code in the blog series. However, we could suggest the vga crate at the end of the post.

Sounds good to me. Thanks for the feedback.

@StackDoubleFlow I just pushed 0.1.2 to crates so you should be good switching off the the dev branch.