Fix QMC5883P detection

[haslinghuis]

Finally could test 5883P with a real device and needed to make a small adjustment in the code.
Tested with GEPRC M1025Q.

Summary by CodeRabbit

• Bug Fixes
  • Improved magnetometer initialization to perform explicit configuration writes with acknowledgment checks and to fail cleanly on write errors.
  • Simplified device detection to only verify chip identity and attach handlers, removing prior configuration writes from the detection path.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[github-actions]

Do you want to test this code? You can flash it directly from the Betaflight App:

• Simply put #14782 (this pull request number) in the Select commit field in the Firmware Flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

[coderabbitai]

Walkthrough

Move P-variant QMC5883 special configuration writes out of qmc5883pDetect into qmc5883Init. qmc5883pDetect now only verifies chip ID and attaches handlers; qmc5883Init performs three sequential writes (XYZ unlock, CONF1, CONF2) and returns false on any write failure.

Changes

Cohort / File(s)	Summary
QMC5883 P Variant Init & Detect src/main/drivers/compass/compass_qmc5883.c	Removed P-variant configuration writes from qmc5883pDetect (now only checks chip ID and attaches descriptor/handlers). Added explicit sequential writes in qmc5883Init for P variant: QMC5883P_REG_XYZ_UNLOCK (with QMC5883P_XYZ_SIGN_CONFIG), then QMC5883P_REG_CONF1 (QMC5883P_DEFAULT_CONF1), then QMC5883P_REG_CONF2 (QMC5883P_DEFAULT_CONF2), each gated on ack and returning false on failure.

Sequence Diagram(s)

sequenceDiagram
    participant Probe as Detection Flow
    participant Init as Initialization Flow
    participant Chip as QMC5883P

    Note over Probe,Chip `#e6f7ff`: Detect only verifies ID
    Probe->>Chip: Read CHIP_ID
    alt CHIP_ID matches
        Probe-->>Probe: attach descriptor & handlers
        Probe->>Init: invoke qmc5883Init
    else mismatch
        Probe-->>Probe: detection failed
    end

    Note over Init,Chip `#fff7e6`: Init performs gated sequential writes
    Init->>Chip: Write XYZ_UNLOCK (expect ACK)
    alt ACK OK
        Init->>Chip: Write CONF1 (expect ACK)
        alt ACK OK
            Init->>Chip: Write CONF2 (expect ACK)
            alt ACK OK
                Init-->>Init: init success
            else ACK FAIL
                Init-->>Init: init fail (return false)
            end
        else ACK FAIL
            Init-->>Init: init fail (return false)
        end
    else ACK FAIL
        Init-->>Init: init fail (return false)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Single file but behavioral change across detection vs init flows.
• Verify correctness of write sequencing, ack gating, and failure paths.
• Areas to focus:
  • Correctness of chip ID gating and absence of side effects from removing early writes.
  • Proper error handling and return values in qmc5883Init.
  • Any callers/assumptions that expected device configured during detect.

Possibly related PRs

• QMC5883 unified driver for L and P variant #14713: Adjusts the unified QMC5883 driver and specifically moves/removes P-variant config writes between detect and init—directly related.

Suggested labels

Tested

Suggested reviewers

• KarateBrot
• nerdCopter
• blckmn

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix QMC5883P detection' directly relates to the main change: moving initialization writes from qmc5883pDetect to qmc5883Init to fix detection issues.
Description check	✅ Passed	The description briefly explains the fix and mentions testing with a real device (GEPRC M1025Q), but lacks detail on what the issue was and technical specifics of the solution.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ae5c5cd and 88807b3.

📒 Files selected for processing (1)

• src/main/drivers/compass/compass_qmc5883.c (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: ledvinap
Repo: betaflight/betaflight PR: 14578
File: src/main/telemetry/jetiexbus.c:547-556
Timestamp: 2025-08-12T18:26:17.016Z
Learning: For Betaflight Jeti ExBus backport PR #14578: ledvinap prefers to keep the PR focused on backporting fixes from iNavFlight rather than expanding into broader architectural improvements. Additional improvements to TX/RX state machine should be handled in separate PRs.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: CI / Build (SPRACINGH7RF)
• GitHub Check: CI / Build (STM32H723)
• GitHub Check: CI / Build (SPRACINGH7EXTREME)
• GitHub Check: CI / Build (STM32H743)
• GitHub Check: CI / Build (STM32H725)
• GitHub Check: CI / Build (RP2350B)
• GitHub Check: CI / Test

🔇 Additional comments (2)

src/main/drivers/compass/compass_qmc5883.c (2)

197-202: LGTM! Initialization sequence correctly implemented.

The P variant initialization properly implements the three-step configuration sequence with cumulative error checking. The pattern ack = ack && busWriteRegister(...) ensures that if any write fails, subsequent writes are skipped and the function returns false (lines 205-207). This addresses the past review comment and matches the error-handling pattern used for the L variant.

---

324-352: LGTM! Detection logic correctly separated from initialization.

The function now properly focuses solely on detection by reading and verifying the chip ID, then attaching the appropriate handlers. The removal of initialization writes from this function (now in qmc5883Init) correctly separates detection from initialization, which aligns with the pattern used in qmc5883lDetect and likely resolves the detection issue described in the PR.

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/main/drivers/compass/compass_qmc5883.c (1)

193-207: P-variant init writes look correct; consider minor consistency/clarity tweaks

Functionally this looks good: moving the P-variant unlock/CONF1/CONF2 writes into qmc5883Init keeps detection read-only while preserving the actual register configuration sequence, and the early returns on write failure are straightforward.

Two minor, optional nits:

• ack is now only meaningful for the L variant; for readability you could either:
  • Limit ack’s scope to the L branch, or
  • Use the same ack = ack && busWriteRegister(...) pattern for the P branch as well so both variants use a consistent style.
• The comment “(retaining original behavior)” might be a bit confusing now that the writes have moved from detect to init; something like “using the same register values as previously configured during detect” would be more precise.

Both are non-blocking; the current change is fine to ship as-is.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf7efdd and ae5c5cd.

📒 Files selected for processing (1)

• src/main/drivers/compass/compass_qmc5883.c (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)

• GitHub Check: CI / Build (SPRACINGH7EXTREME)
• GitHub Check: CI / Build (SPRACINGH7RF)
• GitHub Check: CI / Build (IFLIGHT_BLITZ_F722)
• GitHub Check: CI / Build (STM32H750)
• GitHub Check: CI / Build (STM32H735)
• GitHub Check: CI / Build (STM32H743)
• GitHub Check: CI / Build (AIRBOTG4AIO)
• GitHub Check: CI / Build (STM32H730)
• GitHub Check: CI / Build (STM32H725)
• GitHub Check: CI / Build (STM32H723)
• GitHub Check: CI / Build (RP2350B)
• GitHub Check: CI / Build (RP2350A)
• GitHub Check: CI / Build (APM32F405)
• GitHub Check: CI / Test

🔇 Additional comments (1)

src/main/drivers/compass/compass_qmc5883.c (1)

328-355: QMC5883P detect path is now cleanly ID-based

The detect routine’s flow (set default I2C address if unset → read ID → bail out on NACK or wrong ID → attach descriptor and init/read callbacks) is straightforward and keeps detection side-effect free, which pairs well with moving all configuration into qmc5883Init.

No issues from this code path from a driver/detection standpoint.