refactor(sensors): update crates, eliminate all panics in our code by itscrystalline · Pull Request #56 · Punnawithsut/SignGo

itscrystalline · 2026-02-25T16:25:40Z

No description provided.

Copilot

Pull request overview

This PR refactors the sensors crate to eliminate all panics in the codebase and updates dependencies to the latest esp-hal main branch. The changes enforce strict clippy lints that deny panic-related operations and replace them with error logging using a new backtrace macro.

Changes:

Added clippy lint denials for panic-related operations (unwrap, expect, panic, etc.) across the codebase
Replaced panic! calls with error! logging and backtrace macro invocations
Replaced safe unwrap/expect calls with unsafe unwrap_unchecked to satisfy the new lints
Updated all esp-hal dependencies from published crates to git-based dependencies at rev 6168bc7
Updated bt-hci (0.6.0 → 0.8.0) and trouble-host (0.5.0 → 0.6.0) to compatible versions
Refactored GloveData::encode to encode_into with compile-time size guarantees
Added Saturating arithmetic for error counters to prevent overflow panics
Updated API calls for esp_rtos::start and BleConnector to match new signatures

Reviewed changes

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

Show a summary per file

File	Description
sensors/src/lib.rs	Added clippy deny directives and backtrace! macro for error diagnostics
sensors/src/gyro.rs	Replaced safe unwraps with unsafe unwrap_unchecked, added Saturating arithmetic for error counter, integrated backtrace macro
sensors/src/ble.rs	Replaced panic! with error logging + backtrace, refactored encode method, used unsafe unwrap_unchecked for initialization, added clippy allow directive
sensors/src/bin/main.rs	Added clippy deny directives, updated API calls for esp_rtos and BleConnector, used unsafe unwrap_unchecked for task spawning
sensors/Cargo.toml	Updated dependencies to git-based esp-hal at rev 6168bc7, updated bt-hci and trouble-host versions
sensors/Cargo.lock	Lock file updates reflecting new dependency sources and versions
devenv.nix	Added bacon development tool

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

Copilot · 2026-02-25T16:32:26Z

-    }))
-    .unwrap();
+    let server = unsafe {
+        // SAFETY: `new_with_config` returns Err() if it's name is >22 bytes long, which "SignGoGlove" is not.


Grammar error in SAFETY comment: "if it's name is" should be "if its name is".

Suggested change

// SAFETY: `new_with_config` returns Err() if it's name is >22 bytes long, which "SignGoGlove" is not.

// SAFETY: `new_with_config` returns Err() if its name is >22 bytes long, which "SignGoGlove" is not.

Copilot · 2026-02-25T16:32:27Z

+                let q = unsafe {
+                    // SAFETY: b's len() is already checked in the match statement above
+                    Quaternion::from_bytes(res_buf.get(..16).unwrap_unchecked()).unwrap_unchecked()
+                }
+                .normalize();


This line uses unwrap_unchecked twice: once for get() and once for Quaternion::from_bytes(). While the SAFETY comment addresses the get() call, it doesn't address the from_bytes() call. If Quaternion::from_bytes can return an error for malformed data (even with the correct length), this will cause undefined behavior. Consider documenting why from_bytes cannot fail with valid-length input, or use safe error handling.

Suggested change

let q = unsafe {

// SAFETY: b's len() is already checked in the match statement above

Quaternion::from_bytes(res_buf.get(..16).unwrap_unchecked()).unwrap_unchecked()

}

.normalize();

let q = {

// SAFETY: res_buf's len() is already checked in the match statement above

let bytes = unsafe { res_buf.get(..16).unwrap_unchecked() };

let q = match Quaternion::from_bytes(bytes) {

Ok(q) => q,

Err(e) => {

error!("invalid quaternion data: {e}");

errors += 1;

continue;

}

};

q.normalize()

};

Copilot · 2026-02-25T16:32:27Z

            if let Err(e) = runner.run().await {
-                panic!("[ble_task] error: {:?}", e);
+                error!("[ble_task] error: {:?}", e);
+                backtrace!("ble_task");


Replacing panic with error logging and continuing the loop may mask critical issues. The BLE runner task error typically indicates a serious problem that may require reinitialization. Consider whether the system can safely continue after such errors, or if this should trigger a system reset or halt.

Suggested change

backtrace!("ble_task");

backtrace!("ble_task");

break;

Copilot · 2026-02-25T16:32:27Z

                Err(e) => {
-                    panic!("[adv] error: {:?}", e);
+                    error!("[adv] error: {:?}", e);
+                    backtrace!("adv");
                }


Replacing panic with error logging means advertising errors will be silently logged but the system continues attempting to advertise. This could lead to infinite error loops consuming resources. Consider implementing exponential backoff or a maximum retry count before taking more drastic action.

Copilot · 2026-02-25T16:32:27Z

+# esp-rs/esp-hal rev 6168bc7 (6168bc775349ba35e3c27bb527defccb22a44658) latest main at 25/02/2026
+esp-hal = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7", features = ["log-04", "esp32", "unstable"] }

-esp-rtos = { version = "0.2.0", features = [
+esp-rtos = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7", features = [
  "log-04",
  "embassy",
  "esp-alloc",
  "esp-radio",
  "esp32",
 ] }

-esp-bootloader-esp-idf = { version = "0.4.0", features = ["log-04", "esp32"] }
+esp-bootloader-esp-idf = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7", features = ["log-04", "esp32"] }
 log                    = "0.4.27"

-bt-hci = "0.6.0"
+bt-hci = "0.8.0"
 embassy-executor = { version = "0.9.1", features = ["log"] }
 embassy-time = { version = "0.5.0", features = ["log"] }
 embedded-io = "0.7.1"
 embedded-io-async = "0.7.0"
-esp-alloc = "0.9.0"
-esp-backtrace = { version = "0.18.1", features = [
+esp-alloc = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7" }
+esp-backtrace = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7", features = [
  "esp32",
  "panic-handler",
  "println",
 ] }
-esp-println = { version = "0.16.1", features = ["esp32", "log-04"] }
-esp-radio = { version = "0.17.0", features = [
+esp-println = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7", features = ["esp32", "log-04"] }
+esp-radio = { git = "https://github.com/esp-rs/esp-hal", rev = "6168bc7", features = [
  "ble",
  "log-04",
  "esp-alloc",
  "esp32",
  "unstable",
 ] }
-trouble-host = { version = "0.5.0", features = ["gatt"] }
+trouble-host = { version = "0.6.0", features = ["gatt"] }


The Cargo.toml now uses git dependencies with a specific rev (6168bc7) instead of published crate versions. While this allows using the latest features, it has several implications: 1) builds are less reproducible if the git repository is modified, 2) dependency resolution is slower, 3) it's harder to track security advisories for specific versions. Consider whether these dependencies will be published to crates.io soon, or document why git dependencies are necessary.

Copilot · 2026-02-25T16:32:28Z

+    // SAFETY: `I2c::new(..)` errors when passed in an invalid config, however we do pass in a
+    // valid config.
+    let mut i2c = unsafe { I2c::new(i2c, config).unwrap_unchecked() }


The SAFETY comment claims that I2c::new only errors with an invalid config, but this is an assumption that should be verified. The unsafe unwrap_unchecked will cause undefined behavior if I2c::new ever returns an Err for any reason (including future API changes). Consider using a regular .expect() with a descriptive message instead, or handle the error explicitly if eliminating panics is truly required.

Suggested change

// SAFETY: `I2c::new(..)` errors when passed in an invalid config, however we do pass in a

// valid config.

let mut i2c = unsafe { I2c::new(i2c, config).unwrap_unchecked() }

let mut i2c = I2c::new(i2c, config)

.expect("Failed to create I2C bus with given configuration")

Copilot · 2026-02-25T16:32:28Z

+                // SAFETY: `len` is always shorter than `advertiser_data.len()`
+                adv_data: unsafe { advertiser_data.get(..len).unwrap_unchecked() },


The SAFETY comment claims the len is always shorter than advertiser_data.len(), but this assumption depends on the implementation of AdStructure::encode_slice. If encode_slice returns a len value that exceeds the buffer length, this would cause undefined behavior. Consider verifying this assumption with an assertion or using a safe alternative like .get(..len).ok_or(error)?

Suggested change

// SAFETY: `len` is always shorter than `advertiser_data.len()`

adv_data: unsafe { advertiser_data.get(..len).unwrap_unchecked() },

// `len` is expected to be shorter than or equal to `advertiser_data.len()`

adv_data: &advertiser_data[..len],

Copilot · 2026-02-25T16:32:28Z

@@ -1,3 +1,5 @@
+#![allow(clippy::needless_borrows_for_generic_args)]


The clippy allow directive for needless_borrows_for_generic_args suggests that the API changes in this file (removing & from some references) may be triggering clippy warnings. Consider whether these reference changes are actually necessary or if they're working around an API compatibility issue that should be addressed differently.

Suggested change

#![allow(clippy::needless_borrows_for_generic_args)]

Copilot · 2026-02-25T16:32:28Z

+    fn encode_into(&self, buf: &mut [u8; DATA_SIZE]) {
+        self.fingers
+            .iter()
+            .flat_map(|u| u.to_be_bytes())
+            .chain(self.quaternion.iter().flat_map(|f| f.to_be_bytes()))
+            .chain(self.acceleration.iter().flat_map(|f| f.to_be_bytes()))
+            .chain(self.gyroscope.iter().flat_map(|f| f.to_be_bytes()))
+            .zip(buf.iter_mut())
+            .for_each(|(src, dest)| {
+                *dest = src;
+            });


The encode_into method uses zip() to pair source bytes with destination buffer elements. While this is safe and elegant, it silently truncates if the iterator chain produces more bytes than DATA_SIZE. Consider adding a debug assertion or comment to verify that the total byte count (52 + 44 + 34 + 34 = 50) equals DATA_SIZE to ensure no data is lost.

itscrystalline added 5 commits February 25, 2026 21:24

chore(sensors): add bacon to devenv

d8429f6

chore(sensors): update bt-hci and trouble-host

aa939ec

chore(sensors): update all esp* crates to git main (at time of commit)

6b614fc

refactor(sensors): update init code to new version

998e9db

refactor(sensors): replaced panic points w/ errors, added clippy lints

15b5f2a

itscrystalline linked an issue Feb 25, 2026 that may be closed by this pull request

[Refactor/sensors]: update crates and guarentee no panics #55

Closed

itscrystalline requested a review from Copilot February 25, 2026 16:25

Copilot started reviewing on behalf of itscrystalline February 25, 2026 16:26 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

itscrystalline added 3 commits February 25, 2026 23:35

fix(sensors): fix panics behind feature gates

662bbcf

docs(sensors): clarify other unwrap in quaternion parsing

9f81951

feat(sensors): add backoff if advertisinpg fail

3ed36ea

itscrystalline merged commit c595d04 into main Feb 25, 2026
6 checks passed

Navigation Menu

refactor(sensors): update crates, eliminate all panics in our code - #56

refactor(sensors): update crates, eliminate all panics in our code#56
itscrystalline merged 8 commits into
mainfrom
update-crates-no-panics

itscrystalline commented Feb 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// SAFETY: `new_with_config` returns Err() if it's name is >22 bytes long, which "SignGoGlove" is not.
	// SAFETY: `new_with_config` returns Err() if its name is >22 bytes long, which "SignGoGlove" is not.

-                let q = unsafe {
-                    // SAFETY: b's len() is already checked in the match statement above
-                    Quaternion::from_bytes(res_buf.get(..16).unwrap_unchecked()).unwrap_unchecked()
-                }
-                .normalize();
+                let q = {
+                    // SAFETY: res_buf's len() is already checked in the match statement above
+                    let bytes = unsafe { res_buf.get(..16).unwrap_unchecked() };
+                    let q = match Quaternion::from_bytes(bytes) {
+                        Ok(q) => q,
+                        Err(e) => {
+                            error!("invalid quaternion data: {e}");
+                            errors += 1;
+                            continue;
+                        }
+                    };
+                    q.normalize()
+                };

		// SAFETY: `len` is always shorter than `advertiser_data.len()`
		adv_data: unsafe { advertiser_data.get(..len).unwrap_unchecked() },

		@@ -1,3 +1,5 @@
		#![allow(clippy::needless_borrows_for_generic_args)]

Conversation

itscrystalline commented Feb 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants