Skip to content

fix(m5stack-tab5): only enable charging when a battery is fitted (periodic speaker pop) - #876

Merged
finger563 merged 3 commits into
mainfrom
fix/tab5-charger-no-battery
Nov 2, 2026
Merged

finger563 merged 3 commits into
mainfrom
fix/tab5-charger-no-battery

Conversation

@finger563

@finger563 finger563 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes the periodic pop on the Tab5 speaker (one every few seconds to a couple of minutes, idle or not) that remained after #868 / #875.

It is not the audio path. The example enables battery charging unconditionally at boot, and on a Tab5 with no NP-F550 in the bay (the battery is kit-dependent) the IP2326 keeps retrying to charge nothing; every retry is a step on the supply that the amplifier reproduces as a pop. Ruled out on hardware first: the I2S feed (the codec receives continuous digital zeros, no underruns), and the esp_video ISP pipeline controller (its sensor exposure/gain writes were logged with timestamps and never coincided with a pop: auto-exposure converged 33 s after boot, pops followed at 78 s, 192 s and 253 s). The same camera-on build with charging left disabled ran five minutes of camera, rotation and touch without a pop.

  • BatteryStatus::is_present now means a battery is in the bay: the INA226 pack-line voltage is at least 4 V. An empty bay reads the charger's open-circuit output (about 2 V, measured); a 2S pack never reads below its protection cut-off (about 5 V). It used to be true whenever the INA226 read succeeded. With no battery, charge_percent is 0 and is_charging is false.
  • The example reads the battery status once after the monitor comes up and enables charging only when a battery is present, logging which way it went.
  • espp::Ina226::conversion_ready() (new, reads CVRF in the Mask/Enable register) and initialize_battery_monitoring() waits for the first conversion (up to 200 ms; 35 ms expected with 16x averaging) before returning. Without it the first read after init returns zeros, which a presence check would take for an empty bay even with a battery fitted (seen on hardware: 0.00 V, then 1.83 V with the wait).
  • README notes on both the component and the example say why.

With a battery fitted: the charger pops on every speaker transient

Fitting a battery removed the idle pops and brought a second kind back: pops during clicks and playback. Three builds on hardware, all with the pack fitted:

charging (CHG_EN) quick charge (nCHG_QC_EN) charge current playback
off - none clean
on enabled (M5Stack's boot default) charging pops
on disabled pack full, charger idling pops

So it is the charger being enabled, not the current it draws, and the fix is to hold CHG_EN low while the speaker is active. The BSP now does this itself (set_charging_pause_on_playback_enabled(), default on): the playback task reports per block whether it carried audio, the charger is switched off on the first audio block and back on 3 s after the last one, so consecutive clicks do not toggle it. set_charging_enabled() keeps the requested state and a request to enable made during playback is applied when playback ends. Verified by ear with the same logic in the example first: pops gone, charging visibly resuming on the example's battery line.

One more data point: with a battery fitted and charging enabled, the pops come and go with the charger's own state. On boots where the example's status line reads charging (status pin asserted, current flowing into the pack) playback is clean; on boots where it reads not charging, playback pops, and the empty-bay case is the charger in that same not-charging, retrying state all the time. What puts the IP2326 into that state on a given boot is not established (it has no digital interface; the pack was near full during these tests). The pause during playback holds either way, since it switches the charger off exactly when a retry would be audible.

Test plan

  • Tab5 (ST71xx, ESP-IDF v6.1, no battery fitted): before, pops every 1-2 min while idle; with charging disabled, 5 min of camera / rotation / touch with no pop
  • components/m5stack-tab5/example builds for esp32p4 (component manager off)
  • Tab5 with this build: boot log says "No battery detected (1.83 V on the pack line)", no pops (user, by ear)
  • Tab5 with an NP-F550 fitted, charging forced off: clean playback; charging on with quick charge off: pops (user, by ear)
  • Tab5 with an NP-F550 fitted, charging paused during playback (example-level prototype): clicks, drag-clicks and recorded playback clean, charging resumes on the battery line (user, by ear)
  • Same with the pause in the BSP (76bcd03 on the board): clicks, drag-clicks and recorded playback clean, charging resumes (user, by ear)
  • CI

🤖 Generated with Claude Code

https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI balanced review requested due to automatic review settings November 2, 2026 19:29
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open issues
What changed in this PR

Updates Tab5 battery/charging logic so charging is only enabled when a battery is actually present, preventing periodic speaker pops caused by the charger retrying against an empty bay.

Changes:

  • Add INA226 “conversion ready” polling and wait for first conversion during battery-monitor init to avoid initial zero readings.
  • Redefine BatteryStatus::is_present to reflect real battery presence via pack-voltage threshold, and gate SoC/charging fields accordingly.
  • Update the example and READMEs to only enable charging when a battery is detected, documenting the audible-pop rationale.
File Description
components/​m5stack-tab5/​src/​power.cpp Waits for INA226 first conversion; updates battery presence semantics and charge fields when no pack is present.
components/​m5stack-tab5/​include/​m5stack-tab5.hpp Documents new meaning/behavior of BatteryStatus::is_present and why charging should stay off when absent.
components/​m5stack-tab5/​example/​main/​m5stack_tab5_example.cpp Enables charging only if read_battery_status().is_present is true; logs both outcomes.
components/​m5stack-tab5/​example/​README.md Documents conditional charging due to speaker pop when bay is empty.
components/​m5stack-tab5/​README.md Documents that charging is off by default and should be enabled only when a battery is present.
components/​ina226/​include/​ina226.hpp Adds Ina226::conversion_ready() by reading CVRF from Mask/Enable.

Review effort: Lite

Comment thread components/m5stack-tab5/src/power.cpp Resolved
Comment thread components/m5stack-tab5/src/power.cpp Resolved
Comment thread components/m5stack-tab5/src/power.cpp Resolved
Comment thread components/m5stack-tab5/src/power.cpp Resolved
finger563 added a commit that referenced this pull request Nov 2, 2026
…om its settings

Review follow-up on #876: Ina226::conversion_time_us() (and the
averaging_count / per-setting helpers) state the data-register update
time for a configuration; the BSP's wait for the first conversion uses a
named poll interval and a timeout that is a multiple of that expected
time, and the warning reports both. Also a double space in a comment.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
finger563 and others added 3 commits November 2, 2026 15:42
…iodic speaker pop)

The example enabled charging unconditionally at boot. With no NP-F550 in
the bay (kit-dependent) the IP2326 keeps retrying to charge nothing and
every retry is a step on the supply that the amplifier reproduces as a pop,
every few seconds to a couple of minutes, idle or not. Verified on
hardware: the same camera-on build with charging left disabled ran five
minutes of camera / rotation / touch without a pop; the audio feed and the
esp_video ISP controller's sensor writes were ruled out first.

- BatteryStatus::is_present now means a battery is in the bay: the INA226
  pack-line voltage is at least 4 V (an empty bay reads the charger's ~2 V
  open-circuit output, a 2S pack never reads below ~5 V). It used to be true
  whenever the read succeeded. With no battery, charge_percent is 0 and
  is_charging is false.
- The example enables charging only when a battery is present.
- Ina226::conversion_ready() (CVRF) and initialize_battery_monitoring()
  waits for the first averaged conversion (35 ms expected, 200 ms cap), so
  the first read after init is real instead of zeros.
- README notes on the component and the example.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
With a battery fitted the enabled IP2326 pops the speaker on every audio
transient, whatever the charge current (hardware A/B: clean with CHG_EN
low; popping with it high both while charging and with the pack full and
the charger idling with quick charge off). The playback task now reports
per block whether it carried audio; the charger is switched off on the
first audio block and back on CHARGING_RESUME_DELAY_MS (3 s) after the
last, so consecutive clicks do not toggle it. set_charging_enabled()
keeps the requested state and applies an enable made during playback
when playback ends; set_charging_pause_on_playback_enabled() (default
on) opts out. Verified by ear with the same logic in the example first.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…om its settings

Review follow-up on #876: Ina226::conversion_time_us() (and the
averaging_count / per-setting helpers) state the data-register update
time for a configuration; the BSP's wait for the first conversion uses a
named poll interval and a timeout that is a multiple of that expected
time, and the warning reports both. Also a double space in a comment.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
@finger563

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #875 (shared I2S feeder) is merged. One conflict in audio.cpp: the playback callback is now the feeder version, so the charger hook takes fill_block()'s returned audio byte count (update_charging_for_audio(audio_bytes > 0)); the teardown resume_charging_after_audio() sits next to detach_task(). Example rebuilt clean on 853ba44.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

@finger563
finger563 force-pushed the fix/tab5-charger-no-battery branch from 26d43e1 to 853ba44 Compare November 2, 2026 21:44
@finger563
finger563 merged commit bc20941 into main Nov 2, 2026
178 checks passed
@finger563
finger563 deleted the fix/tab5-charger-no-battery branch November 2, 2026 21:55
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