Fix reverse-read timing and add small-buffer safeguard in AccidentalReverse mode

[ovelhaaa]

Motivation

• Ensure the reverse playback path uses the same channel offset as forward playback and avoid invalid reads when the delay buffer is very small.

Description

• Include params.offsetSamples in the reverse delay calculation so the effective reverse delay matches forward-path channel offsets.
• Add a small-buffer safeguard that detects when delaySize cannot accommodate the minimum reverse window plus guard bands and falls back to a safe read position with zeroed reverse window and counter.
• Rework the reverse-read branch to clamp or reset reverseCounter/reverseWindowSize when the computed read distance falls inside guard regions, and ensure wet and baseReadPos are set consistently.
• Add comments clarifying that the reverse window size is derived inside readReverseWindowed().

Testing

• Built the project with cmake and cmake --build . with no build errors.
• Ran the automated unit test suite with ctest and all tests passed.

---

Codex Task

Summary by CodeRabbit

• Bug Fixes
  • Refined reverse delay processing to better handle edge cases with small delay buffers, ensuring more consistent and predictable reverse playback behavior with improved delay timing precision.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change modifies the reverse-read delay calculation in processChannelFast to include the offset sample parameter and adds a safety fallback that disables reverse windowing when the delay buffer is too small to fit the minimum reverse window between guard bands.

Changes

Reverse-delay windowing

Layer / File(s)	Summary
Reverse-delay windowing with insufficient-region fallback core/src/orbit_delay_core.cpp	Reverse delay calculation now includes params.offsetSamples. New early fallback: when delaySize is too small for minimum reverse window between guard bands, wet is zeroed and baseReadPos computed from guard-band position. Normal path retains distance-backward guard check with baseReadPos computed from reverse-delay + counter position.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• ovelhaaa/orbit-echo#78: Implements matching refactor to incorporate params.offsetSamples into reverse delay and adds insufficient-region fallback with guard/baseReadPos recomputation.
• ovelhaaa/orbit-echo#68: Modifies processChannelFast reverse-read path to clamp reverse read head using guardSamples to avoid read/write proximity crackle.
• ovelhaaa/orbit-echo#62: Changes how reverse delay and read position are computed and handled in the processChannelFast reverse/AccidentalReverse path.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and specifically describes the primary changes: fixing reverse-read timing and adding a small-buffer safeguard in AccidentalReverse mode, which matches the main objectives and code modifications in the changeset.
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.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/refactor-processchannelfast-for-windowed-reading

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Code Review

This pull request updates the OrbitDelayCore::processChannelFast method in the accidental reverse-read path to include channel-dependent offsets (params.offsetSamples) and introduces a safe fallback mechanism when the delay size is too small for the minimum reverse window size. The reviewer identified two issues: first, a 1-sample mismatch between the actual read position and the base read position due to baseReadPos being calculated after reverseCounter is incremented in readReverseWindowed(); second, a dead store where reverseDelaySamples is assigned a value in the fallback branch but never used.

[gemini-code-assist]

In the reverse-read path, baseReadPos is calculated using reverseCounter after it has been incremented inside readReverseWindowed(). This causes a 1-sample mismatch between the actual read position used for the wet signal and the base read position used for the feedback tap mixer. Calculating baseReadPos before calling readReverseWindowed() ensures they are perfectly aligned.

Suggested change

	wet = readReverseWindowed(delay, reverseDelaySamples, writePos, delaySize, invDelaySize, reverseCounter, reverseWindowSize);
	baseReadPos = wrapPosFloat(writePos - reverseDelaySamples + reverseCounter, delaySize, invDelaySize);
	baseReadPos = wrapPosFloat(writePos - reverseDelaySamples + reverseCounter, delaySize, invDelaySize);
	wet = readReverseWindowed(delay, reverseDelaySamples, writePos, delaySize, invDelaySize, reverseCounter, reverseWindowSize);

[gemini-code-assist]

The assignment reverseDelaySamples = guardSamples; is a dead store because reverseDelaySamples is a local variable that is not used anywhere else in this fallback branch. Removing it simplifies the code.

Suggested change

	reverseCounter = 0.0f;
	reverseWindowSize = 0.0f;
	reverseDelaySamples = guardSamples;
	wet = 0.0f;
	baseReadPos = wrapPosFloat(writePos - guardSamples, delaySize, invDelaySize);
	reverseCounter = 0.0f;
	reverseWindowSize = 0.0f;
	wet = 0.0f;
	baseReadPos = wrapPosFloat(writePos - guardSamples, delaySize, invDelaySize);

[coderabbitai]

🧹 Nitpick comments (1)

core/src/orbit_delay_core.cpp (1)

725-726: 💤 Low value

Consider extracting the minimum window constant to avoid duplication.

kMinReverseWindowSamples (8.0f) duplicates kMinWindowSamples inside readReverseWindowed() (line 111). If either value changes independently, the fallback threshold here could become inconsistent with the actual minimum window enforced during reverse reading.

Suggested approach

Define a shared constant (e.g., in the anonymous namespace or as a class static) and reference it in both locations:

// In anonymous namespace near top of file
constexpr float kMinReverseWindowSamples = 8.0f;

Then use it in both readReverseWindowed() and in the fallback check.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/src/orbit_delay_core.cpp` around lines 725 - 726, Define a single shared
minimum-window constant and use it in both places to avoid duplication: replace
the local constexpr kMinReverseWindowSamples and the hardcoded kMinWindowSamples
in readReverseWindowed() with a single constant (e.g., constexpr float
kMinWindowSamplesShared or a static member) declared in the anonymous namespace
or as a class static, then update both the fallback check (the if using
kMinReverseWindowSamples) and the readReverseWindowed() logic to reference that
shared symbol so both use the same minimum value.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@core/src/orbit_delay_core.cpp`:
- Around line 725-726: Define a single shared minimum-window constant and use it
in both places to avoid duplication: replace the local constexpr
kMinReverseWindowSamples and the hardcoded kMinWindowSamples in
readReverseWindowed() with a single constant (e.g., constexpr float
kMinWindowSamplesShared or a static member) declared in the anonymous namespace
or as a class static, then update both the fallback check (the if using
kMinReverseWindowSamples) and the readReverseWindowed() logic to reference that
shared symbol so both use the same minimum value.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 243a9a29-7b80-49b9-9601-583af82a5f62

📥 Commits

Reviewing files that changed from the base of the PR and between 1c045b4 and a040a37.

📒 Files selected for processing (1)

• core/src/orbit_delay_core.cpp