Fix: TFT version precision and format upgrade (sensor → text, 2-segment versioning)#98
Fix: TFT version precision and format upgrade (sensor → text, 2-segment versioning)#98
Conversation
…nt versioning) ## What changed The TFT version is now reported as a 2-segment string (e.g. `16.1`) instead of a plain integer. ### Problem The previous implementation stored the TFT version as a `float` sensor, which caused IEEE 754 precision loss on the ESP32 — `16.1` was displayed in Home Assistant as `16.1000003814697`. ### Solution - `version_tft` is now a `text_sensor`, consistent with `version_blueprint` and `version_esphome` - The Nextion boot params parser now stores the raw version string as received (e.g. `"16.1"`) - A new `tft_ver_gte()` function handles 2-segment (`major.minor`) numeric version comparison, correctly handling cases like `16.12 > 16.2` - The Blueprint compares TFT versions using `| float(0)` filters, avoiding both lexicographic pitfalls and the need for parts-splitting logic - The `fire_ha_event` version payload now sends the TFT version as a string, preserving the decimal format end-to-end into Home Assistant
📝 WalkthroughWalkthroughMigrates TFT version handling from numeric floats to validated version strings. Adds Changes
Sequence Diagram(s)(Skipped — changes are localized helpers and YAML wiring; no new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/nspanel_easy/versioning.cpp`:
- Around line 36-46: The sscanf checks (ver_fields/min_fields) allow partial
matches like "16.", "16x", or "16.1.2"; update version parsing in versioning.cpp
to strictly validate the full input before or instead of sscanf: for each input
string (version and min_version) enforce a full-match pattern such as ^\d+$ or
^\d+\.\d+$ (e.g., via std::regex_match) and only then parse into ver_maj/ver_min
and min_maj/min_min (or use sscanf with a trailing-char check "%d.%d%c" and
reject if an extra char is consumed); return false for any string that does not
exactly match the allowed formats.
In `@esphome/nspanel_esphome_page_boot.yaml`:
- Around line 67-88: The TFT version validation currently only whitelists digits
and '.' which accepts invalid values like ".", "16.", or "16..1"; update the
validation logic around ver_str so it only accepts either "major" (one or more
digits) or "major.minor" (one or more digits, a single '.', then one or more
digits): ensure the first character is a digit, allow at most one '.', disallow
trailing or leading '.', and when a '.' exists verify there are digits on both
sides; then keep the same behavior of calling
version_tft->publish_state(ver_str) only when valid and setting
system_flags.display_settings_received based on that stricter tft_ver_valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaf52510-56d5-421f-8aeb-9f1e40aeadc4
📒 Files selected for processing (6)
components/nspanel_easy/versioning.cppcomponents/nspanel_easy/versioning.hesphome/nspanel_esphome_addon_upload_tft.yamlesphome/nspanel_esphome_hw_display.yamlesphome/nspanel_esphome_page_boot.yamlesphome/nspanel_esphome_version.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
esphome/nspanel_esphome_page_boot.yaml (1)
67-68: Use strict numeric parsing for boot parameters.Lines 67–68 parse display settings with
atoi(), which accepts malformed input (e.g., signed prefixes, whitespace). Negative values wrap to largeuint8_tvalues when assigned, potentially bypassing the> 0gate on line 88. Consider usingstrtoul()with full input validation and range checking to reject invalid input outright.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esphome/nspanel_esphome_page_boot.yaml` around lines 67 - 68, The current boot parsing uses atoi on params[2]/params[3] (assigned to display_mode and display_charset) which accepts malformed input and can produce negative values that wrap into large uint8_t; replace with strtoul-based parsing: call strtoul on each params[i], check endptr consumes the whole string (no leading/trailing garbage), ensure errno is cleared/checked for overflow, and validate the numeric value is within the allowed range (e.g., 0–255 and if you rely on a >0 gate later ensure >0 when required) before casting/assigning to display_mode and display_charset; if validation fails, reject the parameter (return error/log) rather than silently accepting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@esphome/nspanel_esphome_page_boot.yaml`:
- Around line 67-68: The current boot parsing uses atoi on params[2]/params[3]
(assigned to display_mode and display_charset) which accepts malformed input and
can produce negative values that wrap into large uint8_t; replace with
strtoul-based parsing: call strtoul on each params[i], check endptr consumes the
whole string (no leading/trailing garbage), ensure errno is cleared/checked for
overflow, and validate the numeric value is within the allowed range (e.g.,
0–255 and if you rely on a >0 gate later ensure >0 when required) before
casting/assigning to display_mode and display_charset; if validation fails,
reject the parameter (return error/log) rather than silently accepting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bc1f272-c319-4da8-93f7-35d6c45fc74b
📒 Files selected for processing (2)
components/nspanel_easy/versioning.cppesphome/nspanel_esphome_page_boot.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- components/nspanel_easy/versioning.cpp
What changed
The TFT version is now reported as a 2-segment string (e.g.
16.1) instead of a plain integer.Problem
The previous implementation stored the TFT version as a
floatsensor, which caused IEEE 754precision loss on the ESP32 —
16.1was displayed in Home Assistant as16.1000003814697.Solution
version_tftis now atext_sensor, consistent withversion_blueprintandversion_esphome"16.1")tft_ver_gte()function handles 2-segment (major.minor) numeric version comparison, correctly handling cases like16.12 > 16.2| float(0)filters, avoiding both lexicographic pitfalls and the need for parts-splitting logicfire_ha_eventversion payload now sends the TFT version as a string, preserving the decimal format end-to-end into Home AssistantSummary by CodeRabbit
New Features
Bug Fixes
Refactor