Skip to content

Comments

battery-service: minimize panic code paths#639

Merged
jerrysxie merged 5 commits intoOpenDevicePartnership:mainfrom
jerrysxie:depanic-battery-service
Dec 20, 2025
Merged

battery-service: minimize panic code paths#639
jerrysxie merged 5 commits intoOpenDevicePartnership:mainfrom
jerrysxie:depanic-battery-service

Conversation

@jerrysxie
Copy link
Contributor

@jerrysxie jerrysxie commented Dec 15, 2025

This pull request refactors error handling and buffer copying logic in the battery service code to make it more robust and explicit. The main improvements are in the way slices are copied into fixed-size buffers and how errors are handled and propagated in the state machine.

Buffer copying improvements:

  • Refactored the buffer copying logic in compute_bix (acpi.rs) to use safer and more explicit slice operations with get_mut and ok_or, replacing the previous use of try_into and direct indexing. This reduces the chance of panics and makes error handling clearer.

Error handling and state machine robustness:

  • Updated the state machine timeout handling in Context (context.rs) to properly propagate errors: now, if the state machine returns an error, it sends a StateError instead of assuming infallibility.
  • Changed the handling of BatteryEventInner::Oem events to return a specific InvalidActionInState error instead of an unimplemented panic (todo!()), making the state machine more robust and explicit about unsupported actions.

@jerrysxie jerrysxie self-assigned this Dec 15, 2025
Copilot AI review requested due to automatic review settings December 15, 2025 23:48
@jerrysxie jerrysxie added the enhancement New feature or request label Dec 15, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors error handling in the battery service by removing expect() calls and replacing todo!() macros with proper error handling, making the code more robust and production-ready.

Key Changes:

  • Replaced expect() with explicit match-based error handling in the timeout path
  • Implemented error handling for the OEM event case instead of using todo!()
  • Added clippy suppression attributes for intentional slice indexing operations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
battery-service/src/context.rs Replaced panic-inducing expect() with proper error propagation; implemented error handling for OEM events
battery-service/src/acpi.rs Added clippy allow attributes to suppress false-positive warnings on bounds-checked slice operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jerrysxie jerrysxie marked this pull request as ready for review December 16, 2025 00:00
@jerrysxie jerrysxie requested a review from a team as a code owner December 16, 2025 00:00
@jerrysxie jerrysxie changed the title battery-service: minimize audit code paths battery-service: minimize panic code paths Dec 16, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

RobertZ2011
RobertZ2011 previously approved these changes Dec 16, 2025
kurtjd
kurtjd previously approved these changes Dec 17, 2025
@jerrysxie jerrysxie dismissed stale reviews from kurtjd and RobertZ2011 via 4e5f4fc December 17, 2025 18:26
@jerrysxie jerrysxie force-pushed the depanic-battery-service branch from 9172dc2 to 4e5f4fc Compare December 17, 2025 18:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

williampMSFT
williampMSFT previously approved these changes Dec 17, 2025
Copy link
Contributor

@williampMSFT williampMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@jerrysxie jerrysxie enabled auto-merge (squash) December 19, 2025 17:05
@jerrysxie
Copy link
Contributor Author

Bypassed policy to merge as the entire battery team is on vacation.

@jerrysxie jerrysxie disabled auto-merge December 20, 2025 00:16
@jerrysxie jerrysxie merged commit 1621263 into OpenDevicePartnership:main Dec 20, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants