Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement LED source output device #246

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cd3dbdd
feat(VSCode IDE): add debug button
NeroReflex Dec 28, 2024
57271e6
improv(manager): reorganize code and reduce dependency of UdevDevice:…
NeroReflex Dec 30, 2024
53edb30
feat(VSCode IDE): specify the language as being rust for remote debug…
NeroReflex Dec 30, 2024
005be00
improv(composite_device): better error reporting
NeroReflex Dec 30, 2024
48772bd
fix(udev::Device): bufix for number being u32 and code cleanup
NeroReflex Dec 30, 2024
7135d06
improv(udev::Device): don't use udevadm to inspect devices
NeroReflex Dec 30, 2024
bd6c100
manager: check for devnode() validity for device types that actually …
NeroReflex Dec 30, 2024
3e21daa
feat(config) implement LED configuration
NeroReflex Dec 30, 2024
006df63
feat(LED): LEDs initial implementation
NeroReflex Dec 31, 2024
0fc2c59
feat(LED): implement leds as a source output device
NeroReflex Dec 31, 2024
b6f0803
fix(manager): avoid cycling twice configurations
NeroReflex Dec 31, 2024
27f33ec
improv(CompositeDevice): make the code more readable
NeroReflex Dec 31, 2024
54db7ae
improv(main) do not lose the reference to the ctrl+C signal watcher
NeroReflex Dec 31, 2024
368cf3e
improv(LED): add configuration for the ASUS ROG Ally device
NeroReflex Dec 31, 2024
45d1cdf
improv(LED): increase code readability
NeroReflex Dec 31, 2024
45eb711
improv(code): cargo fmt
NeroReflex Dec 31, 2024
79a2bc6
fix(MultiColorChassis): solve a warning about an unused field
NeroReflex Dec 31, 2024
bbee9b3
improv(code): increase readability by replacing if let() with match
NeroReflex Dec 31, 2024
c65effe
fix(dbus) make dbus path parth of a CompositeDevice at construction
NeroReflex Dec 31, 2024
7d9f0cc
fix(manager): avoid borrowing syntax from typescript
NeroReflex Dec 31, 2024
0b79670
improv(code): cargo fmt
NeroReflex Dec 31, 2024
ac25809
fix(comments): apply suggestions from the PR feedback
NeroReflex Dec 31, 2024
00efbb9
fix(LED): fix inverted b and r components
NeroReflex Dec 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
{
// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"name": "Remote launch",
"type": "lldb",
"request": "launch",
"program": "${workspaceFolder}/target/debug/inputplumber",
"sourceLanguages": ["rust"],
"initCommands": [
"platform select remote-linux",
"platform connect connect://192.168.1.19:1234",
"settings set target.inherit-env false"
],
"env": {
"PATH": "/usr/bin:/usr/local/sbin:/usr/local/bin:/var/lib/flatpak/exports/bin:/usr/lib/rustup/bin"
}
},
{
"type": "lldb",
"request": "launch",
"name": "Debug unit tests in library 'inputplumber'",
"cargo": {
"args": [
"test",
"--no-run",
"--lib",
"--package=inputplumber"
],
"filter": {
"name": "inputplumber",
"kind": "lib"
}
},
"args": [],
"cwd": "${workspaceFolder}"
},
{
"type": "lldb",
"request": "launch",
"name": "Debug executable 'inputplumber'",
"cargo": {
"args": [
"build",
"--bin=inputplumber",
"--package=inputplumber"
],
"filter": {
"name": "inputplumber",
"kind": "bin"
}
},
"args": [],
"cwd": "${workspaceFolder}"
},
{
"type": "lldb",
"request": "launch",
"name": "Debug unit tests in executable 'inputplumber'",
"cargo": {
"args": [
"test",
"--no-run",
"--bin=inputplumber",
"--package=inputplumber"
],
"filter": {
"name": "inputplumber",
"kind": "bin"
}
},
"args": [],
"cwd": "${workspaceFolder}"
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't commit editor-specific configurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't commit editor-specific configurations

This is not editor configuration as in "dev favourite color palette" this is what is needed to remote debug it. The PATH env var is important too as it specify where to find udevadm.

3 changes: 3 additions & 0 deletions rootfs/usr/share/inputplumber/devices/50-rog_ally.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ source_devices:
x: [1, 0, 0]
y: [0, -1, 0]
z: [0, 0, -1]
- group: led
led:
name: ASUS ROG Ally Config
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically we want to use udev matching instead of creating a subsystem-specific config.


# Optional configuration for the composite device
options:
Expand Down
88 changes: 68 additions & 20 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ pub struct SourceDevice {
pub evdev: Option<Evdev>,
pub hidraw: Option<Hidraw>,
pub iio: Option<IIO>,
pub led: Option<Led>,
pub udev: Option<Udev>,
pub unique: Option<bool>,
pub blocked: Option<bool>,
Expand Down Expand Up @@ -362,6 +363,24 @@ pub struct IIO {
pub mount_matrix: Option<MountMatrix>,
}

#[derive(Debug, Deserialize, Clone, PartialEq)]
#[serde(rename_all = "snake_case")]
#[allow(clippy::upper_case_acronyms)]
pub struct Led {
pub id: Option<String>,
pub name: Option<String>,
pub led_fixed_color: Option<LedFixedColor>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this used for? Setting the LED color to a fixed value? If so, I think we should probably include this as part of the profile, instead of the composite device config.

}

#[derive(Debug, Deserialize, Clone, PartialEq)]
#[serde(rename_all = "snake_case")]
#[allow(clippy::upper_case_acronyms)]
pub struct LedFixedColor {
pub r: u8,
pub g: u8,
pub b: u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include an alpha entry for devices that support rgba?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do that. An Option for alpha

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think LEDs support alpha. Just the brightness values of each color.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think some provide a duty cycle to change the overall brightness.

}

#[derive(Debug, Deserialize, Clone, PartialEq)]
#[serde(rename_all = "snake_case")]
#[allow(clippy::upper_case_acronyms)]
Expand Down Expand Up @@ -460,6 +479,15 @@ impl CompositeDeviceConfig {
}
}
}
"leds" => {
for config in self.source_devices.iter() {
if let Some(led_config) = config.led.as_ref() {
if self.has_matching_led(udevice, led_config) {
return Some(config.clone());
}
}
}
}
_ => (),
};
None
Expand Down Expand Up @@ -536,14 +564,15 @@ impl CompositeDeviceConfig {

// If no value was specified in the config, then only match on
// the presence of the property and not the value.
let Some(prop_value) = property.value.as_ref() else {
continue;
};

// Glob match on the property value
log::trace!("Checking property: {prop_value} against {device_prop_value}");
if !glob_match(prop_value.as_str(), device_prop_value.as_str()) {
return false;
match property.value.as_ref() {
Some(prop_value) => {
// Glob match on the property value
log::trace!("Checking property: {prop_value} against {device_prop_value}");
if !glob_match(prop_value.as_str(), device_prop_value.as_str()) {
return false;
}
}
None => continue,
Comment on lines +567 to +575
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally not a fan of this change. It adds additional nesting and isn't related to the feature.

}
}
}
Expand Down Expand Up @@ -578,7 +607,6 @@ impl CompositeDeviceConfig {
/// Returns true if a given hidraw device is within a list of hidraw configs.
pub fn has_matching_hidraw(&self, device: &UdevDevice, hidraw_config: &Hidraw) -> bool {
log::trace!("Checking hidraw config '{:?}'", hidraw_config,);
let hidraw_config = hidraw_config.clone();

// TODO: Switch either evdev of hidraw configs to use the same type. Legacy version had i16
// for hidraw and string for evdev.
Expand Down Expand Up @@ -606,7 +634,7 @@ impl CompositeDeviceConfig {
}
}

if let Some(name) = hidraw_config.name {
if let Some(name) = hidraw_config.name.as_ref() {
let dname = device.name();
log::trace!("Checking name: {name} against {dname}");
if !glob_match(name.as_str(), dname.as_str()) {
Expand All @@ -620,17 +648,39 @@ impl CompositeDeviceConfig {
/// Returns true if a given iio device is within a list of iio configs.
pub fn has_matching_iio(&self, device: &UdevDevice, iio_config: &IIO) -> bool {
log::trace!("Checking iio config: {:?} against {:?}", iio_config, device);
let iio_config = iio_config.clone();

if let Some(id) = iio_config.id {
if let Some(id) = iio_config.id.as_ref() {
let dsyspath = device.syspath();
log::trace!("Checking id: {id} against {dsyspath}");
if !glob_match(id.as_str(), dsyspath.as_str()) {
return false;
}
}

if let Some(name) = iio_config.name {
if let Some(name) = iio_config.name.as_ref() {
let dname = device.name();
log::trace!("Checking name: {name} against {dname}");
if !glob_match(name.as_str(), dname.as_str()) {
return false;
}
}

true
}

/// Returns true if a given led device is within a list of led configs.
pub fn has_matching_led(&self, device: &UdevDevice, led_config: &Led) -> bool {
log::trace!("Checking led config: {:?} against {:?}", led_config, device);

if let Some(id) = led_config.id.as_ref() {
let dsyspath = device.syspath();
log::trace!("Checking id: {id} against {dsyspath}");
if !glob_match(id.as_str(), dsyspath.as_str()) {
return false;
}
}

if let Some(name) = led_config.name.as_ref() {
let dname = device.name();
log::trace!("Checking name: {name} against {dname}");
if !glob_match(name.as_str(), dname.as_str()) {
Expand All @@ -650,41 +700,39 @@ impl CompositeDeviceConfig {
device
);

let evdev_config = evdev_config.clone();

if let Some(name) = evdev_config.name {
if let Some(name) = evdev_config.name.as_ref() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can also be done as

        if let Some(name) = &evdev_config.name {

which is more concise. Keep in mind that this change isn't required at all, since String is clone and not copy, the string is moved to here and ownership taken, then the .as_str() is called below to get a str ref.

Personally I would drop these changes from the commit to reduce noise.

let dname = device.name();
log::trace!("Checking name: {name} against {dname}");
if !glob_match(name.as_str(), dname.as_str()) {
return false;
}
}

if let Some(phys_path) = evdev_config.phys_path {
if let Some(phys_path) = evdev_config.phys_path.as_ref() {
let dphys_path = device.phys();
log::trace!("Checking phys_path: {phys_path} against {dphys_path}");
if !glob_match(phys_path.as_str(), dphys_path.as_str()) {
return false;
}
}

if let Some(handler) = evdev_config.handler {
if let Some(handler) = evdev_config.handler.as_ref() {
let handle = device.sysname();
log::trace!("Checking handler: {handler} against {handle}");
if !glob_match(handler.as_str(), handle.as_str()) {
return false;
}
}

if let Some(vendor_id) = evdev_config.vendor_id {
if let Some(vendor_id) = evdev_config.vendor_id.as_ref() {
let id_vendor = format!("{:04x}", device.id_vendor());
log::trace!("Checking vendor ID: {vendor_id} against {id_vendor}");
if !glob_match(vendor_id.as_str(), id_vendor.as_str()) {
return false;
}
}

if let Some(product_id) = evdev_config.product_id {
if let Some(product_id) = evdev_config.product_id.as_ref() {
let id_product = format!("{:04x}", device.id_product());
log::trace!("Checking product ID: {product_id} against {id_product}");
if !glob_match(product_id.as_str(), id_product.as_str()) {
Expand Down
8 changes: 5 additions & 3 deletions src/dbus/interface/source/evdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ pub struct SourceEventDeviceInterface {

impl SourceEventDeviceInterface {
pub fn new(device: UdevDevice) -> SourceEventDeviceInterface {
let handler = device.devnode();
let capabilities = get_capabilities(handler.as_str()).unwrap_or_else(|e| {
log::warn!("Failed to get capabilities for source evdev device '{handler}': {e:?}");
let capabilities = get_capabilities(device.devnode().as_str()).unwrap_or_else(|e| {
log::warn!(
"Failed to get capabilities for source evdev device '{}': {e:?}",
device.name()
);
HashMap::new()
});
SourceEventDeviceInterface {
Expand Down
4 changes: 2 additions & 2 deletions src/dbus/interface/source/iio_imu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use zbus_macros::interface;

use crate::input::source::iio::get_dbus_path;

/// DBusInterface exposing information about a HIDRaw device
/// DBusInterface exposing information about a iio device
pub struct SourceIioImuInterface {
device: UdevDevice,
}
Expand All @@ -16,7 +16,7 @@ impl SourceIioImuInterface {
SourceIioImuInterface { device }
}

/// Creates a new instance of the source hidraw interface on DBus. Returns
/// Creates a new instance of the source imu interface on DBus. Returns
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice :)

/// a structure with information about the source device.
pub async fn listen_on_dbus(
conn: Connection,
Expand Down
45 changes: 45 additions & 0 deletions src/dbus/interface/source/led.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
use crate::input::source::iio::get_dbus_path;
use crate::udev::device::UdevDevice;
use std::error::Error;
use zbus::{fdo, Connection};
use zbus_macros::interface;

/// DBusInterface exposing information about a led device
pub struct SourceLedInterface {
device: UdevDevice,
}
impl SourceLedInterface {
pub fn new(device: UdevDevice) -> SourceLedInterface {
SourceLedInterface { device }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we should use line breaks between functions definitions

Suggested change
}
}

/// Creates a new instance of the source led interface on DBus. Returns
/// a structure with information about the source device.
pub async fn listen_on_dbus(
conn: Connection,
device: UdevDevice,
) -> Result<(), Box<dyn Error>> {
let iface = SourceLedInterface::new(device);
let Ok(id) = iface.id() else {
return Ok(());
};
let path = get_dbus_path(id);
tokio::task::spawn(async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is done in an async function, is it really required just to start the object server? The zbus crate starts async tasks itself, see https://github.com/dbus2/zbus/blob/71834a41a008214acda27b31dc5948c8ca08a594/zbus/src/connection/mod.rs#L950

Copy link
Contributor Author

@NeroReflex NeroReflex Jan 2, 2025

Choose a reason for hiding this comment

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

This is done in an async function, is it really required just to start the object server? The zbus crate starts async tasks itself, see https://github.com/dbus2/zbus/blob/71834a41a008214acda27b31dc5948c8ca08a594/zbus/src/connection/mod.rs#L950

I copied this code from iio_imu and I mantained like that for consistency. The main problem is that right now listen_on_dbus() the function that contains the spawn is called by manager.rs in a function that must terminate (don't block) an the spawned function has a

conn.object_server().at(path.clone(), iface).await

so either more manager.rs logic is changed or that will have to stay like that, unless the await immediatly returns (I assume it won't, but I can try)...

log::debug!("Starting dbus interface: {path}");
let result = conn.object_server().at(path.clone(), iface).await;
if let Err(e) = result {
log::debug!("Failed to start dbus interface {path}: {e:?}");
} else {
log::debug!("Started dbus interface: {path}");
}
});
Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we should have a line break between functions.

Suggested change
}
}

#[interface(name = "org.shadowblip.Input.Source.LEDDevice")]
impl SourceLedInterface {
/// Returns the human readable name of the device (e.g. XBox 360 Pad)
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be updated to reflect this property.

#[zbus(property)]
fn id(&self) -> fdo::Result<String> {
Ok(self.device.sysname())
}
}
1 change: 1 addition & 0 deletions src/dbus/interface/source/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub mod evdev;
pub mod hidraw;
pub mod iio_imu;
pub mod led;
pub mod udev;
Loading
Loading