I am somewhat new to Rust and specially new to FFI programming, and I am porting a macOS specific C library called librazermacos to Rust.
While writing some public APIs I came up with my first two methods which, incidentally, are the ones I needed to write to check my mouse battery and build some kind of proactive monitoring of when it's about to run out. That was my initial goal when porting and writing this code.
The extend goal is to gradually improve the exposed functionality to eventually get parity with the current C library and maybe even write a desktop app using Tauri that leverages the library.
Here's the relevant methods:
pub fn battery(&self) -> u8 {
let c_str = unsafe {
let devices = getAllRazerDevices();
let slice = slice::from_raw_parts(devices.devices, devices.size as usize);
let device = slice
.iter()
.find(|d| d.internalDeviceId == self.internal_device_id)
.unwrap();
let mut buf = [0i8; 4];
razer_attr_read_get_battery(device.usbDevice, buf.as_mut_ptr());
closeAllRazerDevices(devices);
CStr::from_ptr(buf.as_ptr())
};
let str = format!("{}", c_str.to_str().unwrap()).trim().to_string();
let current: u8 = str.parse().unwrap();
return ((current as f32 / 255.0) * 100.0) as u8;
}
pub fn is_charging(&self) -> bool {
let c_str = unsafe {
let devices = getAllRazerDevices();
let slice = slice::from_raw_parts(devices.devices, devices.size as usize);
let device = slice
.iter()
.find(|d| d.internalDeviceId == self.internal_device_id)
.unwrap();
let mut buf = [0i8; 4];
razer_attr_read_is_charging(device.usbDevice, buf.as_mut_ptr());
closeAllRazerDevices(devices);
CStr::from_ptr(buf.as_ptr())
};
format!("{}", c_str.to_str().unwrap()).starts_with("1")
}
This code works perfectly as is, but it's far from ideal.
My feedback/review goals here are:
Some guidance on how to evaluate and hopefully improve the safety of the unsafe blocks;
Figure out a way to reduce duplications on the two methods above, where I first get all devices, use some pointers to get information, and finally close the devices. From my experience with other languages I was tempted to extract both razer_attr_read_get_battery and razer_attr_read_is_charging into closures that would then be wrapped on a extracted function that opens, calls the closure and closes the devices. I was looking into Fn/FnOnce/etc. but not really sure which direction would be more idiomatic.
I would recommend not opening and closing all of the devices between every operation unless you have a really good reason to. That would help with the boilerplate as well. You could have a Drop impl to close all the devices.
Here's a loose sketch of what that might look like. Disclaimer: It's possible there's some UB in here somewhere, I haven't thought about it very hard.
pub struct RazerDevices(librazermacos_sys::RazerDevices);
impl RazerDevices {
pub fn all() -> Self {
let devices = unsafe { getAllRazerDevices() };
assert!(!devices.devices.is_null());
Self(devices)
}
pub fn find(&mut self, product_id: u16) -> Option<RazerDevice<'_>> {
self.slice_mut()
.iter_mut()
.find(|device| device.productId == product_id)
.map(RazerDevice)
}
fn slice_mut(&mut self) -> &mut [librazermacos_sys::RazerDevice] {
unsafe { slice::from_raw_parts_mut(self.0.devices, self.0.size as usize) }
}
}
impl Drop for RazerDevices {
fn drop(&mut self) {
println!("Closing devices");
unsafe { closeAllRazerDevices(self.0) }
}
}
#[derive(Debug)]
pub struct RazerDevice<'a>(&'a mut librazermacos_sys::RazerDevice);
impl RazerDevice<'_> {
pub fn battery(&self) -> u8 {
let c_str = unsafe {
let mut buf = [0i8; 4];
razer_attr_read_get_battery(self.0.usbDevice, buf.as_mut_ptr());
CStr::from_ptr(buf.as_ptr())
};
let str = format!("{}", c_str.to_str().unwrap()).trim().to_string();
let current: u8 = str.parse().unwrap();
return ((current as f32 / 255.0) * 100.0) as u8;
}
pub fn is_charging(&self) -> bool {
let c_str = unsafe {
let mut buf = [0i8; 4];
razer_attr_read_is_charging(self.0.usbDevice, buf.as_mut_ptr());
CStr::from_ptr(buf.as_ptr())
};
format!("{}", c_str.to_str().unwrap()).starts_with("1")
}
}
#[cfg(test)]
mod tests {
use librazermacos_sys::USB_DEVICE_ID_RAZER_VIPER_ULTIMATE_WIRELESS;
use super::*;
#[test]
fn it_works() {
let mut devices = RazerDevices::all();
let device = devices.find(USB_DEVICE_ID_RAZER_VIPER_ULTIMATE_WIRELESS as u16);
if let Some(device) = device {
println!("Battery: {}", device.battery());
} else {
println!("Device not found");
}
}
}
That would keep your API more in line with how the C API appears to operate too.
This looks great, thank you. I am about to go to bed but super excited to go though this in depth tomorrow. Will reply here in case I have any further questions. Really appreciate your insight here.
I would recommend not opening and closing all of the devices between every operation unless you have a really good reason to. That would help with the boilerplate as well. You could have a Drop impl to close all the devices.
It is a named tuple with a single element. It is essentially equivalent to struct Foo { field: Bar }, except that the field is unnamed and the syntax is different.
I don't know what you mean by "default field". There is nothing like the "null is a valid value of every type" like in C or Go, if that's what you mean, and neither are there any automatic implicit conversions between types. Wrapping a single field in a new struct allows you to implement different semantics for the wrapped type and prevent a misuse of the API. This is called a "newtype pattern".
Yes, took me a bit but I realized that was the case, I got carried away with the “tuple with only one element” thing. Thanks for clarifying that!
The default thing was a total brainfart mixed with inability to express myself clear. If it was a “default” field, how would you even access it, right?
The whole thing became a lot more clear when I saw self.0 being used to access that field.
Thanks also for the link on newtype pattern, that’s really great and even moreso for the enlightenment here. I think I got it now. Momentarily for sure, but that’s the beauty of learning, right?
Hey there @semicoleon and @afetisov, just wanted to reply to this thread one last time to thank you both again for the initial push on reviewing that initial part of my code.
Since then, I've been able to release an initial version of the library:
Along with the initial version of the Tauri desktop app that uses the library:
Here's a screenshot of it running locally on my machine:
I am also capturing all battery charge changes into a SQLite database. I plan on implementing next an estimated time until the battery is discharged.
