feat(Hardware Support): Qualcomm SSC sensors through FastRPC#590
feat(Hardware Support): Qualcomm SSC sensors through FastRPC#590gio3k wants to merge 11 commits into
Conversation
Adds a default value for MountMatrix (just an identity matrix) Signed-off-by: Gianni Spadoni <me@gio.blue>
libloading will help us load dylibs, which we need for the libssc / FastRPC implementation Signed-off-by: Gianni Spadoni <me@gio.blue>
This implements support for the Qualcomm Sensor Core (SSC), which provides accel and gyro to some Snapdragon based handhelds made in the past 8 years or so This requires libssc as a runtime dependency, and requires libloading as a build time & runtime dependency As FastRPC devices don't actually show up in a "fastrpc" subsystem, and instead use the "misc" subsystem, some logic has been added to udev/device.rs to check for fastrpc devices and fake the subsystem Signed-off-by: Gianni Spadoni <me@gio.blue>
Based on IIOIMUDevice Signed-off-by: Gianni Spadoni <me@gio.blue>
Signed-off-by: Gianni Spadoni <me@gio.blue>
Makes sure we don't get stuck trying to init the sensor if the subsystem isn't ready Signed-off-by: Gianni Spadoni <me@gio.blue>
Also ups the gyro scaling from 14.0 -> 16.0 Signed-off-by: Gianni Spadoni <me@gio.blue>
pastaq
left a comment
There was a problem hiding this comment.
Looking good so far. Understand this is a WIP so it isn't in a finished state. I'm asking for a bit of cleanup with the code comments, and one significant change to device discovery. I'm also not convinced we need a fastrpc specific config option.
| pub evdev: Option<Evdev>, | ||
| /// Devices that match the given fastrpc properties will be captured by InputPlumber | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub fastrpc: Option<FastRpc>, |
There was a problem hiding this comment.
Does this new config interface add additional value? I would think that the udev config would be able to find these devices without the need for another protocol specific entry. evdev/hidraw/etc. are all legacy config options we'd ideally like to get away from.
| @@ -0,0 +1,277 @@ | |||
| pub mod glib { | |||
There was a problem hiding this comment.
This file contains a lot of C binding abstractions, but there is very little context to the purpose of each function or variable. Please go through it and add amplifying information such that we don't need to find & read the ssc documentation to have an understanding of the purpose of each. It doesn't need to be an exhaustive reproduction of the documentation, just enough to explain as an overview.
There was a problem hiding this comment.
I reworked it and added comments around the externals. Let me know what you think
| Ok(events) | ||
| } | ||
|
|
||
| /// Rotate the given axis data according to the mount matrix. This is used |
There was a problem hiding this comment.
Any time we start duplicating code, it is a good idea to consider if a higher level utility file is beneficial. This could indicate we need a public IMU Utils file that each driver can import.
There was a problem hiding this comment.
I agree, but iio_imu uses a different mount_matrix type than ssc does (the iio_imu one is deprecated)
InputPlumber/src/config/mod.rs
Lines 398 to 403 in 85f5fd5
iio_imu isn't currently reading the new config.imu.mount_matrix key either, so I was thinking of fixing that in a subsequent PR
If you'd like I could do that in this PR, I'm just worried about it getting too big
| }; | ||
|
|
||
| let subsystem = match device.subsystem().map(|s| s.to_string_lossy().to_string()) { | ||
| // FastRPC has special behaviour, check the comment in "From<::udev::Device> for UdevDevice" |
There was a problem hiding this comment.
Please don't refer to other comments/files. Each section should be understandable within its own context.
There was a problem hiding this comment.
Yeah sorry this is bad I was planning to put this comment in the PR description but I forgot
There was a problem hiding this comment.
Will fix this when I create the misc subsystem
| .unwrap_or(OsStr::new("")) | ||
| .to_string_lossy() | ||
| .to_string(); | ||
|
|
There was a problem hiding this comment.
I think it would be better to fully support the "misc" subsystem, then determine a device is a fastrpc when searching for a driver. That will be more scalable in the future if we add additional "misc" devices as we won't need to add additional cases here or fake the subsystem.
There was a problem hiding this comment.
In the same vein as the comment below, this file should be for "misc" subsystem, then it should identify a device as a fastrpc IMU here and load the driver. This will allow us to add more "misc" devices in the future if we need to without refactoring udev/manager again.
Signed-off-by: Gianni Spadoni <me@gio.blue>
Should this be docs(ssc) instead of refactor(ssc)? Signed-off-by: Gianni Spadoni <me@gio.blue>
This is a rework of the whole driver side of this PR Bunch more documentation / commenting, thread safety, code cleanup etc Rewrote bindings.rs, made it a single struct for all dylibs & added documentation for all the bindings Reworked SscRuntime to be wrapped in an Arc Stopped passing pointers around all over the place Signed-off-by: Gianni Spadoni <me@gio.blue>
Signed-off-by: Gianni Spadoni <me@gio.blue>
This PR adds support for the FastRPC "subsystem" and the Qualcomm Sensor Core
When combined with the correct sensor data and hexagonrpc, this can provide accelerometer & gyroscope support to Snapdragon handhelds that use the SSC for that
Big changes, in no order:
I'm opening this as a draft as I have a couple concerns:
Also, minor naming concern:
Maybe the ssc driver should be split into two, something like:
drivers/ssc/runtime.rs, drivers/ssc/bindings.rsdrivers/ssc_imu/driver.rs, drivers/ssc_imu/event.rsThis is because SSC can provide other sensor data in the future aside from gyro / accel.
Where would I put the
drivers/sscfolder in that case - it seems out of place to have it there if it's just the core SSC stuff?