fix: use RAII render lock everywhere by ngxson · Pull Request #916 · crosspoint-reader/crosspoint-reader

ngxson · 2026-02-16T10:58:51Z

Summary

Follow-up to #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.

ngxson · 2026-02-16T10:59:27Z

src/activities/settings/KOReaderSettingsActivity.cpp

    // Authenticate
    if (!KOREADER_STORE.hasCredentials()) {
      // Can't authenticate without credentials - just show message briefly
-      xSemaphoreGive(renderingMutex);


Note: this is a stray mutex unlock, left-over from the original PR

daveallie

Looks good to me, just going to quickly test on device too

daveallie · 2026-02-16T12:23:39Z

@coderabbitai review

coderabbitai · 2026-02-16T12:23:49Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2026-02-16T12:25:47Z

📝 Walkthrough

Walkthrough

The PR systematically replaces explicit semaphore-based locking (xSemaphoreTake/xSemaphoreGive) with RAII-style RenderLock scopes across seven activity files. State updates and rendering-related critical sections now use scoped lock objects instead of manual semaphore management, with one notable exception where a semaphore release was removed.

Changes

Cohort / File(s)	Summary
Simple RenderLock Replacements `src/activities/network/WifiSelectionActivity.cpp`, `src/activities/settings/ClearCacheActivity.cpp`	Replaces single semaphore block (xSemaphoreTake/xSemaphoreGive) with scoped RenderLock around state transitions. Minimal scope changes (~4 lines each).
Complex Reader Activity Refactorings `src/activities/reader/EpubReaderActivity.cpp`, `src/activities/reader/KOReaderSyncActivity.cpp`	Multiple semaphore blocks across diverse code paths replaced with RenderLock scopes. Covers navigation, caching, sync operations, and state updates. EpubReaderActivity affects loop() navigation branches and jumpToPercent(); KOReaderSyncActivity affects state transitions, sync stages, and progress handling.
Settings Activity Refactorings `src/activities/settings/KOReaderAuthActivity.cpp`, `src/activities/settings/OtaUpdateActivity.cpp`	Replaces semaphore blocks around state and message updates with RenderLock scopes. Covers authentication flow and OTA state transitions (CHECKING_FOR_UPDATE, FAILED, etc.).
Lock Management Anomaly `src/activities/settings/KOReaderSettingsActivity.cpp`	Removes xSemaphoreGive(renderingMutex) call in handleSelection() when no credentials exist, potentially leaving the mutex unreleased on this error path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

refactor: move render() to Activity super class, use freeRTOS notification #774: Directly tied as the source of the RenderLock API that replaces per-activity semaphore take/give patterns throughout this PR.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Merge Conflict Detection	⚠️ Warning	⚠️ Unable to check for merge conflicts: Failed to fetch base branch: From https://github.com/crosspoint-reader/crosspoint-reader ! [rejected] master -> master (non-fast-forward) + `0bc6747...6702060` master -> origin/master (forced update)

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main objective of the PR: replacing explicit semaphore locking with RAII-style RenderLock across multiple files. This is the core change across all modified files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (3)

src/activities/reader/KOReaderSyncActivity.cpp (1)
203-206: Inconsistent locking pattern: manual semaphore still used here.

This code path still uses xSemaphoreTake/xSemaphoreGive while the rest of this file and the PR migrate to RenderLock. For consistency, consider using RenderLock here as well.
♻️ Suggested refactor for consistency
-          xSemaphoreTake(self->renderingMutex, portMAX_DELAY);
-          self->statusMessage = "Calculating document hash...";
-          xSemaphoreGive(self->renderingMutex);
+          {
+            RenderLock lock(*self);
+            self->statusMessage = "Calculating document hash...";
+          }
src/activities/reader/EpubReaderActivity.cpp (2)

410-431: Consider narrowing the lock scope around I/O operations.

The lock is held during epub->clearCache() and epub->setupCacheDir(), which involve file system I/O. While the atomicity across these operations is desirable, holding the render lock during I/O could potentially cause UI stutter if the render task needs to update. Since this path immediately triggers pendingGoHome, the impact is likely minimal, but it's worth noting.

466-483: Lock scope includes file I/O.

The RenderLock is held during SETTINGS.saveToFile(), which performs file I/O. Similar to the DELETE_CACHE case, this could potentially block the render task. However, the scope ensures that the orientation change, setting persistence, and section reset occur atomically, which may be the intended behavior.

daveallie · 2026-02-16T12:28:55Z

@ngxson CodeRabbit found one additional use of semaphores directly in src/activities/reader/KOReaderSyncActivity.cpp, can this also utilise RenderLock or was there a reason to leave it as is?

It could be:

{
  RenderLock lock(*self);
  self->statusMessage = "Calculating document hash...";
}

right?

ngxson · 2026-02-16T12:47:01Z

yeah good catch, I fixed in the last commit

## Summary Follow-up to crosspoint-reader#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.

fix: use RAII render lock everywhere

dcccc37

ngxson commented Feb 16, 2026

View reviewed changes

ngxson requested a review from a team February 16, 2026 10:59

daveallie reviewed Feb 16, 2026

View reviewed changes

coderabbitai bot reviewed Feb 16, 2026

View reviewed changes

KOReaderSyncActivity

c923319

daveallie approved these changes Feb 16, 2026

View reviewed changes

daveallie merged commit 3d47c08 into crosspoint-reader:master Feb 16, 2026
6 checks passed

daveallie mentioned this pull request Feb 19, 2026

chore(release): 1.1.0 Release Candidate #992

Merged

This was referenced Feb 21, 2026

refactor: implement ActivityManager #1016

Merged

perf: Micro-optimizations to eliminate value copies #972

Closed

feat: Auto Page Turn for Epub Reader #1219

Merged

fix: ActivityManager tweaks #1220

Merged

This was referenced Mar 1, 2026

feat: Add push progress and sleep option #1090

Open

chore: remove rendundant xTaskCreate #1264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: use RAII render lock everywhere#916

fix: use RAII render lock everywhere#916
daveallie merged 2 commits intocrosspoint-reader:masterfrom
ngxson:xsn/raii_render_lock

ngxson commented Feb 16, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

ngxson Feb 16, 2026 •

edited

Loading

Uh oh!

daveallie left a comment

Uh oh!

daveallie commented Feb 16, 2026

Uh oh!

coderabbitai bot commented Feb 16, 2026

Uh oh!

coderabbitai bot commented Feb 16, 2026

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

daveallie commented Feb 16, 2026 •

edited

Loading

Uh oh!

ngxson commented Feb 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ngxson commented Feb 16, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

AI Usage

Summary by CodeRabbit

Release Notes

Uh oh!

ngxson Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daveallie left a comment

Choose a reason for hiding this comment

Uh oh!

daveallie commented Feb 16, 2026

Uh oh!

coderabbitai bot commented Feb 16, 2026

Uh oh!

coderabbitai bot commented Feb 16, 2026

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

daveallie commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngxson commented Feb 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ngxson commented Feb 16, 2026 •

edited by coderabbitai bot

Loading

ngxson Feb 16, 2026 •

edited

Loading

daveallie commented Feb 16, 2026 •

edited

Loading