Skip to content

fix: open write handles O_RDWR so read-during-build works - #27

Open
winst0niuss wants to merge 1 commit into
crosspoint-reader:mainfrom
winst0niuss:fix/open-for-write-rdwr
Open

fix: open write handles O_RDWR so read-during-build works#27
winst0niuss wants to merge 1 commit into
crosspoint-reader:mainfrom
winst0niuss:fix/open-for-write-rdwr

Conversation

@winst0niuss

Copy link
Copy Markdown

Entering a not-yet-paginated chapter renders a blank page. Going back to the previous chapter and forward again shows the text. Only the simulator is affected — the same firmware is fine on device.

Cause

openFileForWrite opens the file write-only, while the device SDK opens it read-write:

flags
SDCardManager::openFileForWrite (device) O_RDWR | O_CREAT | O_TRUNC
HalStorage::openFileForWrite (simulator, before this PR) O_WRONLY | O_CREAT | O_TRUNC

Firmware reads back from a write handle while it is still open. Section::loadPageDuringBuild does exactly that — while a section .bin is being built it seeks into that same handle to deserialize a page it has already written, then restores the write cursor:

// The .bin is open O_RDWR for the build. Read the already-written page, then restore
// the write cursor so the next onPageComplete keeps appending where it left off.
const uint32_t writePos = file.position();
file.seek(pos);
auto p = Page::deserialize(file);
file.seek(writePos);

On a write-only fd that read fails, Page::deserialize yields nothing, and the reader paints an empty page.

The recovery path explains why re-entry works: once the build has finalized, Section::loadPage routes to loadPageAt, which opens its own handle through openFileForRead (O_RDONLY) and reads normally.

Reproducing

Open a book with chapters long enough that pagination does not finish instantly, and page forward into a fresh chapter. With a warm cache the build is already done, so clear fs_/.crosspoint/epub_* first.

First entry — blank, and the page counter is prefixed with ~ because the section is still being laid out:

Blank page on first entry into a chapter, status bar reads ~1/64

Same chapter after going back and forward again — the build has finished, the count is exact, the text is there:

Same chapter renders correctly on second entry, status bar reads 2/70

Logs show the build being cut short on first entry (pages 0-7 laid out, then a render), and completing on the second ([EHP] Time to parse and build pages: 186 ms, pages 0-69).

Fix

One line, plus a comment recording why the mode matters. Verified by hand: with the fix, a fresh chapter renders its text on first entry.

This may be the remaining half of the std::fstream bugs listed in .claude/CONTEXT-sim-notes.md under "Ebook reader showed nothing on first press (and 'double press required' symptom)" — those were fixed by moving HalFile::Impl to POSIX fds, but the write-only open mode stayed.

SDCardManager::openFileForWrite on device opens O_RDWR | O_CREAT | O_TRUNC.
The simulator used O_WRONLY, so any firmware path that reads back from a
still-open write handle got EBADF.

Section::loadPageDuringBuild does exactly that: while a section .bin is being
built it seeks into that same handle to deserialize an already-written page.
Under O_WRONLY the read fails and the reader renders a blank page, which shows
up as an empty screen on first entry into a chapter. Re-entering the chapter
works because the build has finished by then and pages are served from a
separate read handle.
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 703ba66f-0bc4-4bf2-bb8c-122c751ecc44

📥 Commits

Reviewing files that changed from the base of the PR and between ed31bd9 and 9b1bece.

📒 Files selected for processing (1)
  • src/HalStorage.cpp
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (3)
src/Hal*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

Each simulator Hal*.cpp/.h must preserve the corresponding firmware HAL class's public surface. When firmware adds a HAL method, add a matching stub—usually a no-op—with the exact signature; do not invent public HAL methods.

Files:

  • src/HalStorage.cpp
src/HalStorage.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Use POSIX file descriptors rather than std::fstream for storage; keep simulated paths under ./fs_, map SD /books/ to ./fs_/books/, and skip directory entries beginning with ..

Files:

  • src/HalStorage.cpp
src/**/*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

When adding an Arduino or ESP-IDF symbol, add the minimum matching stub to the corresponding header under src/, match the upstream signature, and return a sensible default.

Files:

  • src/HalStorage.cpp
🔇 Additional comments (1)
src/HalStorage.cpp (1)

446-451: LGTM!


📝 Walkthrough

Walkthrough

HalStorage::openFileForWrite now opens files with read/write access, creation enabled, and truncation enabled. Its signature and return behavior remain unchanged.

Changes

Storage access

Layer / File(s) Summary
Update file open mode
src/HalStorage.cpp
openFileForWrite now uses O_RDWR | O_CREAT | O_TRUNC instead of write-only access.

Estimated code review effort: 1 (Trivial) | ~2 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the change to use read-write handles for read-during-build behavior.
Description check ✅ Passed The description directly explains the simulator bug, its cause, the fix, and the observed behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant