Fix for spurious multiple tap registration by preventing false releases from registering. Add edge-repeat for page turns.#1
Conversation
GT911 sampling can report no new data when the 0x80 'ready' bit isn't set. Previously gt911_read_point() returned false in this case and touch_task() treated it as a finger-up transition, which could generate spurious release/press cycles and show up as accidental double taps on short presses. Fix this by returning a richer Gt911ReadResult: - NO_UPDATE when no new sample is available (leave gesture state untouched) - NO_TOUCH when a new sample indicates 0 points (true release) - UPDATED when coordinates are read - ERROR on I2C/device failure touch_task() now ignores NO_UPDATE for state transitions but still checks timers, so HOLD can be detected while the finger is down without creating fake releases. I2C errors back off briefly and retry. Also add an optional edge-repeat behavior: touches starting in the left/right third of the screen emit SWIPE_RIGHT/SWIPE_LEFT after 500ms and repeat every 500ms (while remaining within the long-press move threshold), followed by a RELEASE when the finger lifts.
mikaryyn
changed the title
|
Can confirm, I am no longer seeing double page flips with this PR. Thanks! |
|
I confirm too, thanks a lot. But with this PR, the experience is already significantly better, thank you very much.
|
|
Thanks for testing it! I've been also using this for a few days and the navigation works well. Would you consider merging this @ratedcounsel?
I had the same footer problem and was wondering if it is a hardware or driver problem. I sort of "fixed" it by moving the footer text a little farther from the screen's edge. It works but doesn't feel like a proper solution, so I didn't include it in the pull request. Page navigation is fast enough for me in the experimental branch. |
I don't think it's hardware because I don't have any problem using main branch without fastEPD. Page navigation in book is also fast for me, it's just on the library screen (home) to browse books and if I don't select a book for a while it crash and reload. |
You're probably right. Maybe it's a bug in FastEPD.. My padding fix is now in this branch: https://github.com/mikaryyn/EPub-M5Stack-Paper-S3/tree/exp-landscape, if you want to give it a go. It also has support for landscape mode that I'm testing. It's very recent so probably still buggy. |
Code-quality review on 7ec5e93 returned REQUEST CHANGES with one blocker (issue juicecultus#1, ordering bug that made the multi-page persist dead code) plus two cheap defensive fixes (turgu1#2 page_count clamp + turgu1#3 static_assert sizes). All addressed in this commit. BLOCKER FIX: AppController::going_to_deep_sleep called page_cache.stop() BEFORE wake_snapshot.persist(). PageCache::stop sets active_=false and frees the slab; enumerate_complete short-circuits on !active_ and returns 0. So in v2-format land, every persist wrote page_count=1 and zero extra entries — the multi-page format the whole commit implemented was unreachable in deployed code. Fix: reorder going_to_deep_sleep so persist runs first, while the cache is still alive and enumerate_complete can read live entries. The ordering rationale that justified stop-before-persist (lock- order discipline cache → page_locs → epub for teardown) still holds for the stop ordering — page_cache.stop now runs after persist but still before page_locs.stop_document. DEFENSIVE FIXES: - WakeSnapshot::Header and PageEntryMeta now have static_assert on their sizes (32 and 12 bytes respectively). The v2 file format is a contract with on-disk state on user devices; a future field reorder that re-introduces compiler-inserted padding would silently break that contract without tripping the build. The reserved fields' entire job is to keep this invariant; the assert formalizes it. - hydrate_page_cache now clamps hdr.page_count against PageCache::SLOT_COUNT + 1 BEFORE allocating any buffers. A corrupted file claiming page_count=65535 would otherwise trigger a 768 KB malloc, multi-MB seek loop, and 64-bit overflow in offset arithmetic on 32-bit ESP-IDF where long is 32 bits. The clamp short-circuits at the header check before any of that. - offset arithmetic in hydrate_page_cache uses off_t (64-bit on ESP-IDF) instead of long (32-bit on Xtensa). Removes the overflow class entirely, even within the legal page_count range. POLISH: - PageEntryMeta's pad field renamed from reserved0 to pad0 to avoid name shadow with Header::reserved0 (same TU, same name, different sizes — small footgun). - Hydrate's PageEntryMeta array is now stack-allocated (96 bytes max post-clamp) instead of malloced. One fewer allocation, one fewer failure path. - capture() no longer sets cached_header_.page_count = 1 — that field is now set only by persist() once the live cache count is known. A misuse like calling persist() twice without re- capture is now self-rejecting on read (page_count=0 fails the restore check) rather than silently writing stale data. - persist() comment block tightened: now states the actual invariant (mainTask is the only mutator from this point) rather than apologizing about the API design. Phase D (wake-time event routing for the post-wake-pre-app- controller swipe window) intentionally deferred — see scope discussion in the integration branch's status update. The multi-page persist + hydrate flow is the substantive Stage 2 deliverable; Phase D is a polish tier worth waiting for hardware testing feedback before investing.
Tinos symptom fix juicecultus#1 of 4. User-reported: switching to a custom Tinos font caused books to fail to load (had to manually edit PARS to recover). Two coordinated bugs: (1) Fonts::adjust_default_font ran a non-transactional chain of 4 replace() calls; if any failed, font_cache ended up half- swapped (mismatched metrics across styles → broken layout). PARS was persisted before validation → unrecoverable. (2) Stop-document / page-cache quiesce ran AFTER the font swap, not before. Retriever / pre-paint threads hold raw Font* via fonts.get(); a concurrent dereference during the cache mutation UAFs. The pre-refactor flow happened to be safe by accident (stop_document inside is_modified block ran before adjust); preserving that ordering was load-bearing. Fix: - Fonts::adjust_default_font is now transactional + boolean. Pre-loads all 4 styles into Font* temps, validates via is_ready(), commits atomically under the cache mutex on success / deletes temps + leaves cache untouched on failure. The standalone Fonts::replace() (only caller was the pre-transactional version) is removed. - BookParamController: hoists page_locs.stop_document() + page_cache.invalidate_all() to BEFORE adjust_default_font. Validates the load BEFORE persisting PARS; on failure shows msg_viewer alert and reverts local 'font' so the put writes the working previous value. - OptionController: same pattern for the global default font (Config::Ident::DEFAULT_FONT). page_cache.invalidate is correctly absent — option menu entry already closed the file so page_cache is empty. - EPub::open: now logs LOG_E on adjust_default_font failure but doesn't fail the open (rendering with current default beats refusing the book). Comment notes why no quiesce hoist is needed (no live retriever at open time). - Fonts::clear_glyph_caches now acquires the cache mutex (was previously racing with clear(true)). - Per-font cumulative size cap bumped from 300 KB to 2 MB on touch builds (INKPLATE_6PLUS || TOUCH_TRIAL). Comment clarifies this is a boot-time menu-eligibility gate, not a runtime memory ceiling. - Fonts::get() concurrency contract documented in the header (lock-free read; mutators must not race with renderers). Reviews on the post-fixup state: - architecture-guardian: APPROVE - code-quality-reviewer: APPROVE
sleep_screen_viewer.cpp and usb_msc_viewer.cpp each carried a private copy of make_fmt / put_centered / draw_hrule plus a PAD margin constant. The bodies were byte-identical except for one field — line_height_factor (1.1 in sleep_screen, 1.15 in usb_msc). Extract to a shared CenteredLayout namespace: - include/viewers/centered_layout.hpp: declarations - src/viewers/centered_layout.cpp: definitions The line_height_factor difference is now a parameter to make_fmt; each viewer keeps its tuned value as a private constant alongside its other styling. PAD = 52 is exposed as CenteredLayout::PAD (sleep_screen still uses it directly for the progress-bar geometry at lines ~315-316). Future full-screen text viewers (an "About" / firmware-version screen, a fatal-error screen) can reuse these helpers instead of copy-pasting a third time. Per audit 03-viewers.md finding juicecultus#1 (byte-identical helpers); 30-min mechanical refactor as estimated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Audit 05-code-quality.md finding juicecultus#1: three near-identical Page:: Format builders existed at: - src/viewers/book_viewer.cpp:83 (BookViewer::build_page_at) - src/models/page_locs.cpp:849 (PageLocs::build_page_locs) - src/models/page_cache.cpp:706 (page_cache pre-paint render) The page_cache.cpp comment explicitly acknowledged the duplication ("Kept in lockstep manually rather than factored out"). PR1 (today) already had to fix a silent page_bottom formula drift that resulted from this triple-coupling — so the duplication has actively hurt us once. Adds Page::make_body_format(font_idx, font_size, screen_top, screen_bottom). The four variable inputs are exactly the per-call parts; everything else (left/right margins 10 px, line_height_ factor 0.95, align LEFT, etc.) is fixed in the factory. All three sites collapse from ~22 lines of struct literal to a one-line factory call. Comment in page_cache.cpp updated to point at the factory and reference the audit finding instead of admitting the duplication. No behavior change — values match exactly what each site built before. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
3 participants
GT911 sampling can report no new data when the 0x80 'ready' bit isn't set. Previously gt911_read_point() returned false in this case and touch_task() treated it as a finger-up transition, which could generate spurious release/press cycles and show up as accidental double taps on short presses.
Fix this by returning a richer Gt911ReadResult:
touch_task() now ignores NO_UPDATE for state transitions but still checks timers, so HOLD can be detected while the finger is down without creating fake releases. I2C errors back off briefly and retry.
Also add an optional edge-repeat behavior: touches starting in the left/right third of the screen emit SWIPE_RIGHT/SWIPE_LEFT after 500ms and repeat every 500ms (while remaining within the long-press move threshold), followed by a RELEASE when the finger lifts.