Skip to content

Comments

hid-service: Depanic#641

Merged
jerrysxie merged 3 commits intoOpenDevicePartnership:mainfrom
RobertZ2011:hid-depanic
Dec 19, 2025
Merged

hid-service: Depanic#641
jerrysxie merged 3 commits intoOpenDevicePartnership:mainfrom
RobertZ2011:hid-depanic

Conversation

@RobertZ2011
Copy link
Contributor

@RobertZ2011 RobertZ2011 commented Dec 16, 2025

Breaking

Introduces a struct to with named fields for indexing range errors. Interrupt passthrough processing function now returns a result. Migration is to move over to the named indexing error struct and properly handle the interrupt error if using passthrough.

@RobertZ2011 RobertZ2011 self-assigned this Dec 16, 2025
Copilot AI review requested due to automatic review settings December 16, 2025 23:29
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 systematically removes panic-prone code patterns (unwrap(), panic!(), unimplemented!()) from the hid-service and replaces them with proper error handling, making the code more resilient for embedded systems.

Key changes:

  • Introduced InvalidSizeError struct to replace tuple-based error data in Error::InvalidSize
  • Converted buffer indexing from panicking direct access to safe .get()/.get_mut() with error handling
  • Replaced unwrap() calls with .map_err() for explicit error propagation
  • Changed unimplemented!() to return None for unhandled request types

Reviewed changes

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

Show a summary per file
File Description
hid-service/src/i2c/passthrough/interrupt.rs Added Error enum and changed process() to return Result<(), Error> instead of panicking on GPIO errors
hid-service/src/i2c/host.rs Refactored error handling throughout: replaced unwrap() with map_err(), changed buffer indexing to use get()/get_mut() with ok_or() error handling
hid-service/src/i2c/device.rs Replaced unwrap() with match expressions, changed buffer slicing to use get_mut(), replaced unimplemented!() with None
embedded-service/src/hid/mod.rs Introduced InvalidSizeError struct and updated Error::InvalidSize variant to use it instead of tuple
embedded-service/src/hid/command.rs Updated all Error::InvalidSize instantiations to use new InvalidSizeError struct

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

@RobertZ2011 RobertZ2011 added the BREAKING CHANGE Marks breaking changes label Dec 16, 2025
Copilot AI review requested due to automatic review settings December 16, 2025 23:52
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 5 out of 5 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.

kurtjd
kurtjd previously approved these changes Dec 17, 2025
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:

Copilot AI review requested due to automatic review settings December 17, 2025 22:42
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 5 out of 5 changed files in this pull request and generated 2 comments.


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

Copy link
Contributor

@jerrysxie jerrysxie left a comment

Choose a reason for hiding this comment

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

What testing have we done with these changes? Do we have simulated integration test for HID?

@jerrysxie
Copy link
Contributor

What testing have we done with these changes? Do we have simulated integration test for HID?

This breaking changes should be minimal impact to our clients.

@RobertZ2011
Copy link
Contributor Author

What testing have we done with these changes? Do we have simulated integration test for HID?

This breaking changes should be minimal impact to our clients.

There should be no behavioral changes except for no more panics in this code. I tested these changes against a HID keyboard implementation and saw no differences.

@RobertZ2011
Copy link
Contributor Author

Holding off on merging for the moment to allow some downstream blockages to clear.

@jerrysxie jerrysxie enabled auto-merge (squash) December 19, 2025 17:46
@jerrysxie jerrysxie merged commit 653eb8d into OpenDevicePartnership:main Dec 19, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKING CHANGE Marks breaking changes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants