code cleanup

cgreening · cgreening · commit 715e9a030d2e · 2026-01-15T15:36:29.000Z
diff --git a/pong/main/Display.cpp b/pong/main/Display.cpp
@@ -6,13 +6,17 @@
 #include <cstdlib>
 #include <vector>
 
-static inline uint8_t clampU8(int v)
+namespace {
+
+// Clamp integer to uint8_t range [0, 255].
+inline uint8_t clampU8(int v)
 {
   if (v < 0) return 0;
   if (v > 255) return 255;
   return (uint8_t)v;
 }
 
+// Byte-swap a 32-bit value (big-endian <-> little-endian).
 uint32_t swapEndian32(uint32_t val)
 {
   return ((val >> 24) & 0x000000FF) |
@@ -21,20 +25,24 @@ uint32_t swapEndian32(uint32_t val)
          ((val << 24) & 0xFF000000);
 }
 
+// Read a big-endian int32 from a byte buffer (used for VLW font parsing).
 int32_t readInt32(const uint8_t *data)
 {
   int32_t val = *((int32_t *)data);
   val = (int32_t)swapEndian32((uint32_t)val);
   return val;
 }
 
+// Read a big-endian uint32 from a byte buffer (used for VLW font parsing).
 uint32_t readUInt32(const uint8_t *data)
 {
   uint32_t val = *((uint32_t *)data);
   val = (uint32_t)swapEndian32((uint32_t)val);
   return val;
 }
 
+} // namespace
+
 Display::Display(int width, int height) : _width(width), _height(height)
 {
   // RGB565 framebuffer: width * height.
diff --git a/pong/main/Display.h b/pong/main/Display.h
@@ -1,3 +1,9 @@
+// Display.h - Software RGB565 framebuffer with drawing primitives
+//
+// Provides a TFT-style API for 2D graphics: rectangles, lines, polygons, and
+// anti-aliased text rendering using VLW fonts. The framebuffer can be read
+// directly for JPEG encoding or other processing.
+
 #pragma once
 
 #include <cstdint>
@@ -89,8 +95,8 @@ class Display
   virtual void drawLine(int16_t x0, int16_t y0, int16_t x1, int16_t y1, Color color);
   virtual void drawPolygon(const std::vector<Point> &vertices, Color color);
   virtual void drawFilledPolygon(const std::vector<Point> &vertices, Color color);
-  int width() { return _width; }
-  int height() { return _height; }
+  int width() const { return _width; }
+  int height() const { return _height; }
   virtual void drawPixel(Color color, int x, int y);
 
   // Raw RGB565 framebuffer access (row-major, 1 uint16_t per pixel).
diff --git a/pong/main/Game.h b/pong/main/Game.h
@@ -1,3 +1,8 @@
+// Game.h - Abstract base class for games rendered via USB UVC
+//
+// Defines the standard game loop interface: readInput() -> update(dt) -> render().
+// FrameStats provides performance metrics for on-screen display.
+
 #pragma once
 
 #include <stdint.h>
diff --git a/pong/main/PongGame.cpp b/pong/main/PongGame.cpp
@@ -80,9 +80,8 @@ void PongGame::resetBall(int field_w, int field_h, int dir)
     ball_y_ = (float)field_h * 0.5f;
 
     // Fixed initial angle so self-play is deterministic.
-    const float vy = 0.35f;
     ball_vx_ = (float)dir * kBallSpeed;
-    ball_vy_ = kBallSpeed * vy;
+    ball_vy_ = kBallSpeed * kServeAngleRatio;
 }
 
 void PongGame::readInput()
@@ -245,7 +244,7 @@ void PongGame::render(Display &display, const FrameStats &stats)
                 ball_vx_ = std::abs(ball_vx_);
                 const float center = (py0 + py1) * 0.5f;
                 const float rel = (ball_y_ - center) / ((float)kPaddleH * 0.5f); // -1..1
-                ball_vy_ += rel * 60.0f;
+                ball_vy_ += rel * kPaddleSpinFactor;
             }
         }
 
@@ -258,12 +257,12 @@ void PongGame::render(Display &display, const FrameStats &stats)
                 ball_vx_ = -std::abs(ball_vx_);
                 const float center = (py0 + py1) * 0.5f;
                 const float rel = (ball_y_ - center) / ((float)kPaddleH * 0.5f); // -1..1
-                ball_vy_ += rel * 60.0f;
+                ball_vy_ += rel * kPaddleSpinFactor;
             }
         }
 
         // Out of bounds.
-        if (ball_x_ < -10.0f) {
+        if (ball_x_ < -kOutOfBoundsMargin) {
             if (playing) {
                 score_r_++;
                 if (score_r_ >= kWinPoints) {
@@ -279,7 +278,7 @@ void PongGame::render(Display &display, const FrameStats &stats)
                 // Demo: just reset ball, no score.
                 resetBall(field_w, field_h, 1);
             }
-        } else if (ball_x_ > (float)field_w + 10.0f) {
+        } else if (ball_x_ > (float)field_w + kOutOfBoundsMargin) {
             if (playing) {
                 score_l_++;
                 if (score_l_ >= kWinPoints) {
diff --git a/pong/main/PongGame.h b/pong/main/PongGame.h
@@ -1,3 +1,8 @@
+// PongGame.h - Classic Pong game implementation
+//
+// Features AI opponent, demo mode (self-play), and optional potentiometer control.
+// The game runs at whatever frame rate the USB host requests.
+
 #pragma once
 
 #include "Game.h"
@@ -62,16 +67,20 @@ class PongGame final : public Game {
 
     bool started_once_ = false;
 
-    // Layout
+    // Layout constants (pixels)
     static constexpr int kPaddleW = 6;
     static constexpr int kPaddleH = 40;
     static constexpr int kPaddleInset = 8;
-    static constexpr int kBallSize = 6; // square ball
+    static constexpr int kBallSize = 6;
 
-    // Tuning
-    static constexpr float kPaddleSpeed = 180.0f; // px/s
-    static constexpr float kBallSpeed = 180.0f;   // px/s
+    // Physics constants
+    static constexpr float kPaddleSpeed = 180.0f;        // px/s
+    static constexpr float kBallSpeed = 180.0f;          // px/s
+    static constexpr float kServeAngleRatio = 0.35f;     // Initial vertical velocity as fraction of ball speed
+    static constexpr float kPaddleSpinFactor = 60.0f;    // Velocity added when hitting paddle off-center
+    static constexpr float kOutOfBoundsMargin = 10.0f;   // How far ball travels past edge before scoring
 
+    // Game rules
     static constexpr int kWinPoints = 3;
     static constexpr uint64_t kGameOverToDemoDelayUs = 10ULL * 1000000ULL;
 };
diff --git a/pong/main/RollingAverage.h b/pong/main/RollingAverage.h
@@ -1,7 +1,14 @@
+// RollingAverage.h - Efficient rolling average calculator
+//
+// Computes a rolling average over the last N samples in O(1) time per update.
+// Useful for smoothing frame timing statistics (FPS, encode time, draw time).
+
 #pragma once
 
 #include <stdint.h>
 
+/// Rolling average over the last WindowSize samples.
+/// Uses O(1) time per update and O(WindowSize) memory.
 template <int WindowSize>
 class RollingAverageU32 {
 public:
diff --git a/pong/main/TestGame.cpp b/pong/main/TestGame.cpp
diff --git a/pong/main/TestGame.h b/pong/main/TestGame.h
diff --git a/pong/main/jpeg_encoder.h b/pong/main/jpeg_encoder.h
@@ -1,3 +1,10 @@
+// jpeg_encoder.h - JPEG encoder wrapper for ESP-IDF
+//
+// Wraps the ESP JPEG encoder with a simple C++ interface. Supports RGB888 and
+// RGB565 input formats. The encoder handle is cached to avoid per-frame overhead.
+//
+// Memory ownership: encode methods allocate output via malloc(); caller must free().
+
 #pragma once
 
 #include <stdint.h>
diff --git a/pong/main/main.cpp b/pong/main/main.cpp
@@ -28,7 +28,7 @@
 #include "fonts/GillSans_25_vlw.h"
 
 
-static const char *TAG = "display";
+static const char *TAG = "main";
 
 extern const unsigned char test_card_jpeg_start[] asm("_binary_test_card_jpeg_start");
 extern const unsigned char test_card_jpeg_end[]   asm("_binary_test_card_jpeg_end");
@@ -47,16 +47,16 @@ extern "C" void app_main(void)
 {
     for(int i = 0; i < 5; i++) {
         vTaskDelay(1000 / portTICK_PERIOD_MS);
-        printf("Startup delay %d seconds...\n", 5 - i);
+        ESP_LOGI(TAG, "Startup delay %d seconds...", 5 - i);
     }
 
-    printf("Hello world!\n");
+    ESP_LOGI(TAG, "Hello world!");
 
     /* Print chip information */
     esp_chip_info_t chip_info;
     uint32_t flash_size;
     esp_chip_info(&chip_info);
-    printf("This is %s chip with %d CPU core(s), %s%s%s%s, ",
+    ESP_LOGI(TAG, "This is %s chip with %d CPU core(s), %s%s%s%s",
            CONFIG_IDF_TARGET,
            chip_info.cores,
            (chip_info.features & CHIP_FEATURE_WIFI_BGN) ? "WiFi/" : "",
@@ -66,26 +66,27 @@ extern "C" void app_main(void)
 
     unsigned major_rev = chip_info.revision / 100;
     unsigned minor_rev = chip_info.revision % 100;
-    printf("silicon revision v%d.%d, ", major_rev, minor_rev);
     if(esp_flash_get_size(NULL, &flash_size) != ESP_OK) {
-        printf("Get flash size failed");
+        ESP_LOGE(TAG, "Get flash size failed");
         return;
     }
 
-    printf("%" PRIu32 "MB %s flash\n", flash_size / (uint32_t)(1024 * 1024),
+    ESP_LOGI(TAG, "Silicon revision v%d.%d, %" PRIu32 "MB %s flash",
+           major_rev, minor_rev,
+           flash_size / (uint32_t)(1024 * 1024),
            (chip_info.features & CHIP_FEATURE_EMB_FLASH) ? "embedded" : "external");
 
     {
         uint32_t freq_hz = 0;
         esp_err_t ret = esp_clk_tree_src_get_freq_hz(SOC_MOD_CLK_CPU, ESP_CLK_TREE_SRC_FREQ_PRECISION_APPROX, &freq_hz);
         if (ret == ESP_OK) {
-            printf("CPU clock: %" PRIu32 " MHz\n", freq_hz / 1000000u);
+            ESP_LOGI(TAG, "CPU clock: %" PRIu32 " MHz", freq_hz / 1000000u);
         } else {
-            printf("CPU clock: unknown (err=%d)\n", (int)ret);
+            ESP_LOGW(TAG, "CPU clock: unknown (err=%d)", (int)ret);
         }
     }
 
-    printf("Minimum free heap size: %" PRIu32 " bytes\n", esp_get_minimum_free_heap_size());
+    ESP_LOGI(TAG, "Minimum free heap size: %" PRIu32 " bytes", esp_get_minimum_free_heap_size());
 
     static Display display(320, 240);
     static const JpegEncoder encoder(JpegEncoder::Config{
diff --git a/pong/main/uvc_streamer.cpp b/pong/main/uvc_streamer.cpp
@@ -18,6 +18,10 @@ bool UvcStreamer::waitForFrameRequest(TickType_t ticks_to_wait)
     if (producer_task_ == nullptr) {
         return false;
     }
+    // xTaskNotifyWait: Block until another task sends a notification via xTaskNotify.
+    // This is more efficient than polling and uses less CPU than a semaphore for
+    // simple task wakeup scenarios. The notification value lets us distinguish
+    // between different event types if needed.
     uint32_t value = 0;
     const BaseType_t ok = xTaskNotifyWait(0, kNotifyFrameReqBit, &value, ticks_to_wait);
     return (ok == pdTRUE) && ((value & kNotifyFrameReqBit) != 0);
@@ -57,6 +61,8 @@ uvc_fb_t *UvcStreamer::camera_fb_get_cb(void *cb_ctx)
 uvc_fb_t *UvcStreamer::fbGet()
 {
     // Host is requesting a frame; wake the producer to draw/encode a new one.
+    // xTaskNotify: Send a notification to the producer task, setting a bit in its
+    // notification value. This wakes the producer if it's blocked in xTaskNotifyWait.
     if (producer_task_ != nullptr) {
         xTaskNotify(producer_task_, kNotifyFrameReqBit, eSetBits);
     }
@@ -65,8 +71,10 @@ uvc_fb_t *UvcStreamer::fbGet()
         return NULL;
     }
 
-    // Take exclusive ownership of the single JPEG buffer.
-    // We keep the timeout small to avoid stalling the UVC task if the producer is mid-copy.
+    // xSemaphoreTake: Acquire exclusive ownership of the JPEG buffer.
+    // Binary semaphores implement mutual exclusion: only one task can "take" it
+    // at a time; others block until it's "given" back. We use a short timeout
+    // to avoid stalling the UVC task if the producer is mid-copy.
     if (xSemaphoreTake(jpeg_sem_, pdMS_TO_TICKS(10)) != pdTRUE) {
         return NULL;
     }
@@ -139,8 +147,18 @@ esp_err_t UvcStreamer::start(uint8_t *uvc_buffer, size_t uvc_buffer_size)
         .cb_ctx = this,
     };
 
-    uvc_device_config(0, &config);
-    uvc_device_init();
+    esp_err_t err = uvc_device_config(0, &config);
+    if (err != ESP_OK) {
+        ESP_LOGE(TAG, "uvc_device_config failed: %s", esp_err_to_name(err));
+        return err;
+    }
+
+    err = uvc_device_init();
+    if (err != ESP_OK) {
+        ESP_LOGE(TAG, "uvc_device_init failed: %s", esp_err_to_name(err));
+        return err;
+    }
+
     return ESP_OK;
 }
 
@@ -154,6 +172,8 @@ void UvcStreamer::setNewJpegData(const uint8_t *jpeg_data, size_t jpeg_data_size
     }
 
     // Block until the UVC task has finished reading the current buffer (fbReturn()).
+    // portMAX_DELAY means wait forever - the producer won't proceed until it
+    // safely owns the buffer. This prevents overwriting data while UVC is reading.
     xSemaphoreTake(jpeg_sem_, portMAX_DELAY);
 
     memcpy(jpeg_buf_, jpeg_data, jpeg_data_size);
diff --git a/pong/main/uvc_streamer.h b/pong/main/uvc_streamer.h
@@ -1,3 +1,12 @@
+// uvc_streamer.h - USB UVC device streaming glue
+//
+// Bridges the game rendering loop with the USB UVC driver. Uses a producer-consumer
+// pattern: the game loop waits for frame requests from the host, renders a frame,
+// encodes it to JPEG, and hands it off for USB transmission.
+//
+// Synchronization uses FreeRTOS task notifications (for wakeup) and a binary
+// semaphore (for buffer ownership).
+
 #pragma once
 
 #include <stddef.h>
diff --git a/static_test_card/main/main.cpp b/static_test_card/main/main.cpp
diff --git a/static_test_card/main/uvc_streamer.cpp b/static_test_card/main/uvc_streamer.cpp
diff --git a/static_test_card/main/uvc_streamer.h b/static_test_card/main/uvc_streamer.h

Original file line number	Diff line number	Diff line change
`@@ -6,13 +6,17 @@`
`6`	`6`	`#include <cstdlib>`
`7`	`7`	`#include <vector>`
`8`	`8`
`9`		`-static inline uint8_t clampU8(int v)`
	`9`	`+namespace {`
	`10`	`+`
	`11`	`+// Clamp integer to uint8_t range [0, 255].`
	`12`	`+inline uint8_t clampU8(int v)`
`10`	`13`	`{`
`11`	`14`	`if (v < 0) return 0;`
`12`	`15`	`if (v > 255) return 255;`
`13`	`16`	`return (uint8_t)v;`
`14`	`17`	`}`
`15`	`18`
	`19`	`+// Byte-swap a 32-bit value (big-endian <-> little-endian).`
`16`	`20`	`uint32_t swapEndian32(uint32_t val)`
`17`	`21`	`{`
`18`	`22`	`return ((val >> 24) & 0x000000FF) \|`
`@@ -21,20 +25,24 @@ uint32_t swapEndian32(uint32_t val)`
`21`	`25`	`((val << 24) & 0xFF000000);`
`22`	`26`	`}`
`23`	`27`
	`28`	`+// Read a big-endian int32 from a byte buffer (used for VLW font parsing).`
`24`	`29`	`int32_t readInt32(const uint8_t *data)`
`25`	`30`	`{`
`26`	`31`	`int32_t val = ((int32_t )data);`
`27`	`32`	`val = (int32_t)swapEndian32((uint32_t)val);`
`28`	`33`	`return val;`
`29`	`34`	`}`
`30`	`35`
	`36`	`+// Read a big-endian uint32 from a byte buffer (used for VLW font parsing).`
`31`	`37`	`uint32_t readUInt32(const uint8_t *data)`
`32`	`38`	`{`
`33`	`39`	`uint32_t val = ((uint32_t )data);`
`34`	`40`	`val = (uint32_t)swapEndian32((uint32_t)val);`
`35`	`41`	`return val;`
`36`	`42`	`}`
`37`	`43`
	`44`	`+} // namespace`
	`45`	`+`
`38`	`46`	`Display::Display(int width, int height) : _width(width), _height(height)`
`39`	`47`	`{`
`40`	`48`	`// RGB565 framebuffer: width * height.`