Add charge start vibration capability#4
Conversation
📝 WalkthroughWalkthroughThis pull request updates FlexPuff capability gating to require an explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds an explicit ChargeStartVibration device capability and centralizes charge-start vibration support behind the capability matrix, while tightening documentation and tests around the per-model feature matrix.
Changes:
- Add
DeviceCapability::ChargeStartVibrationand routesupports_charge_start_vibration()throughDeviceModel::supports(...). - Replace ad-hoc capability tests with a single explicit model-by-capability matrix test.
- Update FlexPuff documentation (and README) to reflect ILUMA i-family support; add project rules doc with the capability matrix.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/protocol/types.rs |
Introduces the new capability, updates the support matrix routing, and refactors tests into an explicit matrix. |
src/protocol/flexpuff.rs |
Updates rustdoc to reflect correct FlexPuff model support. |
src/lib.rs |
Aligns public API rustdoc for read_flexpuff() with the updated capability matrix. |
docs/rules/project.md |
Adds AI-agent project rules and documents the model/capability matrix. |
README.md |
Updates the “Programmatic device control” bullet to annotate FlexPuff model support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### Programmatic device control | ||
|
|
||
| - Brightness, vibration, FlexPuff | ||
| - Brightness, vibration, FlexPuff _(ILUMA i)_ |
There was a problem hiding this comment.
The README now says FlexPuff is available on “ILUMA i”, but elsewhere (and in the capability matrix) it’s specifically IlumaI and IlumaIPrime. Consider updating this bullet to mention ILUMA i and ILUMA i PRIME (or “ILUMA i family”) to avoid underspecifying support.
| - Brightness, vibration, FlexPuff _(ILUMA i)_ | |
| - Brightness, vibration, FlexPuff _(ILUMA i and ILUMA i PRIME)_ |
There was a problem hiding this comment.
Fixed in 3faa3f7: the README now separates Brightness/Vibration from ILUMA i-only controls and lists FlexPuff/FlexBattery/Pause Mode as ILUMA i / ILUMA i PRIME.
| /// Lock/unlock support. | ||
| DeviceLock, | ||
| /// Holder vibration feedback when the stick begins charging. | ||
| ChargeStartVibration, | ||
| } |
There was a problem hiding this comment.
DeviceCapability is a public enum re-exported from the crate; adding a new variant is a SemVer-breaking change for downstream users that exhaustively match on it. If this crate intends to maintain 1.x SemVer stability, consider marking DeviceCapability as #[non_exhaustive] (and updating docs/changelog accordingly) or otherwise treating this as a major-version bump change.
There was a problem hiding this comment.
Fixed in 3faa3f7: DeviceCapability is now #[non_exhaustive], so downstream exhaustive matches must include a wildcard arm while this crate can continue matching internally.
| Authoritative source of project-local constraints, gotchas, and best practices. | ||
| Read this before touching any code. Cross-reference with `AGENTS.md` for | ||
| architecture and git hygiene rules. |
There was a problem hiding this comment.
This doc references AGENTS.md as an architecture/git hygiene source, but that file does not exist in the repository. Either add the referenced file or update the text to point at the correct existing document so readers/agents aren’t sent to a dead link.
| Authoritative source of project-local constraints, gotchas, and best practices. | |
| Read this before touching any code. Cross-reference with `AGENTS.md` for | |
| architecture and git hygiene rules. | |
| Authoritative source of project-local constraints, gotchas, best practices, | |
| architecture, and git hygiene rules. | |
| Read this before touching any code. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/rules/project.md`:
- Line 24: Three fenced code blocks use plain triple backticks (```) with no
language, triggering markdownlint MD040; update each plain fence to specify a
language (e.g., change ``` to ```text or another appropriate language) for the
three occurrences of the triple-backtick fenced code blocks so markdownlint-cli2
stops flagging MD040 and the blocks render consistently.
- Around line 62-82: The command-prefix table is inverted for FlexPuff: change
the table so 0xD2 is listed as the FlexPuff request prefix
(LOAD_FLEXPUFF_COMMAND request byte 1 = 0xD2) and 0x90 is listed as the FlexPuff
response marker (parser checks bytes[1] == 0x90); also add a short example in
the "Critical gotcha" section mirroring the Holder example (e.g., request [0x00,
0xD2, ...] → response [0x00, 0x90, ...]) so readers see the request-vs-response
discriminant for LOAD_FLEXPUFF_COMMAND and FirmwareKind/response matching.
In `@README.md`:
- Line 26: The README's feature bullet wrongly tags "Brightness, vibration,
FlexPuff _(ILUMA i)_" as ILUMA i-only; update the README so that Brightness and
Vibration are shown as supported on all non-Unknown models (remove the trailing
"_(ILUMA i)_" qualifier from those capabilities) and ensure only FlexPuff is
gated to ILUMA i / ILUMA i PRIME (use the same phrasing as src/lib.rs and
src/protocol/flexpuff.rs, e.g., "_(ILUMA i / ILUMA i PRIME)_"); also adjust the
FlexBattery line similarly if present to "_(ILUMA i / ILUMA i PRIME)_". Use
references to the capability matrix in src/protocol/types.rs when editing to
match the canonical gating.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5876b8d-6413-475a-b2c2-1858571c16be
📒 Files selected for processing (5)
README.mddocs/rules/project.mdsrc/lib.rssrc/protocol/flexpuff.rssrc/protocol/types.rs
|
|
||
| ## Device Model Reference | ||
|
|
||
| ``` |
There was a problem hiding this comment.
markdownlint MD040: specify a language for fenced code blocks.
Static analysis flags the three plain fences at lines 24, 55, and 88. Usetext (or an appropriate language) to keep markdownlint-cli2 clean.
Also applies to: 55-55, 88-88
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/rules/project.md` at line 24, Three fenced code blocks use plain triple
backticks (```) with no language, triggering markdownlint MD040; update each
plain fence to specify a language (e.g., change ``` to ```text or another
appropriate language) for the three occurrences of the triple-backtick fenced
code blocks so markdownlint-cli2 stops flagging MD040 and the blocks render
consistently.
| **Known command prefix families:** | ||
|
|
||
| | Byte 1 | Component | Example | | ||
| |--------|-----------|---------| | ||
| | `0xC0` | Stick / device-wide | `LOAD_STICK_FIRMWARE_VERSION_COMMAND` | | ||
| | `0xC9` | Holder | `LOAD_HOLDER_FIRMWARE_VERSION_COMMAND` | | ||
| | `0x08` | Response marker (holder) | appears in holder response frames | | ||
| | `0x90` | FlexPuff | `LOAD_FLEXPUFF_COMMAND` | | ||
| | `0xD2` | (mapped in captures, TBD) | reserved — do not guess | | ||
|
|
||
| ### Critical gotcha: FirmwareKind discriminant vs response byte | ||
|
|
||
| `FirmwareKind::Stick = 0xC0` — the **request** uses `0xC0` at byte 1. | ||
| `FirmwareKind::Holder = 0x08` — the **response** uses `0x08` at byte 1, even though | ||
| the **request** used `0xC9`. Do not confuse request prefix with response discriminant. | ||
|
|
||
| ```rust | ||
| // FirmwareVersion::from_response checks bytes[1] == kind as u8 | ||
| // Holder kind = 0x08 → expects 0x08 in response bytes[1] | ||
| // Holder request = [0x00, 0xC9, ...] → response arrives as [0x00, 0x08, ...] | ||
| ``` |
There was a problem hiding this comment.
Command-prefix table contradicts LOAD_FLEXPUFF_COMMAND and undermines the gotcha section.
src/protocol/flexpuff.rs defines LOAD_FLEXPUFF_COMMAND = [0x00, 0xD2, 0x05, ...] (request byte 1 = 0xD2), and the parser matches bytes[1] == 0x90 on the response. The table here has this inverted:
- Line 69 maps
0x90→LOAD_FLEXPUFF_COMMAND, but0x90is the FlexPuff response marker, not the request prefix. - Line 70 lists
0xD2as "reserved — do not guess", even though0xD2is actually the FlexPuff request prefix in use.
This is exactly the request-vs-response discriminant gotcha the following section warns about, so leaving it wrong in the authoritative rules doc is especially misleading.
📝 Proposed fix
-| `0x90` | FlexPuff | `LOAD_FLEXPUFF_COMMAND` |
-| `0xD2` | (mapped in captures, TBD) | reserved — do not guess |
+| `0xD2` | FlexPuff (request) | `LOAD_FLEXPUFF_COMMAND` |
+| `0x90` | Response marker (FlexPuff) | appears in FlexPuff response frames |Consider also adding a FlexPuff bullet to the "Critical gotcha" section mirroring the Holder 0xC9 → 0x08 example.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/rules/project.md` around lines 62 - 82, The command-prefix table is
inverted for FlexPuff: change the table so 0xD2 is listed as the FlexPuff
request prefix (LOAD_FLEXPUFF_COMMAND request byte 1 = 0xD2) and 0x90 is listed
as the FlexPuff response marker (parser checks bytes[1] == 0x90); also add a
short example in the "Critical gotcha" section mirroring the Holder example
(e.g., request [0x00, 0xD2, ...] → response [0x00, 0x90, ...]) so readers see
the request-vs-response discriminant for LOAD_FLEXPUFF_COMMAND and
FirmwareKind/response matching.
nonnil
force-pushed
the
fix/device-capability-matrix
branch
from
7d87248 to
9b1b56e
Compare
|
@coderabbitai @copilot review |
|
✅ Actions performedReview triggered.
|
Summary
Add
DeviceCapability::ChargeStartVibrationand routesupports_charge_start_vibration()through the capability matrix.Replace the capability tests with an explicit model-by-capability matrix covering all current variants.
Align FlexPuff documentation with the current ILUMA i-only capability gate and add the project rules capability matrix.
closes Audit and refine per-model capability matrix in DeviceCapability #3
Validation
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets --all-featuresRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featurescargo check --all-targets --all-featuresSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor