# Phase 8: Robustness and Logging - Research

**Researched:** 2026-03-22
**Domain:** C++ driver hardening, defensive programming, SteamVR DriverLog
**Confidence:** HIGH

## Summary

Phase 8 is a hardening pass on an existing, functionally complete C++ SteamVR driver. The codebase already has most error handling and logging in place from prior phases. The work is primarily an audit-and-fill-gaps exercise: verify the graceful degradation path is complete, add transition logging, ensure all error paths have log statements, and add null guards at key boundaries.

The existing code is well-structured with clear patterns (C-style error returns, lock-free atomics, DriverLog with prefix convention). No new libraries or architectural changes are needed. The research focuses on identifying specific gaps in the current code that need attention.

**Primary recommendation:** Audit existing code for gaps rather than rewriting -- most robustness infrastructure already exists from prior phases.

<user_constraints>
## User Constraints (from CONTEXT.md)

### Locked Decisions
- HID open failure already returns VRInitError_None (Phase 2) -- verify this path is complete
- When HID is unavailable, proximity features silently disabled: no SetBoolProperty calls, no RunFrame polling
- Driver must still create named pipe and respond to status commands (reporting "HID: not connected")
- Log on-head/off-head transitions with DriverLog: "Proximity: person_detected changed to %s" (true/false)
- Log only on state CHANGE, not every RunFrame poll -- use existing state-change guard pattern from Phase 3.1
- No raw value logging at default verbosity -- that's already behind log_verbosity from Phase 7
- Maintain existing ad-hoc prefix convention (HID:, Pipe:, Proximity:, Config:) -- consistent with 30+ existing log lines
- Log these events: HID open failure, calibration read failure, USB disconnect, USB reconnect, pipe creation failure
- Most of these are already logged -- audit and fill gaps rather than rewriting
- Null-check defensive guards at key boundaries (m_pHidDevice before use, pipe handle before operations)
- No SEH (__try/__except) or C++ exceptions -- keep C-style error returns consistent with existing codebase
- Ensure reader thread catches all hid_read/hid_write failures without propagating
- Validate pipe command input (buffer bounds, null termination) before processing

### Claude's Discretion
- Exact placement of null guards (audit codebase to find gaps)
- Whether to add a "degraded mode" flag or just check HID state each RunFrame
- Log message wording and format details

### Deferred Ideas (OUT OF SCOPE)
None
</user_constraints>

<phase_requirements>
## Phase Requirements

| ID | Description | Research Support |
|----|-------------|-----------------|
| RBST-01 | Driver continues functioning (tracking, display, audio unaffected) even if HID device cannot be opened; proximity features silently disabled | Graceful degradation audit findings (Section: Graceful Degradation Gap Analysis) |
| RBST-02 | Driver logs proximity state changes (on-head/off-head transitions) to SteamVR driver log without spamming raw values | Transition logging analysis (Section: Logging Gap Analysis) |
| RBST-03 | Driver logs errors and warnings (HID open failure, calibration read failure, disconnection events) to SteamVR driver log | Error logging audit (Section: Logging Gap Analysis) |
| RBST-04 | Driver does not crash vrserver.exe under any circumstance (HID errors, null pointers, USB disconnect during read) | Crash prevention audit (Section: Crash Prevention Gap Analysis) |
</phase_requirements>

## Standard Stack

### Core
No new libraries needed. This phase uses only what already exists:

| Library | Version | Purpose | Why Standard |
|---------|---------|---------|--------------|
| OpenVR SDK | 2.5.1 | DriverLog via IVRDriverLog | Already integrated |
| HIDAPI | 0.14.0 | HID communication | Already vendored |
| Windows API | Win32 | Named pipes | Already used |
| C++ std | C++17 | atomics, mutex, thread | Already used |

### Alternatives Considered
None -- this is a hardening pass, not a feature addition.

## Architecture Patterns

### Pattern 1: Null Guard Before Member Access
**What:** Check m_pHidDevice (unique_ptr) before dereferencing in RunFrame and HandlePipeCommand.
**When to use:** Every access to m_pHidDevice from DeviceProvider methods.
**Current state:** RunFrame already guards with `if (m_pHidDevice && !m_bManualOverride)`. HandlePipeCommand's status branch checks `if (m_pHidDevice)`. These are correct.

```cpp
// EXISTING pattern (device_provider.cpp:97) -- already correct
if (m_pHidDevice && !m_bManualOverride)
{
    bool detected = m_pHidDevice->GetPersonDetected();
    SetHmdProximity(detected);
}
```

### Pattern 2: State-Change Transition Logging
**What:** Log only when proximity state transitions, not on every poll.
**When to use:** At the point where person_detected changes.
**Current state:** proximity_algorithm.cpp:68-73 ALREADY logs transitions: `"Proximity: person detected"` / `"Proximity: person removed"`. This satisfies RBST-02 in spirit but the CONTEXT.md specifies the format `"Proximity: person_detected changed to %s"`.

```cpp
// EXISTING (proximity_algorithm.cpp:68-73)
if (newDetected != currentDetected)
{
    m_personDetected.store(newDetected, std::memory_order_relaxed);
    DriverLog("Proximity: %s (avg=%u, thresh=%u)\n",
              newDetected ? "person detected" : "person removed",
              averaged, m_trimmedThreshold);
}
```

### Pattern 3: Graceful Degradation via Null Check
**What:** When HID is unavailable, m_pHidDevice is nullptr, and all HID-dependent code paths naturally skip via null check.
**Current state:** Init() sets m_pHidDevice only if hid_init() succeeds. RunFrame guards on m_pHidDevice. This is already the pattern.

### Anti-Patterns to Avoid
- **C++ exceptions in driver code:** SteamVR driver DLLs must not throw exceptions across the DLL boundary. The codebase correctly uses C-style returns.
- **SEH (__try/__except):** Adds complexity, masks bugs. Not appropriate here.
- **Logging in tight loops:** Raw value logging must remain behind m_logVerbose flag. Already correct.

## Don't Hand-Roll

| Problem | Don't Build | Use Instead | Why |
|---------|-------------|-------------|-----|
| Thread-safe state sharing | Custom lock-free structures | std::atomic (already used) | Battle-tested, correct memory ordering |
| Log infrastructure | Custom file logger | DriverLog / IVRDriverLog (already used) | SteamVR aggregates into vrserver.txt |

## Graceful Degradation Gap Analysis

### What already works (RBST-01)
1. `Init()` line 54-58: If `hid_init()` fails, logs warning and continues -- m_pHidDevice stays nullptr. Returns VRInitError_None. **CORRECT.**
2. `RunFrame()` line 97: Guards `m_pHidDevice` before accessing. **CORRECT.**
3. `HandlePipeCommand()` status branch (line 203-242): Checks `m_pHidDevice` and returns limited status when null. **CORRECT.**
4. `Cleanup()` line 80: Guards `m_pHidDevice` before StopReading. **CORRECT.**
5. Pipe is created regardless of HID state (line 68). **CORRECT.**

### Gaps found
1. **Status response when HID unavailable (line 237-242):** Reports `hid=closed` but does NOT report `"HID: not connected"` as the CONTEXT specifies. The status string for no-HID case is minimal: `proximity=%s hid=closed source=%s`. This should be verified as sufficient or augmented.
2. **hid_init() failure path:** When hid_init() fails, hid_exit() is still called in Cleanup(). This is safe per HIDAPI docs (hid_exit can be called regardless), but worth noting.

### Recommendation: Degraded mode flag
Rather than a separate flag, the existing pattern of checking `m_pHidDevice != nullptr` is cleaner and already used consistently. **No new flag needed.** Just verify all access paths are guarded.

## Logging Gap Analysis

### Already logged (RBST-02, RBST-03)

| Event | Location | Log Line | Status |
|-------|----------|----------|--------|
| HID open failure | hid_device.cpp:32 | `"HID: Failed to open device..."` | EXISTS |
| Calibration read failure | hid_device.cpp:223 | `"HID: Warning - failed to read user signature..."` | EXISTS |
| USB disconnect | hid_device.cpp:264 | `"HID: Device disconnected"` | EXISTS |
| USB reconnect | hid_device.cpp:246 | `"HID: Device opened/reconnected"` | EXISTS |
| Pipe creation failure | device_provider.cpp:138 | `"Pipe: Failed to create named pipe..."` | EXISTS |
| hid_init failure | device_provider.cpp:56 | `"HID: hid_init() failed"` | EXISTS |
| Person detected/removed | proximity_algorithm.cpp:71 | `"Proximity: person detected/removed"` | EXISTS |
| Calibration values | hid_device.cpp:206 | `"HID: Calibration: cal=%u..."` | EXISTS |
| Report rate set | hid_device.cpp:139 | `"HID: Report rate set to %ums"` | EXISTS |

### Gaps found

1. **Transition log format mismatch:** CONTEXT specifies `"Proximity: person_detected changed to %s"` but current code logs `"Proximity: person detected"` / `"Proximity: person removed"`. Need to update format to match the locked decision.
2. **SetBoolProperty result logging:** `SetHmdProximity()` (device_provider.cpp:302) logs the property write result but uses a format that includes `"SetBoolProperty"` -- this is fine for debugging but is essentially a state-change log. Consider whether this should also log the transition in a simpler format.
3. **Feature report send failure in ReaderThread context:** `SetReportRate()` logs failures, but if the rate command fails in the ReaderThread, the thread continues silently. This is correct behavior (non-fatal) but should be logged as a warning.
4. **ReadUserSignature block timeout:** Line 189 logs timeout per-block, which is correct.
5. **ReadReport returning 0 (timeout) repeatedly:** No log for repeated read timeouts in ReaderThread. This is intentional (timeout is normal when no data), so no gap.

## Crash Prevention Gap Analysis

### RBST-04: Potential crash vectors audited

| Vector | Location | Current Protection | Gap? |
|--------|----------|-------------------|------|
| m_pHidDevice nullptr deref | RunFrame:97 | `if (m_pHidDevice && ...)` | NO |
| m_pHidDevice nullptr in status | HandlePipeCommand:203 | `if (m_pHidDevice)` | NO |
| m_pHidDevice nullptr in Cleanup | Cleanup:80 | `if (m_pHidDevice)` | NO |
| hid_read returns -1 | ReaderThread:261 | Closes device, sets reconnecting | NO |
| hid_open returns null | ReaderThread:238 | Null check, continues loop | NO |
| Pipe INVALID_HANDLE_VALUE | PollPipe:148 | Early return check | NO |
| Pipe buffer overflow | HandlePipeCommand:180 | cmd comes from 255-byte ReadFile buf, null-terminated at bytesRead | MINOR |
| Pipe cmd not null-terminated | PollPipe:168 | `buf[bytesRead] = '\0'` | NO |
| SendFeatureReport on null device | SendFeatureReport:109 | m_pDevice not checked before hid_send_feature_report | **YES** |
| ReadReport on null device | ReadReport:121 | m_pDevice not checked before hid_read_timeout | **YES** |
| hid_error(nullptr) in Open | Open:32 | HIDAPI docs say hid_error(NULL) returns global error -- safe | NO |
| Integer underflow in hysteresis | proximity_algorithm.cpp:57 | `m_trimmedThreshold - m_hysteresis` unsigned subtraction could underflow if hysteresis > threshold | **YES** |
| Concurrent m_pDevice access | ReaderThread vs Close | StopReading joins thread before Close -- safe sequence | NO |

### Critical gaps found

1. **SendFeatureReport / ReadReport with null m_pDevice:** These are called from ReaderThreadFunc, which checks `m_pDevice` at the top of the loop. However, if a disconnect occurs between the check and these calls, m_pDevice could be null. In practice, this can't happen because only the reader thread modifies m_pDevice. But defensive null checks are cheap insurance.

2. **Unsigned underflow in hysteresis comparison (proximity_algorithm.cpp:57):**
   ```cpp
   if (averaged <= static_cast<uint32_t>(m_trimmedThreshold) - m_hysteresis)
   ```
   If `m_hysteresis > m_trimmedThreshold`, this produces a very large uint32_t, making the condition always false. This is technically correct behavior (person can never be "removed" if threshold is below hysteresis), but it's fragile. A safer pattern:
   ```cpp
   if (m_trimmedThreshold >= m_hysteresis &&
       averaged <= static_cast<uint32_t>(m_trimmedThreshold - m_hysteresis))
   ```

3. **HandlePipeCommand strcmp on potentially unterminated string:** The null termination at PollPipe:168 (`buf[bytesRead] = '\0'`) protects this. However, `HandlePipeCommand` receives `cmd` and `len` but uses `strcmp` (relying on null terminator). If ReadFile somehow returned bytesRead = 255 (full buffer minus 1), `buf[255] = '\0'` is still within bounds (buf is 256 bytes). **Safe but worth documenting.**

4. **WriteFile to pipe after client disconnect:** In HandlePipeCommand, WriteFile (line 263) is called without checking if the pipe is still connected. If the client disconnects between ReadFile and WriteFile, WriteFile will fail silently (returns FALSE). This is not a crash, just a no-op. **No fix needed.**

## Common Pitfalls

### Pitfall 1: Over-logging in RunFrame
**What goes wrong:** RunFrame is called every frame (~11ms at 90Hz). Logging in RunFrame creates massive log files.
**Why it happens:** Developers add debug logging and forget to remove it.
**How to avoid:** All RunFrame logging MUST be behind `m_logVerbose` flag or state-change guards. Already correctly implemented.
**Warning signs:** vrserver.txt growing rapidly during normal operation.

### Pitfall 2: Crash from vrserver.exe address space corruption
**What goes wrong:** An unhandled exception or memory corruption in the driver DLL takes down the entire vrserver.exe process.
**Why it happens:** Driver runs in-process with vrserver.exe.
**How to avoid:** Never throw exceptions across DLL boundary. Validate all external inputs. Use defensive null checks.
**Warning signs:** SteamVR crashes consistently when HID device is connected/disconnected.

### Pitfall 3: Logging after CleanupDriverLog
**What goes wrong:** DriverLog called after s_pLogFile set to nullptr -- log is silently lost (no crash, since DriverLogVarArgs checks s_pLogFile).
**Why it happens:** Cleanup order puts CleanupDriverLog last, but if some destructor logs during teardown.
**How to avoid:** Current Cleanup order is correct: StopReading (joins thread) -> reset (destroys HidDevice) -> hid_exit -> CleanupDriverLog. This ensures no logging after cleanup. **No change needed.**

### Pitfall 4: HIDAPI thread safety
**What goes wrong:** Calling HIDAPI functions on the same device handle from multiple threads.
**Why it happens:** m_pDevice is accessed from reader thread and potentially from main thread.
**How to avoid:** Current design correctly isolates all m_pDevice operations to the reader thread. Main thread only accesses atomics and mutex-protected data. **No change needed.**

## Code Examples

### Transition logging (updated format per CONTEXT.md)
```cpp
// In proximity_algorithm.cpp, replace existing transition log:
if (newDetected != currentDetected)
{
    m_personDetected.store(newDetected, std::memory_order_relaxed);
    DriverLog("Proximity: person_detected changed to %s (avg=%u, thresh=%u)\n",
              newDetected ? "true" : "false",
              averaged, m_trimmedThreshold);
}
```

### Safe unsigned subtraction for hysteresis
```cpp
// In proximity_algorithm.cpp, safe hysteresis check:
if (m_trimmedThreshold >= m_hysteresis &&
    averaged <= static_cast<uint32_t>(m_trimmedThreshold - m_hysteresis))
    newDetected = false;
```

### Null guard pattern for HID helpers (defensive)
```cpp
bool HidDevice::SendFeatureReport(uint8_t cmdCode, const uint8_t* data, size_t dataLen)
{
    if (!m_pDevice)
        return false;
    // ... existing code
}

int HidDevice::ReadReport(uint8_t* buf, size_t bufLen, int timeoutMs)
{
    if (!m_pDevice)
        return -1;
    return hid_read_timeout(m_pDevice, buf, bufLen, timeoutMs);
}
```

## State of the Art

Not applicable -- this phase is a hardening pass on existing C++ code, not adopting new technologies.

## Validation Architecture

### Test Framework
| Property | Value |
|----------|-------|
| Framework | Manual hardware testing (no automated unit tests in project) |
| Config file | none |
| Quick run command | Build + deploy + manual verification via CLI pipe |
| Full suite command | `"C:/Program Files (x86)/Microsoft Visual Studio/2022/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/bin/cmake.exe" --build build --config Release` |

### Phase Requirements -> Test Map
| Req ID | Behavior | Test Type | Automated Command | File Exists? |
|--------|----------|-----------|-------------------|-------------|
| RBST-01 | HID unavailable, driver still runs | manual | Build, start SteamVR without Beyond 2 connected, verify driver loads and pipe responds with status | N/A manual |
| RBST-02 | Proximity state transitions logged | manual | Put headset on/off, check vrserver.txt for "person_detected changed to" messages | N/A manual |
| RBST-03 | Errors/warnings logged | manual | Disconnect USB during operation, check vrserver.txt for disconnect/reconnect messages | N/A manual |
| RBST-04 | No crash under error conditions | manual | Disconnect USB during read, verify vrserver.exe stays running | N/A manual |

### Sampling Rate
- **Per task commit:** Build succeeds (`cmake --build build --config Release`)
- **Per wave merge:** Full build + deploy to SteamVR + manual smoke test
- **Phase gate:** All 4 RBST requirements verified on hardware

### Wave 0 Gaps
None -- this phase modifies existing code (no new test infrastructure needed). Verification is inherently manual (hardware-dependent SteamVR driver).

## Open Questions

1. **Status response wording for no-HID case**
   - What we know: Status currently reports `hid=closed` when m_pHidDevice is null
   - What's unclear: CONTEXT mentions responding with "HID: not connected" -- is this the status response format or just the concept?
   - Recommendation: Update status response to include descriptive text, e.g., `hid=not_connected` when m_pHidDevice is null (vs `hid=closed` which implies device was opened then closed)

2. **Hysteresis underflow safety**
   - What we know: If threshold < hysteresis, unsigned subtraction wraps, making "person removed" condition unreachable
   - What's unclear: Whether this is an intentional design choice or accidental
   - Recommendation: Add explicit guard to prevent unsigned underflow -- safer and self-documenting

## Sources

### Primary (HIGH confidence)
- Direct codebase audit: device_provider.cpp, hid_device.cpp, proximity_algorithm.cpp, driverlog.cpp
- Phase 08 CONTEXT.md: locked decisions and integration points
- Prior phase decisions in STATE.md: established patterns and conventions

### Secondary (MEDIUM confidence)
- HIDAPI 0.14.0 thread safety: hid_error(NULL) returns global error string (verified from HIDAPI source in vendor/)
- SteamVR IVRDriverLog: Writes to vrserver.txt (confirmed Phase 7 decision)

## Metadata

**Confidence breakdown:**
- Standard stack: HIGH - no new dependencies, everything already integrated
- Architecture: HIGH - direct code audit, all patterns visible and understood
- Pitfalls: HIGH - codebase is small (~400 lines of business logic), fully auditable
- Gap analysis: HIGH - complete code reading performed, all files enumerated

**Research date:** 2026-03-22
**Valid until:** 2026-04-22 (stable -- no external dependency changes expected)