Fix MAX17048 HIBRT register address and shifted default-value comments - #1
Fix MAX17048 HIBRT register address and shifted default-value comments#1Bunton33 wants to merge 1 commit into
Conversation
REG_HIBRT was 0x0B; the datasheet register map puts HIBRT at 0x0A (0x0B is HIBRT's ActThr low byte). The default-value comments were also shifted one row: HIBRT's POR default is 0x8030 (not 0x971C, which is CONFIG's default), and CONFIG's is 0x971C (not 0x00FF, which is VALRT's). Dormant today because nothing writes HIBRT yet, but the datasheet requires all accesses to be 16-bit word operations, so a future hibernate-tuning write through 0x0B would span HIBRT's ActThr byte and CONFIG's RCOMP byte, silently corrupting the SOC-model compensation on every unit. Added a note about the word-access requirement above the register table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThe MAX17048 register definitions now use the documented 16-bit address for ChangesMAX17048 register definition update
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/max17048/max17048.cpp (1)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
uint8_tfor MAX17048 register addresses.
MAX17048uses word-sized values, but I2C register addresses fit in one byte. Make theREG_*constantsuint8_t, matching the 16-bit value reads/writes and register values that needuint16_t.Proposed fix
-constexpr uint16_t REG_VCELL = 0x02; +constexpr uint8_t REG_VCELL = 0x02; ... -constexpr uint16_t REG_CONFIG = 0x0C; +constexpr uint8_t REG_CONFIG = 0x0C;🤖 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 `@components/max17048/max17048.cpp` around lines 16 - 21, Change the REG_VCELL, REG_SOC, REG_MODE, REG_VERSION, REG_HIBRT, and REG_CONFIG constants to use uint8_t while keeping the register values and existing 16-bit read/write data types unchanged.
🤖 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.
Nitpick comments:
In `@components/max17048/max17048.cpp`:
- Around line 16-21: Change the REG_VCELL, REG_SOC, REG_MODE, REG_VERSION,
REG_HIBRT, and REG_CONFIG constants to use uint8_t while keeping the register
values and existing 16-bit read/write data types unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c911819-750b-4c88-90ba-ec5e1da17ba5
📒 Files selected for processing (1)
components/max17048/max17048.cpp
Summary
REG_HIBRTwas0x0B; the MAX17048 datasheet register map puts HIBRT at0x0A(0x0Bis HIBRT's ActThr low byte).0x8030(not0x971C, which belongs to CONFIG), and CONFIG's is0x971C(not0x00FF, which belongs to VALRT).Why it matters
Dormant today because nothing writes HIBRT yet — but the word-access requirement means a future hibernate-tuning write through
0x0Bwould span HIBRT's ActThr byte and CONFIG's RCOMP byte, silently corrupting the SOC-model compensation on every unit. Hibernate/ActThr tuning is the most likely future edit to this component (it controls charge-detection latency), so the misaligned constant is a trap worth removing now.No functional change to current behavior: the constant is unused at runtime today, and the read paths (VCELL/SOC/CRATE) are untouched.
🤖 Generated with Claude Code
Summary by CodeRabbit