Implement LED source output device #246

NeroReflex · 2024-12-31T02:47:49Z

Implement LED source output device contextually a code cleanup:

Remove duplicated code
Avoid spawning useless tasks
Keep references to some spwned tasks and join then in the main() function
Avoid calling a CLI software to fetch devices data and use the (already in use) udev crate
Add VSCode launch.json file to be able to remote debug the software

pastaq

I left a few comments but I'll add some more general notes here. I haven't given everything in the PR a completely thorough look due to some of the reasons posted below.

1.) I'm not a fan of sweeping refactoring that also includes new features as it is difficult to both review and test. I would prefer one PR to add LED RGB and one PR to refactor manager/devnode usage. Order isn't super important but I think it would be easier to refactor then add new features after.

2.) Please explain some of the reasons behind many of the changes. With so many very large changes it is difficult to grasp the context without having discussions beforehand or adding lots of reasoning in the pull request itself.

3.) Please ensure all commit titles are using the semantics format so our CI will pick everything up properly.

pastaq · 2024-12-31T09:31:10Z

src/config/mod.rs

+        true
+    }
+
+    /// Returns true if a given iio device is within a list of iio configs.


Comment incorrect

pastaq · 2024-12-31T09:32:04Z

src/config/mod.rs

+pub struct LedFixedColor {
+    pub r: u8,
+    pub g: u8,
+    pub b: u8,


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

Yes, I can do that. An Option for alpha

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

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

pastaq · 2024-12-31T09:32:55Z

src/dbus/interface/source/led.rs

+use std::error::Error;
+use zbus::{fdo, Connection};
+use zbus_macros::interface;
+/// DBusInterface exposing information about a HIDRaw device


Comment incorrect

well I copied it from iio, so it is wrong in IIO too. lol.

btw, fixed (both).

pastaq · 2024-12-31T09:33:09Z

src/dbus/interface/source/led.rs

+    pub fn new(device: UdevDevice) -> SourceLedInterface {
+        SourceLedInterface { device }
+    }
+    /// Creates a new instance of the source hidraw interface on DBus. Returns


Comment incorrect

same as before: iio is also wrong.

pastaq · 2024-12-31T09:37:29Z

src/drivers/opineo/driver.rs

-        let cs_path = CString::new(path.clone())?;
+        if !Path::new(path.as_str()).exists() {
+            return Err(format!(
+                "Device '{}' does not have a devnode: a valid OrangePi NEO Controller has one",


The phrasing of this is a little strange. "Failed to find device node for {}, aborting initialization." Or something closer to that effect would be better.

I want the message to include the fact it's coming from an opi device... Can you give me a phrase that includes that?

"Failed to find device node for {}, aborting initialization as OrangePi NEO Controller."

pastaq · 2024-12-31T09:43:27Z

src/input/manager.rs

@@ -194,15 +197,7 @@ impl Manager {
        }
    }



Can you explain these changes? This seems like a major departure.

See general answer. Basically reduce reliance of devnode() as leds don't have one, while also removing duplicated code with very minimal specialization.

It is the removal of specialization that actually enables leds functionality, not new added code. In fact I uploaded deactivated LEDs code before so this commit could activate them...

pastaq · 2024-12-31T09:46:46Z

src/input/manager.rs

+            Self::discover_all_devices(&cmd_tx_all_devices),
+            // Watch for IIO device events
+            Self::watch_iio_devices(cmd_tx_iio),
+            // Watch for iio and other devices


Why do we need two iio watchers?

When I refactored this method I kept the logic intact. This question is one I cannot answer.

I re-watched the code: discover_all_devices does the first initialization, while watch_iio_devices is meant to keep living on.

Before the refactor discover_all_devices was spawned in a task and that tash's reference was lost.

In fact before the method that spawned the task was awaited, but not the task itself: so I kept that intact.

discover_all_devices can be awaited now as I think was the original intent, but I want confirmation before doing that.

The code highlight was unfortunate and led you to check something I didn't intend. The comment for line 422 and the comment for line 444 Self::watch_hidraw_devices(cmd_tx_hidraw, &mut watcher_rx), are strange. will watch_hidraw_devices watch for iio? I know that both are in /dev so it's possible?

pastaq · 2024-12-31T09:51:30Z

src/input/manager.rs

+        let tx_for_listen_on_dbus = self.tx.clone();
+        let dbus_for_listen_on_dbus = self.dbus.clone();
+
+        let this: &Self = self;


Why is "this" necessary? I'm not a fan of borrowing syntax from typescript.

Self is mutable. Is used to make the borrow checker happy...

well.... I solved this.

pastaq · 2024-12-31T09:57:49Z

src/input/source/led/multicolor_chassis.rs

Not all RGB interfaces will match this implementation. They should be platform specific. The Ally is able to set 4 quadrants individually and takes a string of four RGB values. Ayn and ayaneo only take a single string but have different modes available.

The way it's implemented is easy to extend to more devices. It scan multi_index and creates a map of component. If a component is recognised (currently only rgb is) when a color arrives it reads the multi_intensity file and changes only relevant and recognised values. There is no way to specialize this when just multi_index contains different count or rgb or rgba.

pastaq · 2024-12-31T10:02:50Z

src/udev/mod.rs

This is an interface that predates our use of udev-rs and is being depreciated over time. I don't know if there is value in updating it vice removing it and using the newer udev methods.

This is an interface that predates our use of udev-rs and is being depreciated over time. I don't know if there is value in updating it vice removing it and using the newer udev methods.

I think there is value in that: if it is kept as small as possible it will reflect the bare minimum needed for the software to run and allows for future ways of discovering devices.

You really want to not use Command::new("udevadm") in these things unless there is no other way. I would suggest full removal

You really want to not use Command::new("udevadm") in these things unless there is no other way. I would suggest full removal

See your answer here: #246 (comment)

Other changes are to cope well with the removal of calling udevadm so you three have to decide if that class has to be:

fixed and I have to find some other way to implement leds (there might be one that will be more complex, will that be fine?): might be major work for me

left bugged (it's rare, who cares lol) and call udevadm and not touch that code at all: remove noise from the PR

pick all changes and futher rework that struct to make it relevant again: maybe needless work as UdevDevice is used way more anyway

drop that udev::Device structure completely and only use UdevDevice: probably better, but still adds noise so I will do in a separate PR

There might be the option "accept the current state", but it received three comments so I guess it's not liked by neither of the two. I will change accordingly to what you choose

NeroReflex · 2024-12-31T16:37:00Z

I left a few comments but I'll add some more general notes here. I haven't given everything in the PR a completely thorough look due to some of the reasons posted below.

1.) I'm not a fan of sweeping refactoring that also includes new features as it is difficult to both review and test. I would prefer one PR to add LED RGB and one PR to refactor manager/devnode usage. Order isn't super important but I think it would be easier to refactor then add new features after.

2.) Please explain some of the reasons behind many of the changes. With so many very large changes it is difficult to grasp the context without having discussions beforehand or adding lots of reasoning in the pull request itself.

3.) Please ensure all commit titles are using the semantics format so our CI will pick everything up properly.

the PR is divided in two stages: early commits are refactors that are needed to avoid losing referemces to spawned threads, then there is the commit that adds leds code and remove useless specialization of source devices creation: as a result of that removal leds are also being taken into consideration. Previous code was very long spanning multiple monitora is length and adding another specialization was both unpractical and basically more tech debt.
well see 1, I can write that to commit messages if you like that more
what? Where do I read about that? Edit: I think I did tha, unsure if that's the proper way

Allow remote debugging the software via the CodeLLDB extension and lldb-server.

…:devnode being populated For a future implementation of leds it is important checks of devnode being populated are pushed where devnode is acually used: do that while also splitting up a gigantic method that both leaked a reference to a spaned task and was very hard to work with.

…ging

Use .to_string() on errors to avoid having non-informative error messages such as "Failed to set composite device for target device: Error { ... }" where those three dots are actually three dots.

Fix a bug with the device number being actually an u64 for udev, but represented as an u32 by us.

Use the native udev library to collect info about a device and avoid invoking a command.

…requires it In order to implement LED(s) handling and potentially other device types that don't have a node in /dev early checks for devnode() being valid must be dropped.

Implement Led configuration support with a fixed color. Cleanup wasteful clone() calls in various lookup methods.

Import initial LEDs code but leave it unused: in order to be able to use it we need to either specialize the source device creation or unify software paths that provided specialization with very little differences.

Perform a code cleanup: - remove a method that was used only once just to avoid calling .clone() - remove a lot of duplicated code in manager - use a specific function to resolve configuration matches instead of using device-specific matches for each device type - solve the SourceDevice scope creep that was in need (for no reason) to know specific internal details about sub-types Removing the duplicated code has the additional effect of activating LED(s) source devices acquisition and activation. This commit adds the ability of defining a fixed RGB color to be applied on device acquisition and never be overwritten.

Function on_source_device_added used to cycle all configurations for active composite devices, if a matching one was not found cycled again to see if one could be created and therefore: - due to the fact that the first config to match was not always the same it could have lead to bugs where one device might have joined two different composite devices upon disconnection and reconnection if another matching one was defined (this is also due to the fact that one source device cannot join two composite devices) - it added a good portion of duplicated code that was just slightly different - it made assumptions that .unwrap() would not fail on certain spots Solve aforementioned problems by using a single cycle to scan for configurations and create a composite device at need.

DBus path cannot be None as part of the code expects it and it will call .unwrap() on the option potentially crashing the software: construct a CompositeDevice with that value in place and ready for every other subsequent usage.

flukejones · 2025-01-02T06:44:35Z

src/input/composite_device/mod.rs

@@ -308,66 +312,66 @@ impl CompositeDevice {
                match cmd {
                    CompositeCommand::ProcessEvent(device_id, event) => {
                        if let Err(e) = self.process_event(device_id, event).await {
-                            log::error!("Failed to process event: {:?}", e);
+                            log::error!("Failed to process event: {}", e.to_string());


All of this is unnecessary and adds noise to the commit series. These types of things should be done as a completely separate "cleanup" series, probably using clippy also.

flukejones · 2025-01-02T06:54:13Z

src/udev/device.rs

+    /// "/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.3/usb1/1-3/1-3:1.2/0003:0B05:1ABE.0003/hidraw/hidraw2/device"
+    fn sysnode_device_link(&self) -> String {
+        let s = self.path.as_str();
+        match s.ends_with("/device") {


Is this actually the correct thing to do? In changes below you change from this:

let path = format!("/sys{}/device/id/vendor", self.path.clone());

to this:

let path = format!("{}/id/vendor", self.sysnode_device_link());

The path now has device appended to it since it didn't end with it. Only get_parent_device_name() expected this.

I would suggest adding some unit tests. Or drop this change completely since the commit apparently addresses only the `u64 change and not this.

In fact please drop all the changes that aren't u64. You've changed the behaviour unnecessarily

Agree, only the bugfix is needed here.

flukejones · 2025-01-02T07:03:10Z

src/udev/mod.rs

+            let metadata = fs::metadata(path).map_err(|e| Box::new(e))?;
+            let devtype = match metadata.st_mode() & nix::libc::S_IFMT {
+                nix::libc::S_IFCHR => Some(udev::DeviceType::Character),
+                nix::libc::S_IFBLK => Some(udev::DeviceType::Block),


It's likely to be preferable to use the rustix crate instead of nix in most cases now.

flukejones · 2025-01-02T07:21:55Z

src/config/mod.rs

-        let evdev_config = evdev_config.clone();
-
-        if let Some(name) = evdev_config.name {
+        if let Some(name) = evdev_config.name.as_ref() {


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.

flukejones · 2025-01-02T07:25:10Z

src/dbus/interface/source/led.rs

+            return Ok(());
+        };
+        let path = get_dbus_path(id);
+        tokio::task::spawn(async move {


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

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)...

ShadowApex · 2024-12-31T16:31:46Z

.vscode/launch.json

+            "cwd": "${workspaceFolder}"
+        }
+    ]
+}


We shouldn't commit editor-specific configurations

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.

ShadowApex · 2024-12-31T17:29:42Z

src/config/mod.rs

+pub struct LedFixedColor {
+    pub r: u8,
+    pub g: u8,
+    pub b: u8,


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

ShadowApex · 2025-01-02T17:44:10Z

src/input/manager.rs

+    async fn watch_hidraw_devices(
+        cmd_tx: mpsc::Sender<ManagerCommand>,
+        watcher_rx: &mut mpsc::Receiver<WatchEvent>,
+    ) -> () {


Suggested change

) -> () {

) {

pastaq said otherwise in another comment....

ShadowApex · 2025-01-02T18:36:06Z

src/input/manager.rs

+            let composite_device = (&config).name.clone();
+            let dev_subsystem = (&device).subsystem();


Suggested change

let composite_device = (&config).name.clone();

let dev_subsystem = (&device).subsystem();

let composite_device = config.name.clone();

let dev_subsystem = device.subsystem();

ShadowApex · 2025-01-02T18:37:08Z

src/input/manager.rs

@@ -1760,6 +1491,9 @@ impl Manager {
        let iio_devices = udev::discover_devices("iio")?;
        let iio_devices = iio_devices.into_iter().map(|dev| dev.into()).collect();
        Manager::discover_devices(cmd_tx, iio_devices).await?;
+        let led_devices = udev::discover_devices("leds")?;
+        let led_devices = led_devices.into_iter().map(|dev| dev.into()).collect();
+        Manager::discover_devices(&cmd_tx, led_devices).await?;


Suggested change

Manager::discover_devices(&cmd_tx, led_devices).await?;

Manager::discover_devices(cmd_tx, led_devices).await?;

A leftover from a previous cleanup. I will solve this in the new PR

ShadowApex · 2025-01-02T19:02:59Z

src/input/composite_device/mod.rs

+                return Err(format!(
+                    "Failed to set composite device for target device: {}",
+                    e.to_string()
+                )


If the error already implements the Display trait, then you can just include the variable in the format string to do the equivalent:

Suggested change

return Err(format!(

"Failed to set composite device for target device: {}",

e.to_string()

)

return Err(format!(

"Failed to set composite device for target device: {e}"

)

ShadowApex · 2025-01-02T19:08:49Z

src/input/composite_device/mod.rs

+        let dev_name = (&device).name();
+        let dev_subsystem = (&device).subsystem();


this expression borrows a value the compiler would automatically borrow

Suggested change

let dev_name = (&device).name();

let dev_subsystem = (&device).subsystem();

let dev_name = device.name();

let dev_subsystem = device.subsystem();

ShadowApex · 2025-01-02T19:10:49Z

src/input/composite_device/mod.rs

+                let config = self
+                    .config
+                    .get_matching_device(&device)
+                    .map_or(None, |cfg| cfg.iio);


Suggested change

let config = self

.config

.get_matching_device(&device)

.map_or(None, |cfg| cfg.iio);

let config = self

.config

.get_matching_device(&device)

.and_then(|cfg| cfg.iio);

ShadowApex · 2025-01-02T19:11:19Z

src/input/composite_device/mod.rs

+                let config = self
+                    .config
+                    .get_matching_device(&device)
+                    .map_or(None, |cfg| cfg.led);


Suggested change

let config = self

.config

.get_matching_device(&device)

.map_or(None, |cfg| cfg.led);

let config = self

.config

.get_matching_device(&device)

.and_then(|cfg| cfg.led);

ShadowApex · 2025-01-02T19:36:32Z

src/input/manager.rs

+                log::trace!(
+                    "Skipping composite device {composite_device}: is a single source device"
+                );
                continue;
            }
            if config.maximum_sources.unwrap_or(0) == 1 {
-                log::trace!("{:?} is a single source device. Skipping.", config.name);
+                log::trace!(
+                    "Skipping composite device {composite_device}: is a single source device"
+                );


With the removal of the second loop, how will new composite devices be created in these circumstances? The expected behavior should be:

If a device matches an existing composite device, and it hasn't reached its maximum number of source devices, it should be added to the existing composite device.

If a device matches an existing composite device, but it HAS reached its maximum, a new composite device should be created.

Lastly, if no existing composite device exists, a new one should be created.

With these changes, I don't see how a new composite device could be ever be created if an existing one reaches its max.

For example, you can follow these steps to see this problem manifested:

Enable management of all supported input devices: busctl set-property org.shadowblip.InputPlumber /org/shadowblip/InputPlumber/Manager org.shadowblip.InputManager ManageAllDevices b 1

Plug in an XBox 360 controller (or other supported external controller). A Composite Device should be created for that controller.

Plug in a second controller of the same type. A second Composite Device should be created for that second controller, but is not with these changes.

pastaq · 2025-01-02T19:58:50Z

src/input/manager.rs

@@ -1294,13 +1294,16 @@ impl Manager {
        let dev_path = device.devpath();
        let sys_name = device.sysname();
        if sys_name.is_empty() {
+            log::warn!(


This is going to be very noisy. Use trace, or at most debug.

pastaq

I would drop these commits:

cd3dbdd
53edb30
bbee9b3

I would cherry pick these commits into a separate cleanup PR and drop them from this one:
005be00
7135d06
27f33ec
54db7ae
45d1cdf
45eb711
c65effe
0b79670

pastaq requested changes Dec 31, 2024

View reviewed changes

NeroReflex force-pushed the cleanup_for_leds branch from b93b9af to ca1968c Compare December 31, 2024 14:22

NeroReflex force-pushed the cleanup_for_leds branch 2 times, most recently from aba3583 to e459060 Compare December 31, 2024 19:58

NeroReflex added 22 commits December 31, 2024 21:22

feat(VSCode IDE): add debug button

cd3dbdd

Allow remote debugging the software via the CodeLLDB extension and lldb-server.

feat(VSCode IDE): specify the language as being rust for remote debug…

53edb30

…ging

improv(composite_device): better error reporting

005be00

Use .to_string() on errors to avoid having non-informative error messages such as "Failed to set composite device for target device: Error { ... }" where those three dots are actually three dots.

fix(udev::Device): bufix for number being u32 and code cleanup

48772bd

Fix a bug with the device number being actually an u64 for udev, but represented as an u32 by us.

improv(udev::Device): don't use udevadm to inspect devices

7135d06

Use the native udev library to collect info about a device and avoid invoking a command.

manager: check for devnode() validity for device types that actually …

bd6c100

…requires it In order to implement LED(s) handling and potentially other device types that don't have a node in /dev early checks for devnode() being valid must be dropped.

feat(config) implement LED configuration

3e21daa

Implement Led configuration support with a fixed color. Cleanup wasteful clone() calls in various lookup methods.

feat(LED): LEDs initial implementation

006df63

Import initial LEDs code but leave it unused: in order to be able to use it we need to either specialize the source device creation or unify software paths that provided specialization with very little differences.

improv(CompositeDevice): make the code more readable

27f33ec

improv(main) do not lose the reference to the ctrl+C signal watcher

54db7ae

improv(LED): add configuration for the ASUS ROG Ally device

368cf3e

improv(LED): increase code readability

45d1cdf

improv(code): cargo fmt

45eb711

fix(MultiColorChassis): solve a warning about an unused field

79a2bc6

improv(code): increase readability by replacing if let() with match

bbee9b3

fix(manager): avoid borrowing syntax from typescript

7d9f0cc

improv(code): cargo fmt

0b79670

fix(comments): apply suggestions from the PR feedback

ac25809

NeroReflex force-pushed the cleanup_for_leds branch from e459060 to ac25809 Compare December 31, 2024 20:23

fix(LED): fix inverted b and r components

00efbb9

flukejones reviewed Jan 2, 2025

View reviewed changes

ShadowApex requested changes Jan 2, 2025

View reviewed changes

pastaq reviewed Jan 2, 2025

View reviewed changes

pastaq requested changes Jan 2, 2025

View reviewed changes

		let composite_device = (&config).name.clone();
		let dev_subsystem = (&device).subsystem();

	Manager::discover_devices(&cmd_tx, led_devices).await?;
	Manager::discover_devices(cmd_tx, led_devices).await?;

		let dev_name = (&device).name();
		let dev_subsystem = (&device).subsystem();

Implement LED source output device #246

Are you sure you want to change the base?

Implement LED source output device #246

Conversation

NeroReflex commented Dec 31, 2024

pastaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeroReflex Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeroReflex Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeroReflex Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeroReflex Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

NeroReflex commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeroReflex Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadowApex Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pastaq left a comment

Choose a reason for hiding this comment

NeroReflex Dec 31, 2024 •

edited

Loading

NeroReflex Dec 31, 2024 •

edited

Loading

NeroReflex Dec 31, 2024 •

edited

Loading

NeroReflex Jan 2, 2025 •

edited

Loading

NeroReflex commented Dec 31, 2024 •

edited

Loading

NeroReflex Jan 2, 2025 •

edited

Loading

ShadowApex Jan 2, 2025 •

edited

Loading