feat(sensor): add experimental RR_INTERVAL beat-to-beat pathway

[tobymurray]

feat(sensor): experimental RR_INTERVAL beat-to-beat pathway

An R-R interval is the gap between one heartbeat and the next. RMSSD, SDNN and DFA-alpha1 are computed from those gaps, and readiness, recovery and training-threshold metrics are built on those in turn.

Nothing in the SDK carries them. HEART_RATE and HEART_RATE_EX both carry a rate. A Bluetooth strap already sends the intervals to the watch in the same 0x2A37 notification the kernel reads for BPM, where the decoder drops them, so an app has nothing to compute HRV from. This adds the SDK side of a sensor type that keeps them: SDK::Sensor::Type::RR_INTERVAL (0x44) and a parser for it, opt-in and experimental. There is no firmware producer yet, so 0x44 is not stable until one has been written against it.

None of this is Polar specific. R-R intervals are a standard field of the Bluetooth Heart Rate Service, so any compliant strap provides them the same way.

The firmware ask is to stop discarding the R-R field; the kernel already parses 0x2A37 for BPM and the intervals are in the same bytes. One warning for whoever writes that. The R-R array is the last field of the notification and its offset moves, because flags bit 3 inserts a two byte energy-expended field ahead of it. Get that wrong and every interval is misread on a strap that reports it, and the misread values still look like plausible intervals. ExternalSensors.md has the layout.

---

Decisions

The header keeps the consumer contract, ExternalSensors.md owns the producer half, and the reasoning below lives here rather than in either. In short:

Two producers are in view, a strap and a wrist-optical path, and the contract is shaped for both rather than for one. That is why the continuity budget does not key on Source, and why field [3] is reserved for a graded per-beat confidence rather than defined now.

One interval per frame. Data has no length field, so a notification carrying several intervals arrives as several frames.

rr_ms is float milliseconds; source and flags are u32, since a bitmask carried in a float is only exact below 2^24.

The frame timestamp is the beat instant its interval ends on. This is the one decision that cannot be relaxed later, because a consumer relying on true beat instants breaks the moment a producer stops supplying them.

Loss detection is an exported operation, checkContinuity(), rather than an exported threshold. The comparison has to be signed and in microseconds or a reordered pair wraps to an enormous forward gap, which is the shape every consumer given a bare number would get wrong independently. It also deletes the overloads that would widen a narrower stamp: getTimestamp() sits next to getTimestampUs() and differs by a factor of 1000, so passing the wrong one returns a confident GAP on a contiguous pair with no diagnostic, and no runtime guard can catch that. How far that deletion reaches is under Open.

Artefact policy belongs to the consumer, which is what ARTIFACT_SUSPECT is for. No sequence number: it would be a second loss-detection mechanism whose obligation falls on a producer that does not exist, when DISCONTINUITY already covers the loss a producer knows about. Appending one stays safe, so that is revisitable.

Pivots

kContiguityToleranceMs is gone, replaced by checkContinuity() and a derived budget. Not because 250 ms was wrong, but because this repository cannot defend it. It was sized partly on the connection interval our accessory central negotiates, and the whole accessory surface here is Kind, State and a device name. @rryles holds a figure this repository does not, and if it should be applied, the right shape is the two-argument overload called from the layer that knows the link.

There is one continuity budget rather than one per stamping class. A tighter budget for DETECTOR_STAMPED was written and then removed: its dominant term is detector-to-stamp jitter, nothing here can measure it, and neither producer in view sets the flag. The magnitude would have been a guess that producers were then required to hold under, and a producer that exceeded it would have every window discarded, which is the false positive this contract treats as the expensive error. The flag keeps its bit and its meaning, so a second budget lands with the detector that can size one, and continuityToleranceMs() stays a member function so that costs no consumer a call site.

The 170 bpm sizing basis became kGuaranteedDetectableBpm at 200 bpm, pinned to 220-minus-age at age 20.

The minimum delivery period is restored as a producer obligation, and is now anchored to the producer's own shortest interval rather than to the simulator's rate adapter, so it carries no borrowed authority. A lost beat reads CONTIGUOUS once the whole missing interval fits inside the budget, which the margin puts at a rate above kGuaranteedDetectableBpm rather than at it; a stated residual with a test.

You asked whether the producer half belonged in ExternalSensors.md beside the 0x2A37 decode notes, leaving the header with the consumer contract and a pointer. I said no. That was the wrong call and I have reversed it.

Concentrating the prose in the header did not prevent drift; it put the copy that rots into the artifact that ships, and the duplication it was meant to avoid existed anyway. So the split is the one you proposed: the header keeps the consumer contract, ExternalSensors.md owns the producer half beside the 0x2A37 walk it already owned, and this description carries the reasoning.

Open

• Source::ECG = 3 is not held by a static_assert. The kernel HR arbiter defines these values and has none for ECG, unlike the other three. If the arbiter later assigns 3 to something else, every frame already written is mislabelled, so this wants settling before anything ships against 0x44.
• kLostBeatMarginFraction is chosen with no measurement behind its magnitude, and it may point the wrong way. A larger margin narrows the budget and so buys more false positives, and HRV has largely collapsed by 200 bpm, so a smaller figure looks more consistent with the rest of the file. Beat-to-beat variation measured near the detection ceiling would settle it.
• The simulator's missing event-based bypass is a TODO (removed from the scope of this PR)
• The compile-time refusal of a wrong stamp is narrower than it looks. The deleted overloads are bypassed by a braced-init-list, which is a non-deduced context, so checkContinuity({getTimestamp()}) compiles and reads a confident GAP on a contiguous pair; and by a pointer-to-member, which names the surviving overload directly. Widening the overload set closes one route and opens another, because a predicate can only refuse the spellings someone thought of. Closing the class wants a tagged microsecond instant instead, and the only callers today are this file's own tests, so it is cheaper now than at any later point.
• Why a new type rather than HEART_BEAT (0x40): it is declared but has no parser and no field layout, and differencing beat-peak instants would put the interval reconstruction, and the loss detection with it, in every consumer.
• HeartRateEx::getSource() has the same unguarded float narrowing this fixed

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds experimental RR interval sensor support. The change adds a sensor type, an RrInterval parser, BPM conversion, metadata and flag accessors, timestamp handling, producer documentation, simulator delivery documentation, and host tests.

Changes

RR interval support

Layer / File(s)	Summary
RR interval type and parser contract Libs/Header/SDK/SensorLayer/SensorTypes.hpp, Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp, Docs/SensorsLayer.md, Docs/ExternalSensors.md	Adds RR_INTERVAL and defines RR fields, sources, flags, timestamps, delivery stride, producer requirements, and validation rules.
RR interval parsing behavior Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp, Tests/Host/sensorlayer/RrIntervalParser_test.cpp, Tests/Host/CMakeLists.txt	Validates finite positive RR values, derives BPM, decodes optional sources and flags, handles continuity, and returns guarded values for invalid frames.
Simulator delivery rules and validation Libs/Header/SDK/Simulator/Components/SampleRateAdapter.hpp, Libs/Source/Simulator/Components/SensorDataQueue.cpp, Tests/Host/simulator/SampleRateAdapter_test.cpp, Tests/Host/CMakeLists.txt	Documents timestamp-based microsecond gating and tests decimation formulas, period boundaries, shared timestamps, and repeated timestamps.

Estimated code review effort: 4 (Complex) | ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 32.35% 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
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main change: adding an experimental RR_INTERVAL beat-to-beat sensor pathway.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp`:
- Around line 139-149: In
Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp:139-149,
update getSource() to compare SOURCE as a float against valid Source values and
return UNKNOWN for non-integral, non-finite, negative, or out-of-range metadata
before any narrowing conversion. In the same file at 203-208, update getFlags()
to validate FLAGS as finite, non-negative, and integral before converting it to
an integer type.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 05d6169e-9e7b-456e-b42d-4df7f566a82e

📥 Commits

Reviewing files that changed from the base of the PR and between 2764a3e and af8bb1e.

📒 Files selected for processing (4)

• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp
• Libs/Header/SDK/SensorLayer/SensorTypes.hpp
• Tests/Host/CMakeLists.txt
• Tests/Host/sensorlayer/RrIntervalParser_test.cpp

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp (1)

147-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject fractional source and flag metadata.

1.5f currently becomes Source::OPTICAL and DISCONTINUITY. Metadata values must be exact discrete values before conversion.

• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp#L147-L155: compare s directly with the valid Source float values. Do not narrow it first.
• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp#L216-L220: after the safe range check, reject a value when conversion to uint32_t does not round-trip to the original float.
• Tests/Host/sensorlayer/RrIntervalParser_test.cpp#L106-L128: add 1.5f to bad and expect UNKNOWN with all flags clear.

🤖 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 `@Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp`
around lines 147 - 155, Reject fractional metadata values before narrowing: in
Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp lines
147-155, update the source parsing switch to compare s directly against the
valid Source float values; in lines 216-220, after the safe range check, reject
values whose uint32_t conversion does not round-trip to the original float. In
Tests/Host/sensorlayer/RrIntervalParser_test.cpp lines 106-128, add 1.5f to the
invalid inputs and verify the result is UNKNOWN with all flags clear.

🤖 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.

Inline comments:
In `@Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp`:
- Around line 112-134: Update SensorDataParserRrInterval::isDataValid() to
require the RR_MS field to be present, finite, and strictly greater than zero,
using the appropriate standard finite-value check. Ensure getRrMs(), getBpm(),
and timestamp accessors rely on this validity result so malformed frames expose
invalid/default values, and add coverage for NaN, infinity, zero, and negative
RR_MS inputs.

In `@Tests/Host/sensorlayer/RrIntervalParser_test.cpp`:
- Around line 21-25: Update the RrData storage in the RrData::data() path so it
returns an actually constructed SDK::Sensor::Data object under the existing
C++17 test target, using placement construction and the required lifetime-safe
access. Preserve the aligned buffer layout and operator-> behavior.

---

Duplicate comments:
In `@Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp`:
- Around line 147-155: Reject fractional metadata values before narrowing: in
Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp lines
147-155, update the source parsing switch to compare s directly against the
valid Source float values; in lines 216-220, after the safe range check, reject
values whose uint32_t conversion does not round-trip to the original float. In
Tests/Host/sensorlayer/RrIntervalParser_test.cpp lines 106-128, add 1.5f to the
invalid inputs and verify the result is UNKNOWN with all flags clear.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e298c0f8-e727-42df-a880-35c6429251b0

📥 Commits

Reviewing files that changed from the base of the PR and between af8bb1e and c469d04.

📒 Files selected for processing (4)

• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp
• Libs/Header/SDK/SensorLayer/SensorTypes.hpp
• Tests/Host/CMakeLists.txt
• Tests/Host/sensorlayer/RrIntervalParser_test.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• Tests/Host/CMakeLists.txt
• Libs/Header/SDK/SensorLayer/SensorTypes.hpp

[rryles]

Thanks for this, and for framing the open questions so precisely — that made them answerable. All three have answers and they're favourable, so this shouldn't be closed. Details below, then review comments.

Open questions

1. Is the R-R field recoverable before HR-source arbitration collapses it? Yes.

The strap's 0x2A37 notification is decoded in exactly one place kernel-side, and that decoder reads the flags byte and the BPM and simply never looks at the RR-interval field (flags bit 4). The collapse to a single BPM happens inside that parser, not upstream of it — the R-R bytes are fully intact at precisely the seam a producer would hook into.

Extending it means widening the decoded-sample type (today it carries kind/value/trust) or adding a parallel R-R callback, then publishing 0x44 from a new sub-sensor. Nothing structural in the way.

One implementation note for whoever writes it: the decoder currently only knows the 16-bit-value, contact-detected and contact-supported flag bits. A producer must also account for the optional 2-byte energy-expended field (bit 3), which sits before the R-R array when present, or the offsets will be wrong on straps that report it.

2. Per-beat timestamps and decimation. Resolved — and you don't actually need per-beat timestamps.

Your read of the rate adapter is correct, and I measured it rather than trusting the reading: five R-R values sharing one arrival instant, pushed through the periodic path against a 1000 ms listener period, deliver 1 of 5. Exactly the failure you predicted.

But sensors registered as Driver::Mode::EVENT_BASED never take that path. On-device the driver dispatches event-based data straight into each listener's queue, bypassing the rate adapter entirely — specifically because discrete events must not be dropped when two land inside one sample period — and those listeners get a single-slot queue so every frame is delivered immediately. So an RR_INTERVAL producer registered event-based delivers every frame, timestamp collisions or not. The "no-decimation event path" you say the platform needs already exists.

Distinct per-beat timestamps also work as a fallback (5 of 5 delivered), and the adapter is more forgiving than "gates on the frame timestamp" implies — the emit rule passes anything slower than roughly 2x the requested period. Measured against a 1000 ms listener period: 40–90 bpm passes 100%, 120 bpm drops to ~50%, 180 bpm to ~33%. Requesting 500 ms restores 100% at every rate.

Important for your planned simulator replay mock: the SDK simulator does not currently implement that event-based bypass. Simulator/Components/SensorDriver.cpp::pushDataSample() calls pushData() for every listener unconditionally, and its DataQueue has no single-slot case for event-based sensors. So a replay mock built against the simulator today would drop beats in the sim while working correctly on hardware — which would look like the contract failing when it isn't. That's a simulator gap for us to close rather than something you should design around; worth fixing as part of, or just before, the follow-up PR.

3. Who owns the tick to ms conversion? Producer-side, and float milliseconds is safe to pin.

1/1024 s = 125/128 ms = 0.9765625 ms is exactly representable in binary floating point, and n x 125/128 stays exact in float32 across any plausible tick count. Converting producer-side is therefore lossless, and field [0] as float milliseconds is the contract — no silent unit change waiting to happen.

On the premise

One correction worth making before it becomes the quotable justification in the commit history: the "the wrist sensor is below the HRV floor" framing is no longer accurate. Without going into specifics, we're now evaluating a wrist-optical path that would also produce per-beat intervals.

That makes this contract more valuable rather than less. A wire format validated by two independent producers is in far better shape than one designed against a single hypothetical, and it means Source::OPTICAL is a real near-term value rather than a placeholder.

Review comments

Blocking

getFieldsNumber() returning 1 breaks the stride convention. Everywhere else in the SDK this is the full field count, and producers feed it directly into the Driver constructor as the delivery stride — HeartRate returns 2, HeartRateEx returns 7, and the simulator sensors pass it straight through (see SensorPressure.cpp and friends). Returning the minimum means a producer built the conventional way registers a 1-field stride and can then never deliver SOURCE or FLAGS. Suggest returning 3 and keeping the lenient isDataValid() >= 1.

Decide float vs u32 for SOURCE/FLAGS now. Data::Field is a union and DataView already exposes .u32, so reading mData.u[FLAGS] would eliminate the entire NaN/narrowing-guard class your second commit had to add. Mirroring the HeartRateEx all-float convention is defensible and so is the union — but with no producers yet, this is the last moment it's free. After something ships against 0x44 it's a silent ABI break.

Worth adding while it's still free

A graded confidence field at [3]. A realistic optical producer can supply a per-beat confidence value rather than a boolean. Projecting that down onto the 1-bit ARTIFACT_SUSPECT flag discards information that genuinely matters here — DFA-alpha1 is notoriously sensitive to artifact handling, and consumers will want to apply their own threshold rather than inherit the producer's. Cheap now, expensive later.

Smaller points

• getRrMs() is less defended than the metadata fields: a NaN rr_ms passes straight through to consumers. getBpm() is incidentally safe (NaN > 0.0f is false), but a guard or an explicit doc note would make the asymmetry deliberate rather than accidental.
• The tests are genuinely thorough — minimal and full frames, each flag independently, unknown source, NaN/inf/negative/large for both metadata fields, and the zero-field guard. The only gap is a combined-flags case, e.g. DISCONTINUITY | ARTIFACT_SUSPECT.
• __SENSOR_DATA_PARSER_RR_INTERVAL_HPP uses a reserved identifier (leading double underscore). This matches existing SDK precedent so it isn't on you, but we're moving new files off that form — worth doing here since the file is new.

Not yours, but noted

Your second commit's narrowing fix applies verbatim to HeartRateEx::getSource(), which has the same unguarded static_cast<uint8_t> on a float that could be NaN/inf/out-of-range. We'll pick that up separately.

---

Net: the design holds up, the open questions are answered, and the two blocking items are both small. Happy to see this land once those are addressed.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 4

🤖 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.

Inline comments:
In `@Docs/ExternalSensors.md`:
- Around line 119-124: Update the event-based delivery documentation in both
referenced examples to state that a 1 s-period adapter drops beats at or above
120 bpm, matching the strict “more than half a sample period” comparison;
preserve the existing explanation about beats sharing an arrival instant.
- Around line 169-175: Update the guidance in ExternalSensors.md to require
producers to write every registered field, including FLAGS, on every frame.
Clarify that DISCONTINUITY should be set only on the first frame after reconnect
and that subsequent frames should explicitly write the current flags, typically
zero, to prevent the reused Driver sample buffer from retaining the bit.
- Around line 88-89: Update the parser type reference in the RR_INTERVAL
documentation to use the public SDK name, SDK::SensorDataParser::RrInterval,
instead of SensorDataParserRrInterval, keeping the text aligned with the current
SDK header contract.

In `@Docs/SensorsLayer.md`:
- Around line 165-166: In Docs/SensorsLayer.md, add one blank line after each
**Fields**: label before its following table to satisfy markdownlint MD058,
including both affected table sections.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 82f1a02f-82c3-4da0-b98b-ce06973bc67d

📥 Commits

Reviewing files that changed from the base of the PR and between 0d861ae and a5dfc76.

📒 Files selected for processing (6)

• Docs/ExternalSensors.md
• Docs/SensorsLayer.md
• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp
• Libs/Header/SDK/SensorLayer/SensorTypes.hpp
• Tests/Host/CMakeLists.txt
• Tests/Host/sensorlayer/RrIntervalParser_test.cpp

🚧 Files skipped from review as they are similar to previous changes (3)

• Libs/Header/SDK/SensorLayer/SensorTypes.hpp
• Tests/Host/CMakeLists.txt
• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp

[rryles]

Thanks for the revisions — everything I flagged as blocking is properly addressed, and the u32 choice for SOURCE/FLAGS is better than what I asked for: it deletes the whole narrowing class rather than guarding it.

Rather than send another round of requested changes, I've pushed two commits on top (a5dfc766, 10852883) and rebased the branch onto current main. Your commit is otherwise untouched. Splitting what's in them into defects and proposals, since you should feel free to push back on the second group.

Defects — places a producer written faithfully from the header would still have been wrong

• Metadata survived a malformed frame. isDataValid() rejects a non-finite, zero or negative interval, but getSource() and the flag predicates didn't consult it, so a NaN interval arrived with a confident Source::EXTERNAL beside it. The header already claimed such a frame "reads as entirely absent"; now it does.
• The zero-fill invariant is allocation-time, not per-frame. A Driver owns one DataSample and republishes it on every push, so an unwritten field still holds the previous beat's value. The failure this invites is a DISCONTINUITY bit set once on a reconnect and never cleared — which reads downstream as a stream that is never contiguous, exactly the signal that makes an HRV consumer discard its windows. The contract now requires writing every registered field on every frame, flags included.
• t[n] - t[n-1] != rr[n] cannot hold as stated. An R-R value is a whole number of 1/1024 s ticks, so it's fractional in milliseconds — and 125/128 ms isn't a whole number of microseconds either. A contiguous stream fails that equality by construction. It now compares against a tolerance, and back-dating moves to the microsecond domain so the error budget covers transport and nothing else.
• The DELIVERY numbers were wrong, in both your text and my first correction of it. It said a periodic driver drops every beat landing inside one sample period after the first. I replaced that with "every beat above 120 bpm", which was still wrong twice over: 120 bpm is spaced at exactly half a 1 s period and so is affected too, and — more importantly — the adapter doesn't drop those beats at all. It passes roughly one frame per sample period and thins the stream: against a 1 s period, half the beats at 120 bpm and two thirds at 180. That matches the emit rule and reproduces bench measurements taken earlier. Third time lucky; worth a sceptical read.
• "Physiologically impossible values" depended on an unstated min period. At a 1000 ms registration the simulator starts thinning genuine beats from 120 bpm, so the ~250 ms requirement is now written down.

Proposals — your call, and easy to drop

• kContiguityToleranceMs = 250.0f as a pinned constant in the parser class. It's consumer policy living in a header, so it only binds per SDK release; the argument for pinning it is that otherwise every consumer invents its own threshold and they disagree. The number is boxed in from both sides: above the connection interval our accessory central negotiates plus event-drain delay, and below the ~350 ms a lost beat adds at 170 bpm. That window is narrow enough that I've documented the residuals rather than claiming it's clean — a retransmission can breach it, and a beat lost above ~240 bpm hides inside it.
• Field [3] reserved for a graded per-beat confidence. Not defined, just reserved, so nothing else takes the slot.
• No status-only frames — a producer with nothing to report emits nothing, rather than flags with a zero interval.
• "Opt-in companion to HEART_RATE" reframed as a statement about consumers rather than a promise about hardware.

The last two are downstream of the optical question. Without going into specifics: the wrist path we're evaluating produces one interval per beat with a per-beat graded confidence, which is why [3] is worth reserving now — and it may have to reconfigure its front end to detect individual beats at all, possibly serving both types from that one configuration. Your contract accommodates all of that as written; I've only made the assumptions explicit. Designing for two producers rather than one has held up well.

CodeRabbit threads

Three fixed: the parser type name in ExternalSensors.md (it named the file, not the type), the FLAGS wording that read as advice to stop writing the field, and the 120 bpm boundary above.

Two declined, each with the reasoning in the thread rather than here: the RrData test-fixture lifetime (the suggested remedy doesn't cure it, and the fixture is a copy of the pre-existing HrExData one — a real fix is a shared helper across both files), and MD058 blank lines before tables (established style throughout that file, three instances predating this PR, and no lint config or CI job in the repo). Overrule either if you disagree.

One structural question, which I deliberately haven't acted on

About half the header is producer contract — DELIVERY, TIMESTAMP, the per-frame write rule, the min period, the simulator caveat — and the reader who needs it is writing firmware, not consuming the parser. Meanwhile ExternalSensors.md restates much of it: the delivery claim above was wrong in both places and had to be fixed in both, which is how one drifts from the other.

Worth moving the producer half into ExternalSensors.md, where the 0x2A37 decode notes already live, and leaving the header with the consumer contract plus a pointer? I can see the case against — you put the reasoning next to the code so it can't be edited away without someone reading it, which is a real benefit for decisions this expensive to revisit. Your structure, your call.

Where it stands

Tests are at 19 and host-tests is green. New coverage: the metadata gating, a four-field frame (the append-safety the deferred confidence field rests on), the microsecond tail surviving on a valid frame, and the three cases the tolerance has to separate. SensorsLayer.md also gains the HEART_RATE_EX (0x43) row and section it never had, which your new section points at.

Could you review the two commits when you get a chance? I've changed wording you wrote and added contract rules you didn't, so I'd rather you checked them than inherited them — particularly the tolerance constant and the reserved [3]. Anything you disagree with, say so or just revert it; I'd sooner land your version of this than mine.

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@Libs/Source/Simulator/Components/SensorDataQueue.cpp`:
- Around line 142-148: Update Sensor::DataQueue::pushData() to bypass
mSRA.shouldEmit() for event-based RR_INTERVAL frames, preserving every replayed
beat including shared or repeated timestamps. Retain the existing
SampleRateAdapter gating for periodic sensor frames, and add an integration test
covering replayed R-R beats.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 31c7e818-b8ff-472e-a90e-fabac8c8a418

📥 Commits

Reviewing files that changed from the base of the PR and between a5dfc76 and 5a17cac.

📒 Files selected for processing (6)

• Docs/ExternalSensors.md
• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp
• Libs/Header/SDK/Simulator/Components/SampleRateAdapter.hpp
• Libs/Source/Simulator/Components/SensorDataQueue.cpp
• Tests/Host/CMakeLists.txt
• Tests/Host/simulator/SampleRateAdapter_test.cpp

🚧 Files skipped from review as they are similar to previous changes (3)

• Tests/Host/CMakeLists.txt
• Docs/ExternalSensors.md
• Libs/Header/SDK/SensorLayer/DataParsers/SensorDataParserRrInterval.hpp

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Bypass rate gating for event-based RR_INTERVAL frames.

Sensor::DataQueue::pushData() still calls mSRA.shouldEmit() for every frame at Line 152. Replayed R-R frames with shared or repeated timestamps can therefore lose beats. SharedArrivalInstantKeepsOneOfN and RepeatedIdenticalInstantPassesOnce explicitly demonstrate this behavior.

Route event-based RR_INTERVAL frames around SampleRateAdapter. Keep rate gating for periodic sensors. Add a Sensor::DataQueue integration test for replayed R-R beats.

🤖 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 `@Libs/Source/Simulator/Components/SensorDataQueue.cpp` around lines 142 - 148,
Update Sensor::DataQueue::pushData() to bypass mSRA.shouldEmit() for event-based
RR_INTERVAL frames, preserving every replayed beat including shared or repeated
timestamps. Retain the existing SampleRateAdapter gating for periodic sensor
frames, and add an integration test covering replayed R-R beats.

[tobymurray]

I was feeling a bit intimidated at the prospect of getting this all to land in one shot so I pulled the simulator work out as a follow on. I think that helps clarify that this contract has to stand on its own and what commitments can and cannot be met right now.

Your two commits are folded into the squash, co-authored. Answers to what you raised:

Could you review the two commits when you get a chance? I've changed wording you wrote and added contract rules you didn't, so I'd rather you checked them than inherited them, particularly the tolerance constant and the reserved [3].

The reserved [3] stays as you wrote it, and the append-safety is now tested in both directions, a wider stride and a narrower one, so deferring the field is backed rather than assumed. Your per-frame write rule stands as written. Your malformed-frame gating stands and is tested, including that the timestamps go absent with the rest of the frame. The tolerance constant is the one thing I reversed.

kContiguityToleranceMs = 250.0f as a pinned constant in the parser class. The number is boxed in from both sides: above the connection interval our accessory central negotiates plus event-drain delay, and below the ~350 ms a lost beat adds at 170 bpm.

Removed, replaced by checkContinuity() and a derived budget. Not because 250 ms is wrong, but because this repository cannot show its working. The lower bound you gave it is the connection interval, and the whole accessory surface here is Kind, State and a device name. I think this belongs to the firmware and the right shape is the two-argument overload called from the layer that knows the link. Say so and I will wire that up instead. Your 170 bpm upper bound became kMaxDetectableBpm at 200 bpm, pinned to 220-minus-age at age 20.

Worth moving the producer half into ExternalSensors.md, where the 0x2A37 decode notes already live, and leaving the header with the consumer contract plus a pointer? Your structure, your call.

I moved everything into the header to avoid the duplication and risk of drift. I generally come down on the side of preferring inline documentation vs. external documentation (though I acknowledge this one is beefy). ExternalSensors.md now says explicitly that it does not restate the contract, and why. The 0x2A37 walk, the energy-expended trap and the NO_SKIN_CONTACT mapping stay there, since it owns those.

"Physiologically impossible values" depended on an unstated min period. At a 1000 ms registration the simulator starts thinning genuine beats from 120 bpm, so the ~250 ms requirement is now written down.

Restored after I had removed it as belonging to the adapter. You were right that a producer built today falls into the gap regardless of which component owns the rule. It is now a producer obligation anchored to the producer's own output rather than to any platform's behaviour, so it carries no borrowed authority. The header also separates its 240 bpm, an emission ceiling, from kMaxDetectableBpm, a detection ceiling; above the latter a lost beat reads CONTIGUOUS, which is a stated residual with a test.

It passes roughly one frame per sample period and thins the stream: against a 1 s period, half the beats at 120 bpm and two thirds at 180. Third time lucky; worth a sceptical read.

No argument here but I've removed the simulator facet entirely from this PR to try and narrow the focus.

Important for your planned simulator replay mock: the SDK simulator does not currently implement that event-based bypass. That's a simulator gap for us to close rather than something you should design around.

Not designed around at this stage, though I've also done nothing with it. It's captured as a bullet in the "Open questions" section of the description, but that's it so far.

[tobymurray]

I've been churning on this, I think the amount I've put in the code comments really becomes problematic as a contract. I'm going to pivot back the other direction (which you had done) and externalize the documentation.