bitfixer / esp32s3vga

A VGA output driver for the ESP32S3
GNU General Public License v3.0
13 stars 2 forks source link

Quick Code Review of VGA.cpp #1

Open kktos opened 1 year ago

kktos commented 1 year ago

Hi there, nothing really important, just 2 or 3 things I've noticed watching bounceEvent. I'm in the chasing cycles mode. Or I think I'm :)

line 311: skip is never use line 354: could we hold (vga->_frameHeight-bufLineIndex) in a var instead of computing it twice line 365: what about regrouping the blocks per frameScale and colorBits ?

  switch(vga->_frameScale) {

    case 1:
       switch(vga->_colorBits) {
          case 8:
            ...
            break;
          case 3:
            ...
            break;
          case 1:
            ...
            break;
       }
       break;

    case 2:
       switch(vga->_colorBits) {
          case 8:
            ...
            break;
          case 3:
            ...
            break;
          case 1:
            ...
            break;
       }
       break;

  }

line 457: what about having pixelBits & 0b00000111 in a var instead of the repetition ? Not sure about this as the compiler may be smart and do it by itself... line 498: same here... the compiler may be smart enough to realise the vga->_frameScale == 1 so the mult is useless line 529: again.... a shift left could do the trick but the compiler may have seen it too ;)

Brilliant work, btw ;)