Cleanup to C Plugins (#66)

* Cleanup cplugin interface to fully use DeviceInfo_FFI and eliminate _ prefix on some funcs

- Remove dangling pointer to callback on cplugin unload
- Remove need to allocate DeviceInfo in rust from a c plugin. We just let them use the FFI type and we copy the data over
- Add some const qualifiers to things that should never be mutated

* Further C plugin cleanup (#67)

* Simplify CMakeLists for test_c_plugin
It no longer needs linking against Rust code

* Remove new_device_info & drop_device_info

* Bump CPLUGIN_ABI_VERSION since everything's definitely incompatible now

* Fix test (hopefully) (#68)

* Fix "bindings changed"

* Also run workflow on PR

* Re-enable test plugin building

* Fix ffi test that uses test-plugin

* Fix test plugin tests on sdk.rs

---------

Co-authored-by: Sainan <sainan@calamity.gg>

.github/workflows/test.yml

@@ -1,6 +1,6 @@
 name: Test CI
 
-on: push
+on: [push, pull_request]
 
 env:
   CARGO_TERM_COLOR: always

includes/plugin.h

@@ -1,7 +1,7 @@
+#include "wooting-analog-plugin-dev.h"
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
-#include <stdbool.h>
-#include "wooting-analog-plugin-dev.h"
 
 #if defined(_WIN32) || defined(WIN32)
 #ifdef ANALOGSDK_EXPORTS
@@ -15,38 +15,48 @@
 #define ANALOGSDK_API
 #endif
 
-const uint32_t ANALOG_SDK_PLUGIN_ABI_VERSION = 0;
+const uint32_t ANALOG_SDK_PLUGIN_ABI_VERSION = 1;
 
-
-typedef void(*device_event)(void*, WootingAnalog_DeviceEventType, WootingAnalog_DeviceInfo*);
+typedef void (*device_event)(void const *, WootingAnalog_DeviceEventType,
+                             const WootingAnalog_DeviceInfo_FFI *);
 
 /// Get a name describing the `Plugin`.
-ANALOGSDK_API const char* _name();
+ANALOGSDK_API const char *name();
 
 /// A callback fired immediately after the plugin is loaded. Usually used
 /// for initialization.
-ANALOGSDK_API int _initialise(void* callback_data, device_event callback);
+ANALOGSDK_API int initialise(void const *callback_data, device_event callback);
 
 /// A function fired to check if the plugin is currently initialised
 ANALOGSDK_API bool is_initialised();
 
 /// A callback fired immediately before the plugin is unloaded. Use this if
-/// you need to do any cleanup.
+/// you need to do any cleanup. Any references to `callback_data` or `callback`
+/// should be dropped as these will be invalidated after this function returns.
 ANALOGSDK_API void unload();
 
-/// Function called to get the full analog read buffer for a particular device with ID `device`. `len` is the maximum amount
-/// of keys that can be accepted, any more beyond this will be ignored by the SDK.
-/// If `device` is 0 then no specific device is specified and the data should be read from all devices and combined
-ANALOGSDK_API int _read_full_buffer(uint16_t code_buffer[], float analog_buffer[], int len, WootingAnalog_DeviceID device);
-
-/// This function is fired by the SDK to collect up all Device Info structs. The memory for the struct should be retained and only dropped
-/// when the device is disconnected or the plugin is unloaded. This ensures that the Device Info is not garbled when it's being accessed by the client.
+/// Function called to get the full analog read buffer for a particular device
+/// with ID `device`. `len` is the maximum amount of keys that can be accepted,
+/// any more beyond this will be ignored by the SDK. If `device` is 0 then no
+/// specific device is specified and the data should be read from all devices
+/// and combined
+ANALOGSDK_API int read_full_buffer(uint16_t code_buffer[],
+                                    float analog_buffer[], int len,
+                                    WootingAnalog_DeviceID device);
+
+/// This function is fired by the SDK to collect up all Device Info structs. The
+/// memory for the struct should be retained and only dropped when the device is
+/// disconnected or the plugin is unloaded. This ensures that the Device Info is
+/// not garbled when it's being accessed by the client.
 ///
 /// # Notes
 ///
-/// Although, the client should be copying any data they want to use for a prolonged time as there is no lifetime guarantee on the data.
-ANALOGSDK_API int _device_info(WootingAnalog_DeviceInfo* buffer[], int len);
-
-/// Function called to get the analog value for a particular HID key `code` from the device with ID `device`.
-/// If `device` is 0 then no specific device is specified and the value should be read from all devices and combined
+/// Although, the client should be copying any data they want to use for a
+/// prolonged time as there is no lifetime guarantee on the data.
+ANALOGSDK_API int device_info(const WootingAnalog_DeviceInfo_FFI *buffer[],
+                               int len);
+
+/// Function called to get the analog value for a particular HID key `code` from
+/// the device with ID `device`. If `device` is 0 then no specific device is
+/// specified and the value should be read from all devices and combined
 ANALOGSDK_API float read_analog(uint16_t code, WootingAnalog_DeviceID device);

includes/wooting-analog-common.h

@@ -100,13 +100,6 @@ typedef enum WootingAnalogResult {
   WootingAnalogResult_DLLNotFound = -1990,
 } WootingAnalogResult;
 
-/**
- * The core `DeviceInfo` struct which contains all the interesting information
- * for a particular device. This is for use internally and should be ignored if you're
- * trying to use it when trying to interact with the SDK using the wrapper
- */
-typedef struct WootingAnalog_DeviceInfo WootingAnalog_DeviceInfo;
-
 typedef uint64_t WootingAnalog_DeviceID;
 
 /**
@@ -141,29 +134,3 @@ typedef struct WootingAnalog_DeviceInfo_FFI {
    */
   enum WootingAnalog_DeviceType device_type;
 } WootingAnalog_DeviceInfo_FFI;
-
-#ifdef __cplusplus
-extern "C" {
-#endif // __cplusplus
-
-/**
- * Create a new device info struct. This is only for use in Plugins that are written in C
- * Rust plugins should use the native constructor
- * The memory for the struct has been allocated in Rust. So `drop_device_info` must be called
- * for the memory to be properly released
- */
-struct WootingAnalog_DeviceInfo *new_device_info(uint16_t vendor_id,
-                                                 uint16_t product_id,
-                                                 char *manufacturer_name,
-                                                 char *device_name,
-                                                 WootingAnalog_DeviceID device_id,
-                                                 enum WootingAnalog_DeviceType device_type);
-
-/**
- * Drops the given `DeviceInfo`
- */
-void drop_device_info(struct WootingAnalog_DeviceInfo *device);
-
-#ifdef __cplusplus
-}  // extern "C"
-#endif  // __cplusplus

wooting-analog-common/src/lib.rs

@@ -145,39 +145,6 @@ impl DeviceInfo {
     }
 }
 
-/// Create a new device info struct. This is only for use in Plugins that are written in C
-/// Rust plugins should use the native constructor
-/// The memory for the struct has been allocated in Rust. So `drop_device_info` must be called
-/// for the memory to be properly released
-#[no_mangle]
-pub extern "C" fn new_device_info(
-    vendor_id: u16,
-    product_id: u16,
-    manufacturer_name: *mut c_char,
-    device_name: *mut c_char,
-    device_id: DeviceID,
-    device_type: DeviceType,
-) -> *mut DeviceInfo {
-    Box::into_raw(Box::new(DeviceInfo::new_with_id(
-        vendor_id,
-        product_id,
-        unsafe {
-            CStr::from_ptr(manufacturer_name)
-                .to_string_lossy()
-                .into_owned()
-        },
-        unsafe { CStr::from_ptr(device_name).to_string_lossy().into_owned() },
-        device_id,
-        device_type,
-    )))
-}
-
-/// Drops the given `DeviceInfo`
-#[no_mangle]
-pub unsafe extern "C" fn drop_device_info(device: *mut DeviceInfo) {
-    drop(Box::from_raw(device));
-}
-
 #[cfg_attr(feature = "serdes", derive(Serialize, Deserialize))]
 #[derive(Debug, PartialEq, Clone, Primitive)]
 #[repr(C)]

wooting-analog-sdk/build.rs

@@ -1,13 +1,13 @@
 const TEST_PLUGIN_DIR: &str = "test_c_plugin";
 
 fn main() {
-    // println!("Building Test C Plugin");
-    // println!("{:?}", std::env::var("OUT_DIR"));
+    println!("Building Test C Plugin");
+    println!("{:?}", std::env::var("OUT_DIR"));
 
-    // cmake::Config::new(TEST_PLUGIN_DIR)
-    //     .no_build_target(true)
-    //     .out_dir(format!("./{}", TEST_PLUGIN_DIR))
-    //     .always_configure(false)
-    //     .profile("Debug")
-    //     .build();
+    cmake::Config::new(TEST_PLUGIN_DIR)
+        .no_build_target(true)
+        .out_dir(format!("./{}", TEST_PLUGIN_DIR))
+        .always_configure(false)
+        .profile("Debug")
+        .build();
 }

wooting-analog-sdk/src/cplugin.rs

@@ -2,7 +2,6 @@ use ffi_support::FfiStr;
 use libloading::{Library, Symbol};
 use log::*;
 use std::collections::HashMap;
-use std::ffi::CStr;
 use std::os::raw::{c_float, c_int, c_uint, c_ushort, c_void};
 use wooting_analog_common::*;
 use wooting_analog_plugin_dev::*;
@@ -68,10 +67,11 @@ macro_rules! lib_wrap_option {
     };
 }
 
-const CPLUGIN_ABI_VERSION: u32 = 0;
+const CPLUGIN_ABI_VERSION: u32 = 1;
 
 pub struct CPlugin {
     lib: Library,
+    cb_data_ptr: Option<*mut Box<dyn Fn(DeviceEventType, &DeviceInfo) + Send>>,
     //funcs: HashMap<&'static str, Option<Symbol>>
 }
 
@@ -93,59 +93,72 @@ impl CPlugin {
 
         Ok(CPlugin {
             lib,
-            //funcs: HashMap::new()
+            cb_data_ptr: None, //funcs: HashMap::new()
         })
         .into()
     }
 
     lib_wrap_option! {
         //c_name has to be over here due to it not being part of the Plugin trait
-        fn _initialise(data: *mut c_void, callback: extern "C" fn(*mut c_void, DeviceEventType, *mut DeviceInfo)) -> i32;
-        fn _name() -> FfiStr<'static>;
+        fn initialise(data: *const c_void, callback: extern "C" fn(*mut c_void, DeviceEventType, *const DeviceInfo_FFI)) -> i32;
+        fn name() -> FfiStr<'static>;
 
-        fn _read_full_buffer(code_buffer: *const c_ushort, analog_buffer: *const c_float, len: c_uint, device: DeviceID) -> c_int;
-        fn _device_info(buffer: *mut *mut DeviceInfo_FFI, len: c_uint) -> c_int;
+        fn read_analog(code: u16, device: DeviceID) -> f32;
+        fn read_full_buffer(code_buffer: *const c_ushort, analog_buffer: *const c_float, len: c_uint, device: DeviceID) -> c_int;
+        fn device_info(buffer: *mut *const DeviceInfo_FFI, len: c_uint) -> c_int;
+    }
+
+    lib_wrap! {
+        fn is_initialised() -> bool;
+        fn unload();
     }
 }
 
-extern "C" fn call_closure(data: *mut c_void, event: DeviceEventType, device_raw: *mut DeviceInfo) {
+extern "C" fn call_closure(
+    data: *mut c_void,
+    event: DeviceEventType,
+    device_raw: *const DeviceInfo_FFI,
+) {
     debug!("Got into the callclosure");
     unsafe {
         if data.is_null() {
             error!("We got a null data pointer in call_closure!");
             return;
         }
 
-        let device = Box::from_raw(device_raw);
+        let device_info = device_raw.as_ref().unwrap().into_device_info();
 
         let callback_ptr =
             Box::from_raw(data as *mut Box<dyn Fn(DeviceEventType, &DeviceInfo) + Send>);
 
-        (*callback_ptr)(event, &device);
+        (*callback_ptr)(event, &device_info);
+
         //Throw it back into raw to prevent it being dropped so the callback can be called multiple times
         Box::into_raw(callback_ptr);
-        //We also want to convert this back to a pointer as we want the C Plugin to be in control and aware of
-        //when this memory is being dropped
-        Box::into_raw(device);
     }
 }
 
 impl Plugin for CPlugin {
     fn name(&mut self) -> SDKResult<&'static str> {
-        self._name().0.map(|s| s.as_str()).into()
+        self.name().0.map(|s| s.as_str()).into()
     }
 
     fn initialise(
         &mut self,
         callback: Box<dyn Fn(DeviceEventType, &DeviceInfo) + Send>,
     ) -> SDKResult<u32> {
         let data = Box::into_raw(Box::new(callback));
-        self._initialise(data as *mut _, call_closure)
+        self.cb_data_ptr = Some(data);
+        self.initialise(data as *const _, call_closure)
             .0
             .map(|res| res as u32)
             .into()
     }
 
+    fn read_analog(&mut self, code: u16, device: DeviceID) -> SDKResult<f32> {
+        self.read_analog(code, device).0.into()
+    }
+
     fn read_full_buffer(
         &mut self,
         max_length: usize,
@@ -157,7 +170,7 @@ impl Plugin for CPlugin {
         analog_buffer.resize(max_length, 0.0);
         let count: usize = {
             let ret = self
-                ._read_full_buffer(
+                .read_full_buffer(
                     code_buffer.as_ptr(),
                     analog_buffer.as_ptr(),
                     max_length as c_uint,
@@ -182,40 +195,38 @@ impl Plugin for CPlugin {
     }
 
     fn device_info(&mut self) -> SDKResult<Vec<DeviceInfo>> {
-        let mut device_infos: Vec<*mut DeviceInfo_FFI> = vec![std::ptr::null_mut(); 10];
+        let mut device_infos: Vec<*const DeviceInfo_FFI> = vec![std::ptr::null_mut(); 10];
 
         match self
-            ._device_info(device_infos.as_mut_ptr(), device_infos.len() as c_uint)
+            .device_info(device_infos.as_mut_ptr(), device_infos.len() as c_uint)
             .0
             .map(|no| no as u32)
         {
             Ok(num) => unsafe {
                 device_infos.truncate(num as usize);
                 let devices = device_infos
                     .drain(..)
-                    .map(|dev| {
-                        DeviceInfo {
-                            vendor_id: (*dev).vendor_id,
-                            product_id: (*dev).product_id,
-                            manufacturer_name: CStr::from_ptr((*dev).manufacturer_name).to_str().unwrap().to_owned(),
-                            device_name: CStr::from_ptr((*dev).device_name).to_str().unwrap().to_owned(),
-                            device_id: (*dev).device_id,
-                            device_type: (*dev).device_type.clone(),
-                        }
-                    })
+                    .map(|dev| dev.as_ref().unwrap().into_device_info())
                     .collect();
                 Ok(devices).into()
             },
             Err(e) => Err(e).into(),
         }
     }
 
-    lib_wrap! {
-        fn is_initialised() -> bool;
-        fn unload();
+    fn is_initialised(&mut self) -> bool {
+        self.is_initialised()
     }
-    lib_wrap_option! {
-        fn read_analog(code: u16, device: DeviceID) -> f32;
-        //fn neg(x: u32, y: u32) -> u32;
+
+    fn unload(&mut self) {
+        self.unload();
+        // Drop cb_data_ptr
+        if let Some(ptr) = self.cb_data_ptr {
+            unsafe {
+                drop(Box::from_raw(
+                    ptr as *mut Box<dyn Fn(DeviceEventType, &DeviceInfo) + Send>,
+                ));
+            }
+        }
     }
 }