From 3d47c081f23727a52e98e95f26b0185c4aaf64e3 Mon Sep 17 00:00:00 2001 From: Xuan-Son Nguyen Date: Mon, 16 Feb 2026 13:53:00 +0100 Subject: [PATCH] fix: use RAII render lock everywhere (#916) ## Summary Follow-up to https://github.com/crosspoint-reader/crosspoint-reader/pull/774 --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **NO** ## Summary by CodeRabbit ## Release Notes * **Refactor** * Modernized internal synchronization mechanisms across multiple components to improve code reliability and maintainability. All functionality remains unchanged. --- .../network/WifiSelectionActivity.cpp | 7 +- src/activities/reader/EpubReaderActivity.cpp | 108 +++++++++--------- .../reader/KOReaderSyncActivity.cpp | 91 ++++++++------- .../settings/ClearCacheActivity.cpp | 7 +- .../settings/KOReaderAuthActivity.cpp | 35 +++--- .../settings/KOReaderSettingsActivity.cpp | 1 - src/activities/settings/OtaUpdateActivity.cpp | 28 +++-- 7 files changed, 151 insertions(+), 126 deletions(-) diff --git a/src/activities/network/WifiSelectionActivity.cpp b/src/activities/network/WifiSelectionActivity.cpp index 793f2492..bbfefdb7 100644 --- a/src/activities/network/WifiSelectionActivity.cpp +++ b/src/activities/network/WifiSelectionActivity.cpp @@ -244,9 +244,10 @@ void WifiSelectionActivity::checkConnectionStatus() { // Save this as the last connected network - SD card operations need lock as // we use SPI for both - xSemaphoreTake(renderingMutex, portMAX_DELAY); - WIFI_STORE.setLastConnectedSsid(selectedSSID); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + WIFI_STORE.setLastConnectedSsid(selectedSSID); + } // If we entered a new password, ask if user wants to save it // Otherwise, immediately complete so parent can start web server diff --git a/src/activities/reader/EpubReaderActivity.cpp b/src/activities/reader/EpubReaderActivity.cpp index e7e03184..0c523619 100644 --- a/src/activities/reader/EpubReaderActivity.cpp +++ b/src/activities/reader/EpubReaderActivity.cpp @@ -221,11 +221,12 @@ void EpubReaderActivity::loop() { if (skipChapter) { // We don't want to delete the section mid-render, so grab the semaphore - xSemaphoreTake(renderingMutex, portMAX_DELAY); - nextPageNumber = 0; - currentSpineIndex = nextTriggered ? currentSpineIndex + 1 : currentSpineIndex - 1; - section.reset(); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + nextPageNumber = 0; + currentSpineIndex = nextTriggered ? currentSpineIndex + 1 : currentSpineIndex - 1; + section.reset(); + } requestUpdate(); return; } @@ -241,11 +242,12 @@ void EpubReaderActivity::loop() { section->currentPage--; } else { // We don't want to delete the section mid-render, so grab the semaphore - xSemaphoreTake(renderingMutex, portMAX_DELAY); - nextPageNumber = UINT16_MAX; - currentSpineIndex--; - section.reset(); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + nextPageNumber = UINT16_MAX; + currentSpineIndex--; + section.reset(); + } } requestUpdate(); } else { @@ -253,11 +255,12 @@ void EpubReaderActivity::loop() { section->currentPage++; } else { // We don't want to delete the section mid-render, so grab the semaphore - xSemaphoreTake(renderingMutex, portMAX_DELAY); - nextPageNumber = 0; - currentSpineIndex++; - section.reset(); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + nextPageNumber = 0; + currentSpineIndex++; + section.reset(); + } } requestUpdate(); } @@ -325,12 +328,13 @@ void EpubReaderActivity::jumpToPercent(int percent) { } // Reset state so render() reloads and repositions on the target spine. - xSemaphoreTake(renderingMutex, portMAX_DELAY); - currentSpineIndex = targetSpineIndex; - nextPageNumber = 0; - pendingPercentJump = true; - section.reset(); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + currentSpineIndex = targetSpineIndex; + nextPageNumber = 0; + pendingPercentJump = true; + section.reset(); + } } void EpubReaderActivity::onReaderMenuConfirm(EpubReaderMenuActivity::MenuAction action) { @@ -403,24 +407,25 @@ void EpubReaderActivity::onReaderMenuConfirm(EpubReaderMenuActivity::MenuAction break; } case EpubReaderMenuActivity::MenuAction::DELETE_CACHE: { - xSemaphoreTake(renderingMutex, portMAX_DELAY); - if (epub) { - // 2. BACKUP: Read current progress - // We use the current variables that track our position - uint16_t backupSpine = currentSpineIndex; - uint16_t backupPage = section->currentPage; - uint16_t backupPageCount = section->pageCount; + { + RenderLock lock(*this); + if (epub) { + // 2. BACKUP: Read current progress + // We use the current variables that track our position + uint16_t backupSpine = currentSpineIndex; + uint16_t backupPage = section->currentPage; + uint16_t backupPageCount = section->pageCount; - section.reset(); - // 3. WIPE: Clear the cache directory - epub->clearCache(); + section.reset(); + // 3. WIPE: Clear the cache directory + epub->clearCache(); - // 4. RESTORE: Re-setup the directory and rewrite the progress file - epub->setupCacheDir(); + // 4. RESTORE: Re-setup the directory and rewrite the progress file + epub->setupCacheDir(); - saveProgress(backupSpine, backupPage, backupPageCount); + saveProgress(backupSpine, backupPage, backupPageCount); + } } - xSemaphoreGive(renderingMutex); // Defer go home to avoid race condition with display task pendingGoHome = true; break; @@ -458,23 +463,24 @@ void EpubReaderActivity::applyOrientation(const uint8_t orientation) { } // Preserve current reading position so we can restore after reflow. - xSemaphoreTake(renderingMutex, portMAX_DELAY); - if (section) { - cachedSpineIndex = currentSpineIndex; - cachedChapterTotalPageCount = section->pageCount; - nextPageNumber = section->currentPage; + { + RenderLock lock(*this); + if (section) { + cachedSpineIndex = currentSpineIndex; + cachedChapterTotalPageCount = section->pageCount; + nextPageNumber = section->currentPage; + } + + // Persist the selection so the reader keeps the new orientation on next launch. + SETTINGS.orientation = orientation; + SETTINGS.saveToFile(); + + // Update renderer orientation to match the new logical coordinate system. + applyReaderOrientation(renderer, SETTINGS.orientation); + + // Reset section to force re-layout in the new orientation. + section.reset(); } - - // Persist the selection so the reader keeps the new orientation on next launch. - SETTINGS.orientation = orientation; - SETTINGS.saveToFile(); - - // Update renderer orientation to match the new logical coordinate system. - applyReaderOrientation(renderer, SETTINGS.orientation); - - // Reset section to force re-layout in the new orientation. - section.reset(); - xSemaphoreGive(renderingMutex); } // TODO: Failure handling diff --git a/src/activities/reader/KOReaderSyncActivity.cpp b/src/activities/reader/KOReaderSyncActivity.cpp index 54c4dabe..c60fdc97 100644 --- a/src/activities/reader/KOReaderSyncActivity.cpp +++ b/src/activities/reader/KOReaderSyncActivity.cpp @@ -51,18 +51,20 @@ void KOReaderSyncActivity::onWifiSelectionComplete(const bool success) { LOG_DBG("KOSync", "WiFi connected, starting sync"); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = SYNCING; - statusMessage = "Syncing time..."; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = SYNCING; + statusMessage = "Syncing time..."; + } requestUpdate(); // Sync time with NTP before making API requests syncTimeWithNTP(); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - statusMessage = "Calculating document hash..."; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + statusMessage = "Calculating document hash..."; + } requestUpdate(); performSync(); @@ -76,19 +78,21 @@ void KOReaderSyncActivity::performSync() { documentHash = KOReaderDocumentId::calculate(epubPath); } if (documentHash.empty()) { - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = SYNC_FAILED; - statusMessage = "Failed to calculate document hash"; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = SYNC_FAILED; + statusMessage = "Failed to calculate document hash"; + } requestUpdate(); return; } LOG_DBG("KOSync", "Document hash: %s", documentHash.c_str()); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - statusMessage = "Fetching remote progress..."; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + statusMessage = "Fetching remote progress..."; + } requestUpdateAndWait(); // Fetch remote progress @@ -96,19 +100,21 @@ void KOReaderSyncActivity::performSync() { if (result == KOReaderSyncClient::NOT_FOUND) { // No remote progress - offer to upload - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = NO_REMOTE_PROGRESS; - hasRemoteProgress = false; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = NO_REMOTE_PROGRESS; + hasRemoteProgress = false; + } requestUpdate(); return; } if (result != KOReaderSyncClient::OK) { - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = SYNC_FAILED; - statusMessage = KOReaderSyncClient::errorString(result); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = SYNC_FAILED; + statusMessage = KOReaderSyncClient::errorString(result); + } requestUpdate(); return; } @@ -122,18 +128,20 @@ void KOReaderSyncActivity::performSync() { CrossPointPosition localPos = {currentSpineIndex, currentPage, totalPagesInSpine}; localProgress = ProgressMapper::toKOReader(epub, localPos); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = SHOWING_RESULT; - selectedOption = 0; // Default to "Apply" - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = SHOWING_RESULT; + selectedOption = 0; // Default to "Apply" + } requestUpdate(); } void KOReaderSyncActivity::performUpload() { - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = UPLOADING; - statusMessage = "Uploading progress..."; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = UPLOADING; + statusMessage = "Uploading progress..."; + } requestUpdate(); requestUpdateAndWait(); @@ -149,17 +157,19 @@ void KOReaderSyncActivity::performUpload() { const auto result = KOReaderSyncClient::updateProgress(progress); if (result != KOReaderSyncClient::OK) { - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = SYNC_FAILED; - statusMessage = KOReaderSyncClient::errorString(result); - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = SYNC_FAILED; + statusMessage = KOReaderSyncClient::errorString(result); + } requestUpdate(); return; } - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = UPLOAD_COMPLETE; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = UPLOAD_COMPLETE; + } requestUpdate(); } @@ -190,9 +200,10 @@ void KOReaderSyncActivity::onEnter() { auto* self = static_cast(param); // Sync time first syncTimeWithNTP(); - xSemaphoreTake(self->renderingMutex, portMAX_DELAY); - self->statusMessage = "Calculating document hash..."; - xSemaphoreGive(self->renderingMutex); + { + RenderLock lock(*self); + self->statusMessage = "Calculating document hash..."; + } self->requestUpdate(); self->performSync(); vTaskDelete(nullptr); diff --git a/src/activities/settings/ClearCacheActivity.cpp b/src/activities/settings/ClearCacheActivity.cpp index c3dbfbb6..b0f59bbc 100644 --- a/src/activities/settings/ClearCacheActivity.cpp +++ b/src/activities/settings/ClearCacheActivity.cpp @@ -118,9 +118,10 @@ void ClearCacheActivity::loop() { if (state == WARNING) { if (mappedInput.wasPressed(MappedInputManager::Button::Confirm)) { LOG_DBG("CLEAR_CACHE", "User confirmed, starting cache clear"); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = CLEARING; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = CLEARING; + } requestUpdateAndWait(); clearCache(); diff --git a/src/activities/settings/KOReaderAuthActivity.cpp b/src/activities/settings/KOReaderAuthActivity.cpp index fcbba5db..30838bc4 100644 --- a/src/activities/settings/KOReaderAuthActivity.cpp +++ b/src/activities/settings/KOReaderAuthActivity.cpp @@ -14,18 +14,20 @@ void KOReaderAuthActivity::onWifiSelectionComplete(const bool success) { exitActivity(); if (!success) { - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = FAILED; - errorMessage = "WiFi connection failed"; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = FAILED; + errorMessage = "WiFi connection failed"; + } requestUpdate(); return; } - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = AUTHENTICATING; - statusMessage = "Authenticating..."; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = AUTHENTICATING; + statusMessage = "Authenticating..."; + } requestUpdate(); performAuthentication(); @@ -34,15 +36,16 @@ void KOReaderAuthActivity::onWifiSelectionComplete(const bool success) { void KOReaderAuthActivity::performAuthentication() { const auto result = KOReaderSyncClient::authenticate(); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - if (result == KOReaderSyncClient::OK) { - state = SUCCESS; - statusMessage = "Successfully authenticated!"; - } else { - state = FAILED; - errorMessage = KOReaderSyncClient::errorString(result); + { + RenderLock lock(*this); + if (result == KOReaderSyncClient::OK) { + state = SUCCESS; + statusMessage = "Successfully authenticated!"; + } else { + state = FAILED; + errorMessage = KOReaderSyncClient::errorString(result); + } } - xSemaphoreGive(renderingMutex); requestUpdate(); } diff --git a/src/activities/settings/KOReaderSettingsActivity.cpp b/src/activities/settings/KOReaderSettingsActivity.cpp index 4196ef90..ff0fe055 100644 --- a/src/activities/settings/KOReaderSettingsActivity.cpp +++ b/src/activities/settings/KOReaderSettingsActivity.cpp @@ -121,7 +121,6 @@ void KOReaderSettingsActivity::handleSelection() { // Authenticate if (!KOREADER_STORE.hasCredentials()) { // Can't authenticate without credentials - just show message briefly - xSemaphoreGive(renderingMutex); return; } exitActivity(); diff --git a/src/activities/settings/OtaUpdateActivity.cpp b/src/activities/settings/OtaUpdateActivity.cpp index 7e50fa4c..d254f236 100644 --- a/src/activities/settings/OtaUpdateActivity.cpp +++ b/src/activities/settings/OtaUpdateActivity.cpp @@ -20,33 +20,37 @@ void OtaUpdateActivity::onWifiSelectionComplete(const bool success) { LOG_DBG("OTA", "WiFi connected, checking for update"); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = CHECKING_FOR_UPDATE; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = CHECKING_FOR_UPDATE; + } requestUpdateAndWait(); const auto res = updater.checkForUpdate(); if (res != OtaUpdater::OK) { LOG_DBG("OTA", "Update check failed: %d", res); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = FAILED; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = FAILED; + } requestUpdate(); return; } if (!updater.isUpdateNewer()) { LOG_DBG("OTA", "No new update available"); - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = NO_UPDATE; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = NO_UPDATE; + } requestUpdate(); return; } - xSemaphoreTake(renderingMutex, portMAX_DELAY); - state = WAITING_CONFIRMATION; - xSemaphoreGive(renderingMutex); + { + RenderLock lock(*this); + state = WAITING_CONFIRMATION; + } requestUpdate(); }