fix(iio): improve AccelGyro3d driver for HID Sensor Hub devices by honjow · Pull Request #574 · ShadowBlip/InputPlumber

honjow · 2026-04-03T10:16:27Z

Summary

Fix the AccelGyro3d IIO driver for HID Sensor Hub devices (Intel ISH, AMD SFH) where accelerometer and gyroscope are exposed as separate IIO devices (accel_3d, gyro_3d).

Changes

Handle separate accel/gyro IIO devices

The previous code assumed both accel and gyro channels were always present in one device, emitting zero-valued events for the missing sensor. This caused alternating valid/zero data on HID Sensor Hub hardware (e.g. MSI Claw).

Detect capabilities dynamically via has_accel() / has_gyro()
Return None from poll when no channels are found
Keep the IIO Device alive in Driver to prevent Channel pointer invalidation

Correct scaling factors

The previous scaling factors (* 10.0 for accel, * (180/π) * 1500.0 for gyro) were empirically tuned on the Legion Go — the original commit explicitly states they were derived "from testing" to make motion "feel like natural 1:1". These values were likely calibrated against the amd_sfh kernel driver's imprecise output (the driver has known integer division errors that reduce accel values by 10× and gyro by 100×), rather than against correct SI-unit data.

This PR replaces them with physically derived values based on the Steam Deck UHID protocol constants:

Accel: 1 / 0.0006125 (m/s² → UHID LSB)
Gyro: (180/π) / 0.0625 (rad/s → °/s → UHID LSB)

These factors assume the IIO subsystem reports correct SI-unit values. Hardware-specific sensitivity is handled by the per-device scale sysfs attribute. Verified on MSI Claw A1M (Intel ISH) and MSI Claw A8 BZ2EM (AMD SFH with kernel precision patch) — both produce correct and consistent output.

Impact on Legion Go: When the hid-lenovo-go kernel driver is loaded, InputPlumber already disables the internal IMU and uses the controller's built-in gyroscope instead (get_default_event_filter), so this change has no effect. On kernels without hid-lenovo-go where the internal IMU is active, the new factors may produce different output if the amd_sfh precision bug is present — but the old values were not physically correct either.

Initialize sampling rate on startup

Some HID Sensor Hub devices default to very low rates (e.g. 10 Hz on MSI Claw), which is insufficient for gaming input. The driver now initializes the rate with priority: YAML config > hardware max available > 200 Hz default. Supports both per-channel and device-level attributes with fallback.

Test plan

MSI Claw A1M (Intel ISH, separate accel_3d + gyro_3d)
MSI Claw A8 BZ2EM (AMD SFH). Note: stock amd_sfh kernel driver has precision issues requiring a kernel patch, independent of InputPlumber.
BMI260 device — no regression in shared Driver code
Verify no regression on Legion Go / ROG Ally

HID Sensor Hub devices (Intel ISH, AMD SFH) expose accelerometer and gyroscope as separate IIO devices. The previous code assumed both were always present, causing alternating zero-value events. - Detect capabilities dynamically from available channels - Return None from poll_accel/poll_gyro when channels are empty - Keep IIO Device alive in struct to prevent Channel pointer invalidation

Replace empirical scaling multipliers with physically derived values from the Steam Deck UHID protocol constants: - Accelerometer: 1/0.0006125 (m/s² → UHID LSB) - Gyroscope: (180/π)/0.0625 (rad/s → °/s → UHID LSB) These factors apply universally to any IIO device that correctly reports SI-unit values; hardware-specific sensitivity is already handled by the per-device scale attribute in sysfs.

HID Sensor Hub devices default to 10 Hz which is too slow for gaming. Priority: config value > hardware max available > 200 Hz default. Supports both per-channel (BMI-style) and device-level (HID Sensor Hub) sampling_frequency attributes with automatic fallback.

pastaq · 2026-04-15T08:26:07Z

    //a standalone crate. When this driver is eventually separated, refactor the Event type to
    //follow the pattern DeviceEvent(Event, Value) and create a match table for
    //Capability->Event/Event->Capability in the SourceDriver implementation.
+    pub fn has_accel(&self) -> bool {


These functions seem fairly redundant to me. Please remove them and just do the same check they are already doing

pastaq

Any links to substantiate the SI units claim would be good in the commit description.

pastaq · 2026-04-15T08:31:25Z

 fn translate_event(event: iio_imu::event::Event) -> NativeEvent {
    match event {
        iio_imu::event::Event::Accelerometer(data) => {
+            let factor = 1.0 / 0.0006125; // m/s² → UHID LSB


This never changes. Instead of recalculating it every time please declare this as a const f64 1632.65

pastaq · 2026-04-15T08:34:19Z

-            // Adjusting the scale is not possible on the accel_gyro_3d IMU.
-            // From testing this is the highest scale we can apply before noise
-            // is amplified to the point the gyro cannot calibrate.
+            let factor = (180.0 / PI) / 0.0625; // rad/s → UHID LSB


Same for this, const f64 916.73

pastaq · 2026-04-15T08:37:54Z

        }

+        if !accel.is_empty() {
+            set_sample_rate(&device, &accel, ChannelType::Accel, sample_rate);


I don't really like this function as is.

The function doesn't check if the sample rate provided is valid.

If no sample rate is provided, it either uses the max (unintuitive to the function name), or uses a default which isn't checked as available. This will provide inconsistent behavior that is undefined.

The function name doesn't provide any of this context, so depending on how it's used you have two paths to force undefined behavior and one that provides a random result.

I would have two functions:

One called set_sample_rate_or_default() that takes the rate as an option. This function should throw a warning under two circumstances. 1.) The requested rate isn't available. 2.) The default isn't available. If the first failcondition is hit, or if no rate is provided, it should fall back to the default.

One called set_sample_rate_max that you fall back to if the first one warns. This should produce an error if it fails, but the driver should keep working.

I edited this to be more clear. Please adjust.

Also, your changes were very quick which seems tool assisted. Please disclose any use of AI tooling.

pastaq · 2026-04-15T08:38:04Z

+            set_sample_rate(&device, &accel, ChannelType::Accel, sample_rate);
+        }
+        if !gyro.is_empty() {
+            set_sample_rate(&device, &gyro, ChannelType::AnglVel, sample_rate);


pastaq · 2026-04-15T08:48:23Z

+) {
+    let rate = if let Some(r) = target_rate {
+        log::debug!("Using configured sample rate: {r} Hz");
+        r


Please don't represent variables with single letters outside of using Err(e) which is universally understandable in context. Explicit variable names are preferred and shadowing won't happen in rust with the conplilers scope enforcement.

pastaq · 2026-04-15T09:03:12Z

+                return;
+            }
+            Err(e) => {
+                log::debug!(


I don't like hiding errors that obscure information like this behind a debug command as this has an appreciable effect on the behavior of the device. After you refactor the previous function, ensure that both failing to set the config/default rate and if the fallback to max fails that they at least produce a warning.

pastaq · 2026-04-15T09:09:01Z

+
+    match device.attr_write_float(attr, rate) {
+        Ok(_) => {
+            let actual = device.attr_read_float(attr).unwrap_or(rate);


This also obscures an error reading from the sysfs. Please handle the error oth a warning and then report that we're assuming a successful write to the rate.

- Remove has_accel()/has_gyro() wrapper methods, revert to static CAPABILITIES (empty-channel early return in poll is sufficient) - Move scaling factors to module-level const to avoid recalculation - Add kernel IIO ABI doc reference for SI unit convention Co-developed-by: Claude Opus 4.6

…dling - Split into set_sample_rate_or_default() and set_sample_rate_max() - Validate requested rate against hardware available list - Promote write failures and read-back errors to warnings Co-developed-by: Claude Opus 4.6

honjow · 2026-04-15T09:55:11Z

Heads up per the AI policy — the last two commits (027034f, 97c5f5d) used Claude to help with the refactoring. Marked with Co-developed-by in the commit messages.

What I did: after getting the review feedback, I described the requested changes to Claude and had it generate the refactored code. Specifically:

For 027034f: I asked it to remove the has_accel()/has_gyro() methods and revert to the static CAPABILITIES array, and to move the scaling factor expressions to const. The prompt was basically "the reviewer says these wrapper methods are redundant, remove them" and "move these to const f64".
For 97c5f5d: I described the reviewer's concern about set_sample_rate — that it doesn't validate the rate and hides errors behind debug logging — and asked it to split the function into a validated path and a max-fallback path with proper warn-level logging.

I reviewed the generated output, checked that it compiles, and verified the logic matches what was asked for in the review. Haven't re-deployed to hardware yet since these are refactors of the same behavior, but can do if needed.

honjow added 3 commits April 4, 2026 21:52

honjow force-pushed the pr/iio-fix-accelgyro branch from 45e858f to 8f453e2 Compare April 4, 2026 13:59

pastaq requested changes Apr 15, 2026

View reviewed changes

honjow added 2 commits April 15, 2026 17:50

honjow force-pushed the pr/iio-fix-accelgyro branch from 70f8961 to 97c5f5d Compare April 15, 2026 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iio): improve AccelGyro3d driver for HID Sensor Hub devices#574

fix(iio): improve AccelGyro3d driver for HID Sensor Hub devices#574
honjow wants to merge 5 commits intoShadowBlip:mainfrom
honjow:pr/iio-fix-accelgyro

honjow commented Apr 3, 2026

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq left a comment

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq Apr 15, 2026 •

edited

Loading

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq Apr 15, 2026

Uh oh!

pastaq Apr 15, 2026

Uh oh!

honjow commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

honjow commented Apr 3, 2026

Summary

Changes

Handle separate accel/gyro IIO devices

Correct scaling factors

Initialize sampling rate on startup

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pastaq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pastaq Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honjow commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pastaq Apr 15, 2026 •

edited

Loading