Skip to content

fix: TFT code review fixes — null guards, queue, showText4, struct cleanup #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sjordan0228 merged 4 commits into from
Apr 1, 2026
+89 −44
Merged

fix: TFT code review fixes — null guards, queue, showText4, struct cleanup #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14e0770
fix: TFT code review — null queue guards, increase queue to 8, log drops
sjordan0228 Apr 1, 2026
de168c8
fix: TFT showText4 now shows all lines (Spool: Unknown Tag)
sjordan0228 Apr 1, 2026
96b3350
fix: remove duplicate TFTSpoolData struct, use DisplaySpoolData directly
sjordan0228 Apr 1, 2026
1db63bd
fix: check xTaskCreatePinnedToCore result in startTask()
sjordan0228 Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ docs/writer-ui-plan.md
research/
docs/superpowers/
.mcp.json
CODE-REVIEW.md
.mex/
test-preview/
.gstack/
Expand Down
113 changes: 85 additions & 28 deletions src/TFTManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,15 @@ void TFTManager::begin() {
Serial.println("TFTManager: WARNING — sprite allocation failed (low heap)");
}

_messageQueue = xQueueCreate(4, sizeof(TFTMessage));
_messageQueue = xQueueCreate(8, sizeof(TFTMessage));
if (_messageQueue == nullptr) {
Serial.println("TFTManager: WARNING — queue creation failed");
}
_lastActivityMs = millis();
}

void TFTManager::startTask() {
xTaskCreatePinnedToCore(
BaseType_t result = xTaskCreatePinnedToCore(
taskFunc,
"TFTTask",
8192, // TFT + sprite rendering needs more stack than LCD
Expand All @@ -153,7 +153,12 @@ void TFTManager::startTask() {
&_taskHandle,
0 // Core 0, same as LCDTask
);
Serial.println("TFTManager: Task started on core 0");
if (result == pdPASS) {
Serial.println("TFTManager: Task started on core 0");
} else {
Serial.println("TFTManager: WARNING — task creation failed");
_taskHandle = nullptr;
}
}

// ---------------------------------------------------------------------------
Expand All @@ -163,65 +168,101 @@ void TFTManager::showBoot(const char* version) {
TFTMessage msg{};
msg.state = TFTState::Boot;
snprintf(msg.statusText, sizeof(msg.statusText), "%s", version);
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showWifiConnecting() {
TFTMessage msg{};
msg.state = TFTState::WifiConnecting;
snprintf(msg.statusText, sizeof(msg.statusText), "WiFi");
snprintf(msg.statusText2, sizeof(msg.statusText2), "Connecting...");
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showWifiConnected(const char* ip) {
TFTMessage msg{};
msg.state = TFTState::WifiConnecting;
snprintf(msg.statusText, sizeof(msg.statusText), "WiFi Connected");
snprintf(msg.statusText2, sizeof(msg.statusText2), "%s", ip);
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showReady() {
TFTMessage msg{};
msg.state = TFTState::Ready;
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showSpoolScanned(const TFTSpoolData& spool) {
void TFTManager::showSpoolScanned(const DisplaySpoolData& spool) {
TFTMessage msg{};
msg.state = TFTState::SpoolScanned;
msg.spool = spool;
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showWriting(const char* tagFormat) {
TFTMessage msg{};
msg.state = TFTState::Writing;
snprintf(msg.statusText, sizeof(msg.statusText), "%s", tagFormat);
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showWriteResult(bool success, const char* tagFormat) {
TFTMessage msg{};
msg.state = TFTState::WriteResult;
msg.writeSuccess = success;
snprintf(msg.statusText, sizeof(msg.statusText), "%s", tagFormat);
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showKeypadEntry(const char* toolNumber) {
TFTMessage msg{};
msg.state = TFTState::KeypadEntry;
snprintf(msg.statusText, sizeof(msg.statusText), "%s", toolNumber);
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showError(const char* errMsg) {
TFTMessage msg{};
msg.state = TFTState::Error;
snprintf(msg.statusText, sizeof(msg.statusText), "%s", errMsg);
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::setScreenTimeoutMs(uint32_t timeoutMs) {
Expand Down Expand Up @@ -253,7 +294,7 @@ void TFTManager::taskLoop() {

void TFTManager::processQueue() {
TFTMessage msg;
if (xQueueReceive(_messageQueue, &msg, 0) == pdTRUE) {
if (_messageQueue && xQueueReceive(_messageQueue, &msg, 0) == pdTRUE) {
taskENTER_CRITICAL(&_stateMux);
_lastActivityMs = millis();
bool wasOff = _screenOff;
Expand Down Expand Up @@ -372,7 +413,7 @@ void TFTManager::renderReady() {
_sprite.pushSprite(0, 0);
}

void TFTManager::renderSpoolScanned(const TFTSpoolData& spool) {
void TFTManager::renderSpoolScanned(const DisplaySpoolData& spool) {
_sprite.fillScreen(COLOR_BG);

int W = _tft.width(); // 240
Expand Down Expand Up @@ -639,33 +680,49 @@ void TFTManager::showText(const char* line1, const char* line2) {
msg.state = TFTState::WifiConnecting; // generic two-line status renderer
snprintf(msg.statusText, sizeof(msg.statusText), "%s", line1 ? line1 : "");
snprintf(msg.statusText2, sizeof(msg.statusText2), "%s", line2 ? line2 : "");
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showText4(const char* line1, const char* line2,
const char* line3, const char* line4) {
// TFT uses two lines — show line3 as primary, line4 as secondary
// Show all meaningful content — line3 as primary, line4 as secondary
// Prepend cleaned line1 context (strip LCD asterisk decoration) if present
TFTMessage msg{};
msg.state = TFTState::WifiConnecting; // generic two-line status renderer
snprintf(msg.statusText, sizeof(msg.statusText), "%s", line3 ? line3 : (line1 ? line1 : ""));
if (line1 && line3) {
// Strip asterisks/spaces from line1 for clean TFT display
const char* clean = line1;
while (*clean == '*' || *clean == ' ') clean++;
Comment on lines +698 to +699

@coderabbitai coderabbitai Bot Apr 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding braces for clarity.

The while loop on line 699 is a single statement without braces. While syntactically correct, braces improve readability and prevent accidental bugs during future modifications.

Optional style improvement 
-        while (*clean == '*' || *clean == ' ') clean++;
+        while (*clean == '*' || *clean == ' ') {
+            clean++;
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const char* clean = line1;
while (*clean == '*' || *clean == ' ') clean++;
const char* clean = line1;
while (*clean == '*' || *clean == ' ') {
clean++;
}
🧰 Tools
🪛 Clang (14.0.6)

[warning] 699-699: statement should be inside braces

(readability-braces-around-statements)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/TFTManager.cpp` around lines 698 - 699, The while loop that advances the
pointer "clean" (initialized from "line1") is written without braces, which can
hinder readability and lead to future bugs; update the loop that checks (*clean
== '*' || *clean == ' ') to use explicit braces around its body so the intent is
clear and future additions won't be accidentally excluded—locate the
initialization "const char* clean = line1;" and the subsequent while(...) and
add braces around the statement that advances the pointer.

size_t len = strlen(clean);
while (len > 0 && (clean[len-1] == '*' || clean[len-1] == ' ')) len--;
char stripped[48] = {};
if (len > 0 && len < sizeof(stripped)) { memcpy(stripped, clean, len); stripped[len] = '\0'; }
if (stripped[0]) {
snprintf(msg.statusText, sizeof(msg.statusText), "%s: %s", stripped, line3);
} else {
snprintf(msg.statusText, sizeof(msg.statusText), "%s", line3);
}
} else {
snprintf(msg.statusText, sizeof(msg.statusText), "%s", line3 ? line3 : (line1 ? line1 : ""));
}
snprintf(msg.statusText2, sizeof(msg.statusText2), "%s", line4 ? line4 : (line2 ? line2 : ""));
xQueueSend(_messageQueue, &msg, 0);
if (_messageQueue) {
if (xQueueSend(_messageQueue, &msg, 0) != pdTRUE) {
Serial.println("TFTManager: Queue full, dropped display update");
}
}
}

void TFTManager::showKeypad(const char* digits) {
showKeypadEntry(digits && digits[0] ? digits : "_");
}

void TFTManager::showSpool(const DisplaySpoolData& spool) {
TFTSpoolData tftSpool{};
strncpy(tftSpool.brand, spool.brand, sizeof(tftSpool.brand) - 1);
strncpy(tftSpool.material, spool.material, sizeof(tftSpool.material) - 1);
snprintf(tftSpool.name, sizeof(tftSpool.name), "%s %s", spool.brand, spool.material);
strncpy(tftSpool.colorHex, spool.colorHex, sizeof(tftSpool.colorHex) - 1);
tftSpool.remainingWeight = spool.remainingWeight;
tftSpool.totalWeight = spool.totalWeight;
tftSpool.tagType = spool.tagType;
showSpoolScanned(tftSpool);
showSpoolScanned(spool);
}

// ---------------------------------------------------------------------------
Expand Down
19 changes: 3 additions & 16 deletions src/TFTManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,6 @@
#include "TFTConfig.h"
#include "DisplayI.h"

// ---------------------------------------------------------------------------
// TFTSpoolData — the data the TFT needs after a scan
// ---------------------------------------------------------------------------
struct TFTSpoolData {
char brand[32]; // e.g. "eSUN"
char material[16]; // e.g. "PLA"
char name[48]; // e.g. "eSUN PLA+ White"
char colorHex[8]; // e.g. "FFFFFF" (no leading #)
float remainingWeight; // grams remaining
float totalWeight; // grams total (for % bar)
uint8_t tagType; // TAG_TYPE_* constants below
};

// Tag type constants — one icon per type
#define TAG_TYPE_UNKNOWN 0
#define TAG_TYPE_OPENPRINTTAG 1
Expand Down Expand Up @@ -46,7 +33,7 @@ enum class TFTState {
// ---------------------------------------------------------------------------
struct TFTMessage {
TFTState state;
TFTSpoolData spool; // valid when state == SpoolScanned
DisplaySpoolData spool; // valid when state == SpoolScanned
char statusText[48]; // used for Boot/Ready/Error/Writing/KeypadEntry
char statusText2[48]; // second line for generic status display
bool writeSuccess; // used for WriteResult
Expand All @@ -67,7 +54,7 @@ class TFTManager : public DisplayI {
void showWifiConnecting();
void showWifiConnected(const char* ip);
void showReady();
void showSpoolScanned(const TFTSpoolData& spool);
void showSpoolScanned(const DisplaySpoolData& spool);
void showWriting(const char* tagFormat);
void showWriteResult(bool success, const char* tagFormat) override;
void showKeypadEntry(const char* toolNumber);
Expand Down Expand Up @@ -95,7 +82,7 @@ class TFTManager : public DisplayI {
// --- Rendering ---
void renderBoot(const char* version);
void renderReady();
void renderSpoolScanned(const TFTSpoolData& spool);
void renderSpoolScanned(const DisplaySpoolData& spool);
void renderStatus(const char* line1, const char* line2 = nullptr);
void renderWriteResult(bool success, const char* tagFormat);
void renderKeypadEntry(const char* toolNumber);
Expand Down