set_output_report (2nd version)

[marek-g]

Adds abstraction for hid_send_output_report(). As the function was added in hidapi v0.15 - the submodule had to be updated.

This PR replaces #159 which doesn't compile.

[ruabmbua]

Seems like there is not actually a release of https://github.com/libusb/hidapi yet with the feature. Also, your PR only makes the new set_output_report work with the hidapi backend.

I recommend you conditionally enable the new function.

[ruabmbua]

Change to:

#[cfg(hidapi)]
pub fn send_output_report(&self, data: &[u8]) -> HidResult<()> {

[marek-g]

Sorry for that, I didn't realize at first that you support also windows-native and linux-native.

Maybe I will wait with adding the #[cfg(hidapi)]. You already provided linux-native implementation and I can try to write windows-native, too. I have such device (Redragon k585 keyboard uses send_output_report to control RGB leds) and I will be able to test it.

But please be patient, because testing it on Windows will take me more time and I'm also very short of it, too :/

[marek-g]

Seems like there is not actually a release of https://github.com/libusb/hidapi yet with the feature. Also, your PR only makes the new set_output_report work with the hidapi backend.

I only don't know what to do with that.

Will you be ok to use unreleased version like in my PR?

Or do you see any other option? Like adding new feature (hidapi_experimental?). But I don't know what's the best way to conditionally use different commit for git submodule for that feature (additional git checkout in build.rs or having two git submodules - hidapi & hidapi_experimental?)....

[ruabmbua]

I am totally ok with updating the module on the git master branch of my repo. Not yet sure if I want to make a crates.io release yet, but I probably will. Anyways, its not a problem for this pull request.

[marek-g]

Thx! So, I will add linux-native and windows-native implementations for completeness.

[ruabmbua]

Nice!

[ruabmbua]

Also, are you maybe willing to test this patch, and see if it works with the linux-native backend? If yes you can add it to your branch. I do not currently have any time to find a USB device that needs output reports and test the patch.

From 2e4b793b4b056f0db10af4f038dd6f0e8f7392f6 Mon Sep 17 00:00:00 2001
From: Roland Ruckerbauer <[email protected]>
Date: Fri, 13 Dec 2024 20:06:31 +0100
Subject: [PATCH] Added send_output_report to linux-native backend

---
 src/linux_native.rs       | 24 +++++++++++++++++++++++-
 src/linux_native/ioctl.rs |  9 +++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/src/linux_native.rs b/src/linux_native.rs
index 464fe4e..0125324 100644
--- a/src/linux_native.rs
+++ b/src/linux_native.rs
@@ -22,7 +22,9 @@ use nix::{
 };
 
 use super::{BusType, DeviceInfo, HidDeviceBackendBase, HidError, HidResult, WcharString};
-use ioctl::{hidraw_ioc_get_feature, hidraw_ioc_grdescsize, hidraw_ioc_set_feature};
+use ioctl::{
+    hidraw_ioc_get_feature, hidraw_ioc_grdescsize, hidraw_ioc_set_feature, hidraw_ioc_set_output,
+};
 
 // Bus values from linux/input.h
 const BUS_USB: u16 = 0x03;
@@ -556,6 +558,26 @@ impl HidDeviceBackendBase for HidDevice {
         Ok(res)
     }
 
+    fn send_output_report(&self, buf: &[u8]) -> HidResult<()> {
+        let res = match unsafe { hidraw_ioc_set_output(self.fd.as_raw_fd(), buf) } {
+            Ok(n) => n,
+            Err(e) => {
+                return Err(HidError::HidApiError {
+                    message: format!("ioctl (SOUTPUT): {e}"),
+                });
+            }
+        };
+
+        if res as usize != buf.len() {
+            return Err(HidError::IncompleteSendError {
+                sent: res as usize,
+                all: buf.len(),
+            });
+        }
+
+        Ok(())
+    }
+
     fn set_blocking_mode(&self, blocking: bool) -> HidResult<()> {
         self.blocking.set(blocking);
         Ok(())
diff --git a/src/linux_native/ioctl.rs b/src/linux_native/ioctl.rs
index 67f7829..e3b740d 100644
--- a/src/linux_native/ioctl.rs
+++ b/src/linux_native/ioctl.rs
@@ -7,6 +7,7 @@ const HIDRAW_IOC_MAGIC: u8 = b'H';
 const HIDRAW_IOC_GRDESCSIZE: u8 = 0x01;
 const HIDRAW_SET_FEATURE: u8 = 0x06;
 const HIDRAW_GET_FEATURE: u8 = 0x07;
+const HIDRAW_SET_OUTPUT: u8 = 0x0b;
 
 ioctl_read!(
     hidraw_ioc_grdescsize,
@@ -21,9 +22,17 @@ ioctl_write_buf!(
     HIDRAW_SET_FEATURE,
     u8
 );
+
 ioctl_read_buf!(
     hidraw_ioc_get_feature,
     HIDRAW_IOC_MAGIC,
     HIDRAW_GET_FEATURE,
     u8
 );
+
+ioctl_write_buf!(
+    hidraw_ioc_set_output,
+    HIDRAW_IOC_MAGIC,
+    HIDRAW_SET_OUTPUT,
+    u8
+);
-- 
2.47.1

[marek-g]

Hi Roland!

I tested your patch and it works for me! I added it to my branch. Having 'linux-native' implementation I don't have any reason to use hidapi. Thanks!

I still want to implement windows-native version. But that can take me longer.

[marek-g]

By the way, Roland - maybe you know. I just bricked another keyboard by sending to it multiple requests to change RGB colors too quickly without required delays. I didn't think it can be dangerous. But now I think the keyboard probably stores it's settings in the flash memory (it was persistent). And it could have firmware bug that erased it or something in unexpected scenario.

Do you think changing RGB keyboard colors frequently (like every time I change keyboard layout with a dedicated key press - that's a few times per hour is dangerous to keyboard's flash (flash wear out) even if all the delays are correct? It's a proprietary firmware, so I cannot disable storing the settings persistently.

Even if the service will fix my keyboard, I am now seriously scared to continue sending requests from mine software :/

[marek-g]

Ok! It was very easy to add windows-native version. I think that should be it and it's ready for re-review.

[ruabmbua]

I am actually quite experienced with embedded software, so this is my take:

Your concerns with delays: The delays itself (as part of the communication via USB) should not affect affect flash wear at all. If you can manage to brick your keyboard easily just by writing a couple times to quickly, it is most likely a software bug at the keyboard side. This can be some kind of race condition, or maybe actually the fact that data is written to fast to the flash controller for some hardware. Anyways, this would still be a bug in the software on the keyboard, a sound design would ensure USB communication can not harm the device.

Flash wear in general: If you are sure the keyboard is immediately writing all config changes to its flash memory, then it would really depend a lot on the used flash technology / specific flash chip. Its most likely (due to economics in relative recent hardware designs), that your keyboard is using some form of NOR flash memory, either embedded into the microcontroller itself, or (more unlikely) as part of a separate SPI / I2C NOR flash chip. In general if its only a few times an hour, and the way writing is done is not stupidly inefficient, it should be fine. NOR flash memory is quite long lasting, it will most likely outlast any USB sticks or SD cards (which are high capacity NAND flash).
If the keyboard software does something unreasonable, like erasing and rewriting a large part of its flash for every config change, this could of course amplify the wear and actually cause problems.

[ruabmbua]

Thanks for working on the PR! I will probably add get_input_report and finish that up soon ^^

[marek-g]

If you can manage to brick your keyboard easily just by writing a couple times to quickly, it is most likely a software bug at the keyboard side.

That's exactly what my understanding is. But this was a cheap ($70) as for mechanical keyboard, so I do not expect it has perfect firmware. It definitely may have bugs.

(...) NOR flash memory, either embedded into the microcontroller itself, or (more unlikely) as part of a separate SPI / I2C NOR flash chip. In general if its only a few times an hour, and the way writing is done is not stupidly inefficient, it should be fine. NOR flash memory is quite long lasting, it will most likely outlast any USB sticks or SD cards (which are high capacity NAND flash). If the keyboard software does something unreasonable, like erasing and rewriting a large part of its flash for every config change, this could of course amplify the wear and actually cause problems.

That's a great knowledge. I'm glad I asked. Thanks! Actually I didn't think about caching option. Using cache makes sense (I hope it does).

That's still possible that the failure is not because of erased flashed. Maybe it just hang itself and removing battery will help (it has a battery as it's wire/wireless combo keyboard). But, here again. If it was designed properly I would expect some kind of watchdog and/or always working reset key combination (interrupts?), so that worries me a little bit.

Thank you Roland for your response, because I know that I should not stop trying to customize my own hardware in the future. I just need to be a little more careful in a case the firmware is buggy.

Thanks for working on the PR! I will probably add get_input_report and finish that up soon ^^

Thank you, too! It was a pleasure to contribute :)