Skip to content

Fix link failure on non-ADC targets from unconditional checkBatteryVoltageState() call - #11865

Open
sensei-hacker wants to merge 2 commits into
iNavFlight:maintenance-10.xfrom
sensei-hacker:fix-battery-voltage-state-non-adc
Open

Fix link failure on non-ADC targets from unconditional checkBatteryVoltageState() call#11865
sensei-hacker wants to merge 2 commits into
iNavFlight:maintenance-10.xfrom
sensei-hacker:fix-battery-voltage-state-non-adc

Conversation

@sensei-hacker

Copy link
Copy Markdown
Member

Summary

checkBatteryVoltageState() is defined only inside #ifdef USE_ADC in src/main/sensors/battery.c, but two call sites invoked it unconditionally:

  • src/main/io/osd.cosdDisplayBatteryVoltage()
  • src/main/fc/multifunction.c — the low-battery-voltage warning check

Any target that doesn't define USE_ADC fails to link with undefined reference to checkBatteryVoltageState. This affects COLIBRI and QUANTON, which have no analog battery-voltage sensing hardware at all. Bisected the regression to commit f3de644 ("osd batt cleanup", 2023-02-07), which extracted the voltage-state check out of the always-defined getBatteryState() path into the new ADC-only function without updating these two unconditional callers. It went unnoticed because both affected targets are SKIP_RELEASES and aren't in the normal release build sweep.

Changes

  • Both call sites now use getBatteryState() instead, which is defined unconditionally and returns the same cached state. multifunction.c already uses getBatteryState() two lines later for the equivalent capacity-based warning check, so this also makes the two checks consistent with each other.

Testing

  • COLIBRI and QUANTON: previously failed to link, now build and link cleanly.
  • MATEKF405, OMNIBUSF4, KAKUTEF7 (ADC-equipped): confirmed no regression, build cleanly with no new warnings.

…ltageState() call

checkBatteryVoltageState() is only defined inside #ifdef USE_ADC in
battery.c, but osdDisplayBatteryVoltage() and the multifunction low-battery
warning check both called it unconditionally. Any target without USE_ADC
(e.g. COLIBRI/QUANTON, which have no analog battery-voltage sensing at all)
fails to link with an undefined reference.

Both call sites now use getBatteryState(), which is defined unconditionally
and returns the same cached state - multifunction.c already uses it two
lines later for the equivalent capacity-based warning check. This restores
the behavior both call sites had before the voltage-state check was
extracted into checkBatteryVoltageState() (commit f3de644, 2023-02-07),
which is when this bug was introduced.
@qodo-code-review

Copy link
Copy Markdown
Contributor

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix non-ADC link failure in battery warning call sites

🐞 Bug fix 🕐 Less than 10 minutes

Grey Divider

AI Description

• Replaces ADC-only voltage checks with the always-available cached battery-state accessor.
• Restores linking for COLIBRI, QUANTON, and other targets without ADC support.
• Aligns OSD voltage and multifunction warnings with the shared battery state.
Diagram

graph TD
  A["ADC targets"] --> B["Battery subsystem"] --> C["State accessor"] --> D["OSD voltage"]
  F["Non-ADC targets"] --> B
  C --> E["Warning messages"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Conditional ADC-specific calls
  • ➕ Preserves a distinct voltage-only state check on ADC-equipped targets.
  • ➕ Limits the behavioral change strictly to non-ADC builds.
  • ➖ Duplicates conditional-compilation logic at every consumer.
  • ➖ Creates target-dependent warning behavior and risks future unguarded call sites.
2. Provide a universal voltage-state wrapper
  • ➕ Retains one consumer-facing API across all targets.
  • ➕ Centralizes the non-ADC fallback inside the battery subsystem.
  • ➖ Adds an API whose voltage semantics are unclear without voltage-sensing hardware.
  • ➖ Requires broader battery subsystem changes for a two-call-site linkage defect.

Recommendation: The PR's shared getBatteryState() approach is the simplest and most maintainable option when these displays should reflect overall cached battery health. It avoids spreading USE_ADC guards into consumers and restores non-ADC linkage; reviewers should only verify that capacity-threshold configurations intentionally use overall rather than voltage-only state.

Files changed (2) +2 / -2

Bug fix (2) +2 / -2
multifunction.cUse shared battery state for voltage warnings +1/-1

Use shared battery state for voltage warnings

• Replaces the ADC-only voltage-state function with getBatteryState() when selecting low-battery multifunction warnings. This removes the unresolved symbol on non-ADC targets and matches the adjacent capacity warning path.

src/main/fc/multifunction.c

osd.cUse shared battery state for voltage blinking +1/-1

Use shared battery state for voltage blinking

• Reads the cached battery state through the unconditional accessor when deciding whether the displayed voltage should blink. Non-ADC targets can now link the OSD code without checkBatteryVoltageState().

src/main/io/osd.c

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 5, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Capacity triggers voltage warning ✓ Resolved 🐞 Bug ≡ Correctness
Description
When capacity thresholds are enabled, getBatteryState() returns capacity-derived state, so the
multifunction voltage warning duplicates the capacity warning and can report VBATT LOW/LAND
despite healthy voltage. It can also omit a genuine low-voltage warning while remaining capacity is
healthy.
Code

src/main/fc/multifunction.c[269]

+    const batteryState_e batteryVoltageState = getBatteryState();
Evidence
checkBatteryVoltageState() independently evaluates voltage thresholds, whereas
checkBatteryCapacityState() writes the shared batteryState from remaining capacity.
batteryUpdate() chooses the capacity path at lines 506-510, and the separate capacity warning at
lines 275-280 already consumes that shared state.

src/main/sensors/battery.c[397-435]
src/main/sensors/battery.c[481-483]
src/main/sensors/battery.c[498-510]
src/main/fc/multifunction.c[268-280]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Preserve voltage-specific warning semantics while avoiding references to `checkBatteryVoltageState()` on builds without `USE_ADC`. The aggregate `getBatteryState()` must not drive the explicitly voltage-based warning because it becomes capacity-derived when capacity thresholds are active.
## Issue Context
`batteryUpdate()` selects `checkBatteryCapacityState()` when `batteryUseCapacityThresholds` is true. The following capacity-warning block already reports that aggregate state separately, so using it for the voltage block produces incorrect and duplicate warnings.
## Fix Focus Areas
- src/main/fc/multifunction.c[268-280]
- src/main/sensors/battery.c[397-435]
- src/main/sensors/battery.c[498-510]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Capacity controls voltage blinking ✓ Resolved 🐞 Bug ≡ Correctness
Description
When capacity thresholds are enabled, getBatteryState() makes the displayed voltage blink
according to remaining capacity rather than the voltage thresholds. Consequently, healthy voltage
can blink on low capacity while genuinely low voltage may not blink when capacity remains healthy.
Code

src/main/io/osd.c[1603]

+    const batteryState_e batteryVoltageState = getBatteryState();
Evidence
The modified function displays a voltage value and uses the returned state solely to set its blink
attribute. The battery implementation proves that the old function evaluates voltage independently,
while the new getter can expose capacity-derived batteryState.

src/main/io/osd.c[1588-1607]
src/main/sensors/battery.c[397-435]
src/main/sensors/battery.c[498-510]
src/main/sensors/battery.c[528-530]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Keep voltage-display blinking based on voltage thresholds while preventing non-ADC builds from referencing an unavailable symbol. Do not substitute the aggregate battery state because it can represent remaining capacity.
## Issue Context
`checkBatteryVoltageState()` evaluates `batteryWarningVoltage` and `batteryCriticalVoltage`, but `getBatteryState()` returns the shared state that `batteryUpdate()` derives from capacity whenever capacity thresholds are enabled. Use compile-time guarding or an always-defined voltage-state API with an appropriate non-ADC fallback.
## Fix Focus Areas
- src/main/io/osd.c[1588-1607]
- src/main/sensors/battery.c[397-435]
- src/main/sensors/battery.c[498-510]
- src/main/sensors/battery.h[76-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/main/fc/multifunction.c Outdated
Comment thread src/main/io/osd.c Outdated
@breadoven

Copy link
Copy Markdown
Collaborator

This isn't the right way to fix this. checkBatteryVoltageState specifically handles voltage whereas getBatteryState doesn't, it could just be capacity based if capacity is used.

Probably best to just make multifunction and OSD voltage conditional on USE_ADC surely ? If there's no voltage available there's nothing to display.

@sensei-hacker

Copy link
Copy Markdown
Member Author

This isn't the right way to fix this. checkBatteryVoltageState specifically handles voltage whereas getBatteryState doesn't, it could just be capacity based if capacity is used.

Probably best to just make multifunction and OSD voltage conditional on USE_ADC surely ? If there's no voltage available there's nothing to display.

Thank you!

The previous commit fixed the non-ADC link failure by switching both
call sites to getBatteryState(), but that broke voltage-specific
semantics: when capacity thresholds are enabled, getBatteryState()
returns capacity-derived state, so the voltage warning/blink would
duplicate the capacity warning (e.g. reporting VBATT LOW on healthy
voltage) or miss a genuine low-voltage condition while capacity is
fine. Caught by review (qodo bot + breadoven) on PR iNavFlight#11865.

Both call sites now use checkBatteryVoltageState() again, guarded by
#ifdef USE_ADC (the only compilation unit it's defined in). On
non-ADC targets there's no voltage to display or warn about, so the
voltage warning/blink is simply omitted instead of substituting
unrelated state - restoring correct behavior on ADC targets while
still fixing the link error on non-ADC ones.
@sensei-hacker

sensei-hacker commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Good catch, thanks! Pushed a fix. Both call sites now use checkBatteryVoltageState() again, guarded by #ifdef USE_ADC

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Test firmware build ready — commit 502def4

Download firmware for PR #11865

249 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

@iNavFlight iNavFlight deleted a comment from github-actions Bot Sep 6, 2026
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.

2 participants