Skip to content

fix: two HAL stub gaps blocking firmware builds against the simulator - #30

Open
sfoulad wants to merge 2 commits into
crosspoint-reader:mainfrom
sfoulad:fix/hal-stub-gaps
Open

fix: two HAL stub gaps blocking firmware builds against the simulator#30
sfoulad wants to merge 2 commits into
crosspoint-reader:mainfrom
sfoulad:fix/hal-stub-gaps

Conversation

@sfoulad

@sfoulad sfoulad commented Aug 7, 2026

Copy link
Copy Markdown

What

Two build-blocking gaps between this simulator's HAL stubs and current CrossPoint firmware, found while running the simulator against sfoulad/midad-by-foulad, an Arabic-first CrossPoint Reader fork.

Both were previously carried as uncommitted local patches to the checked-out .pio/libdeps/simulator/ copy in that fork, re-applied by hand after every pio clean / libdeps refetch, because there was no upstream fix to depend on. Landing them here removes that maintenance burden for us and for any other fork on a current firmware checkout.

1. HalDisplay::displayBuffer arity (src/HalDisplay.h, src/HalDisplay.cpp)

Current CrossPoint firmware's HalDisplay::displayBuffer takes a third argument, forceCleanBaseOnHalf, controlling whether a HALF_REFRESH forces a base-clearing waveform pass before the requested refresh. This simulator's stub still only takes (mode, turnOffScreen), so any firmware built against the current API fails to compile against the simulator at all — this is a hard build blocker, not just a fidelity gap.

Fix: add the third parameter to both the header and implementation and no-op it. The simulator has no e-ink half-refresh base to keep clean, so accepting-and-ignoring the flag is correct — consistent with how this stub already treats other refresh-quality hints.

2. Missing HTTP_METHOD_DELETE (src/esp_http_client.h)

The esp_http_client_method_t stub stops at HTTP_METHOD_PUT. Real ESP-IDF's esp_http_client_method_t defines HTTP_METHOD_DELETE (see components/esp_http_client/include/esp_http_client.h upstream). Firmware HTTP client code that issues a DELETE request (e.g. a device sign-out endpoint) fails to compile against this stub.

Fix: add HTTP_METHOD_DELETE to the enum and the matching case in methodName(). esp_http_client_open() already performs the whole request in one call for methods with no body, which suits DELETE naturally.

Not included: the O_WRONLY vs O_RDWR fidelity gap

We were also carrying a local patch for HalStorage::openFileForWrite opening O_WRONLY instead of O_RDWR (breaking read-back through an open write handle mid-build). While preparing this PR I found that's already fixed upstream in #27 (9b1bece) — no action needed there.

Testing

Both changes are mechanical HAL-stub signature/enum additions with no behavioral branching beyond the no-op default. Verified:

  • g++ -fsyntax-only -std=c++2a against both modified headers in isolation (clean, no repo build toolchain available in this environment to do a full firmware+simulator build here).
  • The exact same two patches have been running against a git-HEAD checkout of this repo in the midad-by-foulad fork's local build for several weeks with no issues.
  • Manual review confirms no other call site in this repo depends on the old 2-arg displayBuffer signature or the old esp_http_client_method_t enum size.

sfoulad added 2 commits August 8, 2026 01:00
CrossPoint firmware's HalDisplay::displayBuffer takes a third
forceCleanBaseOnHalf argument controlling whether a HALF_REFRESH forces a
base-clearing waveform pass. Firmware that calls the 3-arg form fails to
compile against this simulator's 2-arg stub, so the simulator cannot build
at all until the signature matches.

The simulator has no e-ink half-refresh base to keep clean, so the flag is
accepted and ignored -- same behavior as every other refresh-quality hint
this stub already no-ops.
esp_http_client_method_t stops at HTTP_METHOD_PUT, so firmware HTTP client
code that issues a DELETE request (e.g. a device sign-out endpoint) fails
to compile against this stub. Real ESP-IDF's esp_http_client_method_t
defines HTTP_METHOD_DELETE; add it here plus the matching methodName()
case. esp_http_client_open() already performs the whole request in one
call for methods with no body, which suits DELETE.
@coderabbitai

coderabbitai Bot commented Aug 7, 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: 71f4bdf5-0218-4b65-b126-d563c1438393

📥 Commits

Reviewing files that changed from the base of the PR and between 86ac02d and a78a83a.

📒 Files selected for processing (3)
  • src/HalDisplay.cpp
  • src/HalDisplay.h
  • src/esp_http_client.h
📜 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/HalDisplay.h
  • src/HalDisplay.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/HalDisplay.h
  • src/esp_http_client.h
  • src/HalDisplay.cpp
src/HalDisplay.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

src/HalDisplay.cpp: Only the SDL main thread may call SDL rendering functions. refreshDisplay should prepare the framebuffer and signal pendingPresent; presentIfNeeded, called from simulator_main, performs SDL upload and presentation.
Keep display orientation rotation consistent between the firmware renderer and the simulator's SDL_RenderCopyEx transformation; preserve the landscape-shaped, centre-offset destination rectangle.
Set SDL_HINT_RENDER_SCALE_QUALITY=1 before SDL_CreateTexture, and use SDL_WINDOW_ALLOW_HIGHDPI with SDL_RenderSetLogicalSize for correct HiDPI and dithering behavior.

Files:

  • src/HalDisplay.cpp
🔇 Additional comments (3)
src/esp_http_client.h (1)

16-16: LGTM!

Also applies to: 76-77

src/HalDisplay.cpp (1)

358-361: LGTM!

src/HalDisplay.h (1)

44-45: 🗄️ Data Integrity & Integration

Verify the firmware HAL signature before adding the third parameter.

src/HalDisplay.h now declares displayBuffer(..., bool forceCleanBaseOnHalf), but src/EInkDisplay.h still declares displayBuffer(RefreshMode mode, bool turnOffScreen) and no in-repo simulator call site uses the new argument. If this simulator HAL must match the firmware Display::displayBuffer surface, add the same parameter and default everywhere; otherwise confirm this stub is intentionally outside that contract.


📝 Walkthrough

Walkthrough

The change extends displayBuffer with a clean-base option that the simulator ignores. It also adds HTTP_METHOD_DELETE and maps it to the DELETE method name.

Changes

Display buffer API

Layer / File(s) Summary
Display buffer parameter
src/HalDisplay.h, src/HalDisplay.cpp
displayBuffer accepts forceCleanBaseOnHalf, which defaults to true. The simulator ignores the parameter before refreshing the display.

HTTP DELETE method

Layer / File(s) Summary
DELETE method enum and mapping
src/esp_http_client.h
The HTTP method enum includes HTTP_METHOD_DELETE. The method-name helper returns "DELETE" for this value.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: uxjulia, lpla, wutofu

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the two HAL stub updates that resolve simulator firmware build blockers.
Description check ✅ Passed The description directly explains both changes, their build impact, implementation, and testing.
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