Add calibration methods and app#520
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full touch calibration flow: a new Touch Calibration app and manifest; a touch calibration settings API and implementation that persist, validate, cache, and apply calibration data; a new 🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
Tactility/Source/app/display/Display.cpp (1)
134-137: Consider removing unused user_data parameter.The callback receives
thisas user_data (line 311) but doesn't use it. This is harmless but could be cleaner.♻️ Optional cleanup
static void onCalibrateTouchClicked(lv_event_t* event) { (void)event; app::start("TouchCalibration"); }And on line 311:
- lv_obj_add_event_cb(calibrate_button, onCalibrateTouchClicked, LV_EVENT_SHORT_CLICKED, this); + lv_obj_add_event_cb(calibrate_button, onCalibrateTouchClicked, LV_EVENT_SHORT_CLICKED, nullptr);Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cpp (1)
147-173: Sampling introduces ~8ms latency per touch read.The
readRawPointfunction takes 8 samples with 1ms delay between each (line 163), resulting in ~8ms minimum latency per touch poll. Combined with LVGL's input polling rate, this could affect touch responsiveness.This may be intentional for noise filtering, but worth noting for touch-heavy UI interactions.
Tactility/Source/settings/TouchCalibrationSettings.cpp (1)
82-89: Reject invalid enabled calibration settings at save-time.Consider enforcing
isValid(settings)insave()whensettings.enabledis true, so invalid calibration never gets persisted.♻️ Proposed tweak
bool save(const TouchCalibrationSettings& settings) { + if (settings.enabled && !isValid(settings)) { + return false; + } std::map<std::string, std::string> map;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce2bbcf6-1437-4005-8516-64815f0cd904
📒 Files selected for processing (11)
Drivers/XPT2046/Source/Xpt2046Touch.cppDrivers/XPT2046/Source/Xpt2046Touch.hDrivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cppDrivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.hTactility/Include/Tactility/app/touchcalibration/TouchCalibration.hTactility/Include/Tactility/hal/touch/TouchDevice.hTactility/Include/Tactility/settings/TouchCalibrationSettings.hTactility/Source/Tactility.cppTactility/Source/app/display/Display.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/settings/TouchCalibrationSettings.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Tactility/Source/settings/TouchCalibrationSettings.cpp (2)
61-64:⚠️ Potential issue | 🟠 Major
std::strtolparsing is currently lossy and can silently accept bad values.Line 61-Line 64 parse with
nullptrendptr and noerrno/range checks, so malformed or out-of-range values can be accepted unexpectedly.🧮 Proposed fix
`#include` <algorithm> +#include <cerrno> `#include` <cstdlib> +#include <limits> `#include` <map> `#include` <string> @@ static bool toBool(const std::string& value) { return value == "1" || value == "true" || value == "True"; } + +static bool parseInt32(const std::string& value, int32_t& out) { + errno = 0; + char* end = nullptr; + const long parsed = std::strtol(value.c_str(), &end, 10); + if (errno != 0 || end == value.c_str() || *end != '\0') { + return false; + } + if (parsed < std::numeric_limits<int32_t>::min() || parsed > std::numeric_limits<int32_t>::max()) { + return false; + } + out = static_cast<int32_t>(parsed); + return true; +} @@ - loaded.xMin = static_cast<int32_t>(std::strtol(x_min_it->second.c_str(), nullptr, 10)); - loaded.xMax = static_cast<int32_t>(std::strtol(x_max_it->second.c_str(), nullptr, 10)); - loaded.yMin = static_cast<int32_t>(std::strtol(y_min_it->second.c_str(), nullptr, 10)); - loaded.yMax = static_cast<int32_t>(std::strtol(y_max_it->second.c_str(), nullptr, 10)); + if (!parseInt32(x_min_it->second, loaded.xMin) || + !parseInt32(x_max_it->second, loaded.xMax) || + !parseInt32(y_min_it->second, loaded.yMin) || + !parseInt32(y_max_it->second, loaded.yMax)) { + return false; + }
94-105:⚠️ Potential issue | 🔴 CriticalCache state is mutated/read without synchronization.
Line 94-Line 105 accesses shared mutable globals from
save()andgetActive()without locking; concurrent callers can race and observe torn/stale state.🔒 Proposed fix
`#include` <algorithm> `#include` <cstdlib> `#include` <map> +#include <mutex> `#include` <string> @@ static bool cacheInitialized = false; static TouchCalibrationSettings cachedSettings; +static std::mutex cacheMutex; @@ bool save(const TouchCalibrationSettings& settings) { + std::lock_guard<std::mutex> lock(cacheMutex); std::map<std::string, std::string> map; @@ TouchCalibrationSettings getActive() { + std::lock_guard<std::mutex> lock(cacheMutex); if (!cacheInitialized) { cachedSettings = loadOrGetDefault(); cacheInitialized = true; } return cachedSettings; }
🧹 Nitpick comments (1)
Tactility/Source/settings/TouchCalibrationSettings.cpp (1)
82-97: Validate calibration invariants before persisting enabled settings.
save()writes and caches any payload. Rejecting invalid enabled settings here prevents storing unusable calibration state.✅ Proposed guard in save path
bool save(const TouchCalibrationSettings& settings) { + if (settings.enabled && !isValid(settings)) { + return false; + } + std::map<std::string, std::string> map; map[SETTINGS_KEY_ENABLED] = settings.enabled ? "1" : "0";
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f982477e-f476-4543-bc1c-52955f4ecb74
📒 Files selected for processing (1)
Tactility/Source/settings/TouchCalibrationSettings.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tactility/Source/app/touchcalibration/TouchCalibration.cpp (1)
117-124: Consider clamping negative touch coordinates.
lv_point_tcoordinates could theoretically be negative if a touch is reported outside the display bounds. The direct cast touint16_ton line 122-123 would wrap around to large positive values, corrupting calibration data.🛡️ Proposed defensive clamping
lv_point_t point = {0, 0}; lv_indev_get_point(indev, &point); if (sampleCount < 4) { samples[sampleCount] = { - .x = static_cast<uint16_t>(point.x), - .y = static_cast<uint16_t>(point.y), + .x = static_cast<uint16_t>(std::max(static_cast<lv_coord_t>(0), point.x)), + .y = static_cast<uint16_t>(std::max(static_cast<lv_coord_t>(0), point.y)), };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f92f30be-b36a-4d6a-9b35-11457ce612aa
📒 Files selected for processing (5)
Drivers/XPT2046/Source/Xpt2046Touch.hTactility/Include/Tactility/settings/TouchCalibrationSettings.hTactility/Source/app/display/Display.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/settings/TouchCalibrationSettings.cpp
✅ Files skipped from review due to trivial changes (1)
- Tactility/Source/app/display/Display.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Drivers/XPT2046/Source/Xpt2046Touch.h
|
The UI is not fully tested. I will test it properly soon! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tactility/Source/app/touchcalibration/TouchCalibration.cpp (1)
75-90: Prefer a single source of truth for range validation.
MIN_RANGEat Line 75 duplicates constraints already enforced bysettings::touch::isValid(settings)at Line 89. Keeping both risks drift if rules change.Refactor suggestion
- constexpr int32_t MIN_RANGE = 20; - const int32_t xMin = (static_cast<int32_t>(samples[0].x) + static_cast<int32_t>(samples[3].x)) / 2; const int32_t xMax = (static_cast<int32_t>(samples[1].x) + static_cast<int32_t>(samples[2].x)) / 2; const int32_t yMin = (static_cast<int32_t>(samples[0].y) + static_cast<int32_t>(samples[1].y)) / 2; const int32_t yMax = (static_cast<int32_t>(samples[2].y) + static_cast<int32_t>(samples[3].y)) / 2; @@ - if ((xMax - xMin) < MIN_RANGE || (yMax - yMin) < MIN_RANGE || !settings::touch::isValid(settings)) { + if (!settings::touch::isValid(settings)) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 485043d2-21d0-45fe-a604-e82efba3ed29
📒 Files selected for processing (1)
Tactility/Source/app/touchcalibration/TouchCalibration.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Tactility/Source/app/touchcalibration/TouchCalibration.cpp (2)
147-150: Avoid evicting the freshly-saved calibration on teardown.Line 150 throws away the cache that
settings::touch::save()just refreshed. That makes the first post-calibration read hit settings I/O again for no real gain. Re-enabling runtime calibration is enough here.♻️ Proposed fix
void onDestroy(AppContext& app) override { (void)app; settings::touch::setRuntimeCalibrationEnabled(true); - settings::touch::invalidateCache(); }
75-89: Keep calibration validity rules in one place.
settings::touch::isValid()already owns the minimum-range policy. Duplicating it here makes the UI and persistence path drift-prone if that rule changes later.♻️ Proposed fix
- constexpr int32_t MIN_RANGE = 20; - const int32_t xMin = (static_cast<int32_t>(samples[0].x) + static_cast<int32_t>(samples[3].x)) / 2; const int32_t xMax = (static_cast<int32_t>(samples[1].x) + static_cast<int32_t>(samples[2].x)) / 2; const int32_t yMin = (static_cast<int32_t>(samples[0].y) + static_cast<int32_t>(samples[1].y)) / 2; const int32_t yMax = (static_cast<int32_t>(samples[2].y) + static_cast<int32_t>(samples[3].y)) / 2; @@ - if ((xMax - xMin) < MIN_RANGE || (yMax - yMin) < MIN_RANGE || !settings::touch::isValid(settings)) { + if (!settings::touch::isValid(settings)) { lv_label_set_text(titleLabel, "Calibration Failed"); lv_label_set_text(hintLabel, "Range invalid. Tap to close."); lv_obj_add_flag(target, LV_OBJ_FLAG_HIDDEN); setResult(Result::Error); return;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38763d23-dd94-431f-bccb-968b704b73eb
📒 Files selected for processing (1)
Tactility/Source/app/touchcalibration/TouchCalibration.cpp
|
The UI and calibration saving (and runtime calibration) all work! |
|
@KenVanHoeylandt Ready for review! |
1 participant
This pull request introduces a unified and extensible touch calibration system for touch drivers, integrating persistent calibration settings, a new settings API, and a user interface for calibration. The changes remove legacy per-driver calibration logic and replace it with a centralized approach, making calibration consistent across drivers and accessible from the display settings UI.
Summary by CodeRabbit
New Features
Improvements