feat: add device status read - #2
Conversation
- Add `src/protocol/status.rs` with `DeviceStatus` struct holding stick_firmware, holder_firmware (folder-type only), battery_voltage (non-fatal) - Implement `Iqos::read_device_status(model)` — reads stick firmware always, holder firmware only when model.supports_holder_features(), battery voltage via .ok() - Export `DeviceStatus` from protocol module and crate root - Update `inspect_device` CLI to call read_device_status and print results - Add `print_device_status` helper to main.rs - Add 4 unit tests covering one-piece, folder-type, battery failure, and firmware error paths
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Just let me know what you need! 🐇 |
review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Client as BLE Inspection Flow
participant Iqos as Iqos API
participant Transport as Transport Layer
participant Device as IQOS Device
Client->>Iqos: read_device_status(model, device_info)
Iqos->>Transport: read_stick_firmware()
Transport->>Device: diagnostic request
Device-->>Transport: firmware response
Transport-->>Iqos: FirmwareVersion
alt model.supports_holder_features()
Iqos->>Transport: read_holder_firmware()
Transport->>Device: diagnostic request
Device-->>Transport: firmware response
Transport-->>Iqos: FirmwareVersion
else
Iqos->>Iqos: holder_firmware = None
end
Iqos->>Transport: read_battery_voltage()
Transport->>Device: diagnostic request
alt Battery read succeeds
Device-->>Transport: voltage response
Transport-->>Iqos: f32
else Battery read fails (transport)
Transport-->>Iqos: Error::Transport -> battery_voltage = None
end
Iqos-->>Client: DeviceStatus { model, device_info, stick_firmware, holder_firmware?, battery_voltage? }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/protocol/status.rs (1)
3-16: Consider decoupling the doc from specific model names.The doc enumerates
Iluma,IlumaIas the folder-type models, but the actual decision is made inIqos::read_device_statusviaDeviceModel::supports_holder_features(). If that capability set changes, this comment will silently drift. Prefer phrasing it in terms of the capability (e.g. "models wheresupports_holder_features()returns true").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protocol/status.rs` around lines 3 - 16, The doc comment on DeviceStatus ties behavior to specific models (Iluma, IlumaI); instead, rewrite the documentation to describe behavior in terms of the model capability used in Iqos::read_device_status (i.e., whether DeviceModel::supports_holder_features() is true). Update the fields' docs for stick_firmware, holder_firmware and battery_voltage to say "for models where supports_holder_features() is true" rather than naming specific models, and confirm the narrative matches how Iqos::read_device_status populates these fields.src/lib.rs (2)
1100-1184: Good coverage; one gap worth adding.The four new tests cover one-piece, folder, battery failure, and stick-firmware failure paths well. Consider adding a symmetric case for a folder-model holder-firmware error to lock in that holder-read errors do propagate (the doc comment explicitly states firmware reads are fatal). Optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 1100 - 1184, Add a test that exercises read_device_status for DeviceModel::Iluma where the transport returns a successful stick-firmware response then an Err for the holder firmware to assert that the error is propagated (i.e., read_device_status returns Err). Use the same test pattern and helpers (MockTransport::with_responses, Iqos::new, block_on) and reference protocol::LOAD_STICK_FIRMWARE_VERSION_COMMAND and protocol::LOAD_HOLDER_FIRMWARE_VERSION_COMMAND to ensure the holder-read error path is hit and the returned error matches Error::Transport.
385-394: Battery.ok()swallows decode errors silently — consider narrowing or logging.
self.read_battery_voltage().await.ok()discards every error variant, includingError::ProtocolDecode(malformed frame) andError::Unsupported, not just transient transport failures. Callers only seeNoneand cannot distinguish "device offline" from "protocol regression". The CLI inmain.rsalready handles both the innerNoneand an outerErr, so at minimum it would be useful to emit alog::warn!/tracing::warn!when swallowing, or restrict the fallback to transport-class errors:Optional narrower fallback
- let battery_voltage = self.read_battery_voltage().await.ok(); + let battery_voltage = match self.read_battery_voltage().await { + Ok(v) => Some(v), + Err(Error::Transport(_)) => None, + Err(e) => return Err(e), + };Not blocking — the current behavior matches the documented contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 385 - 394, The call to self.read_battery_voltage().await.ok() silently discards all error kinds (including ProtocolDecode/Unsupported); change it to match the Result from read_battery_voltage() in read_device_status so you only swallow transport-class failures or at least log non-transport failures. For example, replace the .ok() with a match on self.read_battery_voltage().await that returns Some(voltage) on Ok, returns None for known transport errors, and for other errors calls tracing::warn!(error = ?e, "failed to read battery voltage, swallowing") (or process_logger.warn) before returning None; reference the read_device_status method and the read_battery_voltage call and handle Error::ProtocolDecode / Error::Unsupported specially.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib.rs`:
- Around line 1100-1184: Add a test that exercises read_device_status for
DeviceModel::Iluma where the transport returns a successful stick-firmware
response then an Err for the holder firmware to assert that the error is
propagated (i.e., read_device_status returns Err). Use the same test pattern and
helpers (MockTransport::with_responses, Iqos::new, block_on) and reference
protocol::LOAD_STICK_FIRMWARE_VERSION_COMMAND and
protocol::LOAD_HOLDER_FIRMWARE_VERSION_COMMAND to ensure the holder-read error
path is hit and the returned error matches Error::Transport.
- Around line 385-394: The call to self.read_battery_voltage().await.ok()
silently discards all error kinds (including ProtocolDecode/Unsupported); change
it to match the Result from read_battery_voltage() in read_device_status so you
only swallow transport-class failures or at least log non-transport failures.
For example, replace the .ok() with a match on self.read_battery_voltage().await
that returns Some(voltage) on Ok, returns None for known transport errors, and
for other errors calls tracing::warn!(error = ?e, "failed to read battery
voltage, swallowing") (or process_logger.warn) before returning None; reference
the read_device_status method and the read_battery_voltage call and handle
Error::ProtocolDecode / Error::Unsupported specially.
In `@src/protocol/status.rs`:
- Around line 3-16: The doc comment on DeviceStatus ties behavior to specific
models (Iluma, IlumaI); instead, rewrite the documentation to describe behavior
in terms of the model capability used in Iqos::read_device_status (i.e., whether
DeviceModel::supports_holder_features() is true). Update the fields' docs for
stick_firmware, holder_firmware and battery_voltage to say "for models where
supports_holder_features() is true" rather than naming specific models, and
confirm the narrative matches how Iqos::read_device_status populates these
fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e7f6922-0fb5-44e2-b71d-947f11e967e7
📒 Files selected for processing (4)
src/lib.rssrc/main.rssrc/protocol/mod.rssrc/protocol/status.rs
There was a problem hiding this comment.
Pull request overview
Adds a typed “device status” snapshot API to the IQOS library (aggregating firmware versions plus an optional battery voltage reading), and wires it into the debug CLI for easier inspection.
Changes:
- Introduce
DeviceStatusand re-export it throughprotocoland crate root. - Add
Iqos::read_device_status(model)to read stick firmware, optional holder firmware, and non-fatal battery voltage. - Expand holder-capability gating to include PRIME models and add unit tests covering sequencing and error semantics.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/protocol/types.rs |
Updates holder-capability detection and related capability gating; adds tests for the expanded model matrix. |
src/protocol/status.rs |
Adds the new DeviceStatus snapshot type. |
src/protocol/mod.rs |
Registers the new module and re-exports DeviceStatus. |
src/lib.rs |
Exposes DeviceStatus at crate root and implements Iqos::read_device_status; adds unit tests. |
src/main.rs |
Updates inspect flow to print GATT battery level and the aggregated device status. |
.gitignore |
Adds docs/firmware_extraction ignore entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 275-286: print_device_status currently treats a None
holder_firmware as "no holder support", which can mislead if None could also
mean a read failure; update iqos::DeviceStatus and print_device_status to
distinguish unsupported vs read-failed: change DeviceStatus.holder_firmware from
Option<String> to an enum (e.g., HolderFirmware::Unsupported |
HolderFirmware::Present(String) | HolderFirmware::ReadFailed) or add a separate
status field, adjust the producer (iqos::read_device_status) to populate the new
variant on failures, and update print_device_status to match on the new enum (or
field) to print "n/a (no holder support)", the firmware string, or "read failed"
accordingly so the CLI output remains accurate.
🪄 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: 26ce48fe-01f8-403f-b6da-f851bd9cb66b
📒 Files selected for processing (5)
.gitignoresrc/lib.rssrc/main.rssrc/protocol/status.rssrc/protocol/types.rs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- src/protocol/status.rs
- src/lib.rs
| #[cfg(feature = "btleplug-support")] | ||
| fn print_device_status(status: &iqos::DeviceStatus) { | ||
| println!("Stick firmware: {}", status.stick_firmware); | ||
| match &status.holder_firmware { | ||
| Some(fw) => println!("Holder firmware: {fw}"), | ||
| None => println!("Holder firmware: n/a (no holder support)"), | ||
| } | ||
| match status.battery_voltage { | ||
| Some(v) => println!("Battery voltage: {v:.3} V"), | ||
| None => println!("Battery voltage: read failed"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor: None holder firmware is reported as "no holder support" even on read failure.
print_device_status assumes status.holder_firmware == None means the model doesn't support a holder. If Iqos::read_device_status ever surfaces a holder-supported model with a failed/absent holder firmware read as None (rather than propagating an error), the CLI will misleadingly print n/a (no holder support). If holder-firmware errors are always propagated via the outer Result today this is fine, but consider distinguishing "unsupported" from "read failed" at the DeviceStatus level (e.g., keep Option for unsupported and return Err for failures, or use an enum) to keep the CLI output honest as the API evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 275 - 286, print_device_status currently treats a
None holder_firmware as "no holder support", which can mislead if None could
also mean a read failure; update iqos::DeviceStatus and print_device_status to
distinguish unsupported vs read-failed: change DeviceStatus.holder_firmware from
Option<String> to an enum (e.g., HolderFirmware::Unsupported |
HolderFirmware::Present(String) | HolderFirmware::ReadFailed) or add a separate
status field, adjust the producer (iqos::read_device_status) to populate the new
variant on failures, and update print_device_status to match on the new enum (or
field) to print "n/a (no holder support)", the firmware string, or "read failed"
accordingly so the CLI output remains accurate.
DeviceStatus previously held only SCP-layer fields (firmware versions, battery voltage). The iqos_cli reference shows that a full device info snapshot should also include model, model number, serial number, software revision, and manufacturer name — all available from GATT at connection time. Changes: - Add model: DeviceModel and device_info: DeviceInfo to DeviceStatus - Update read_device_status signature to accept device_info from caller (already available via IqosBle after connect_and_discover) - Rewrite print_device_status to mirror iqos_cli output format: holder models show Stick/Holder sections; one-piece models show flat layout - Simplify inspect_device: single print_device_status call covers all fields - Update all unit tests to pass DeviceInfo::default() as second arg Product number fields (stick and holder) remain unimplemented — SCP response format is unknown and no packet captures exist.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 127-138: The GATT battery result stored in battery_level is only
printed inside the Ok branch of Iqos::read_device_status, so on device status
read failures you lose battery diagnostics; update the control flow around
Iqos::new(session) and read_device_status(model, device_info).await so that
battery_level is inspected and printed regardless of whether read_device_status
returns Ok or Err — e.g., after matching read_device_status (or by moving the
match on battery_level out of the Ok arm), always match battery_level and print
either "Battery level (GATT): {level}%" or "Battery level (GATT): read failed
({error})"; keep existing calls to Iqos::new, read_device_status, and
print_device_status unchanged except for relocating/duplicating the
battery_level printing.
🪄 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: 8d6f29c3-d4fc-42fa-89d5-2fd89a8b46d9
📒 Files selected for processing (3)
src/lib.rssrc/main.rssrc/protocol/status.rs
✅ Files skipped from review due to trivial changes (1)
- src/protocol/status.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
Summary
DeviceStatusas a typed status snapshot for stick firmware, optional holder firmware, and optional battery voltage.Iqos::read_device_status(model)to aggregate firmware reads and a non-fatal battery voltage read.inspectflow to print GATT battery level plus the aggregated device status.Validation
cargo fmt --all --checkcargo test --all-targets --all-featurescargo clippy --all-targets --all-features -- -D warningsNotes
.gitignoreand an untrackedpackage-lock.json; they are not part of this pushed branch commit.Summary by CodeRabbit
New Features
Tests
Chores