FFI client refactoring (#138)

* simplify how the FfiChannel works * Use workspace-level lints and other inherited attributes * missing docs * fix lint
stepfunc · May 15, 2024 · 7453f72 · 7453f72
1 parent 432d048
commit 7453f72
Show file tree

Hide file tree

Showing 17 changed files with 217 additions and 170 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -18,6 +18,24 @@ tokio = "1.37.0"
 tracing = "0.1.40"
 tracing-subscriber = {  version = "0.3.18" }
 
+[workspace.package]
+authors = ["Step Function I/O LLC <[email protected]>>"]
+rust-version = "1.75"
+edition = "2021"
+license-file = "LICENSE.txt"
+homepage = "https://stepfunc.io/products/libraries/modbus/"
+repository = "https://github.com/stepfunc/rodbus"
+keywords = ["dnp3", "ics", "scada", "security"]
+categories = ["network-programming"]
+
+[workspace.lints.rust]
+unsafe_code = "forbid"
+non_ascii_idents = "deny"
+unreachable_pub = "deny"
+trivial_casts = "deny"
+missing_docs = "deny"
+unused = "deny"
+missing_copy_implementations = "deny"
 
 [profile.release]
 lto=true
diff --git a/ffi/rodbus-bindings/Cargo.toml b/ffi/rodbus-bindings/Cargo.toml
@@ -1,14 +1,22 @@
 [package]
 name = "rodbus-bindings"
 version = "1.4.0-M1"
-authors = ["Step Function I/O LLC <[email protected]>"]
-edition = "2021"
-description = "oobindgen schema for Rodbus"
-keywords = ["ffi", "c", "modbus", "industrial", "plc"]
-categories = ["network-programming"]
-repository = "https://github.com/stepfunc/rodbus"
+description = "application to generate bindings for Rodbus"
 readme = "../README.md"
 
+# inherit from workspace
+authors.workspace = true
+rust-version.workspace = true
+edition.workspace = true
+license-file.workspace = true
+homepage.workspace = true
+repository.workspace = true
+keywords.workspace = true
+categories.workspace = true
+
+[lints]
+workspace = true
+
 [dependencies]
 oo-bindgen = { workspace = true }
 rodbus-schema = { path = "../rodbus-schema" }

diff --git a/ffi/rodbus-bindings/src/main.rs b/ffi/rodbus-bindings/src/main.rs
@@ -1,7 +1,9 @@
+//! Entry point to generate FFI bindings for Rodbus
+
 use std::path::PathBuf;
 use std::rc::Rc;
 
-pub fn main() {
+fn main() {
     tracing_subscriber::fmt()
         .with_max_level(tracing::Level::INFO)
         .with_target(false)

diff --git a/ffi/rodbus-ffi/src/client.rs b/ffi/rodbus-ffi/src/client.rs
@@ -1,11 +1,13 @@
 use crate::ffi;
 use crate::ffi::ParamError;
-use rodbus::client::{ClientState, FfiSessionError, HostAddr, Listener, WriteMultiple};
-use rodbus::{AddressRange, MaybeAsync};
+use rodbus::client::{
+    ClientState, FfiChannel, FfiChannelError, HostAddr, Listener, RequestParam, WriteMultiple,
+};
+use rodbus::{AddressRange, MaybeAsync, UnitId};
 use std::net::IpAddr;
 
 pub struct ClientChannel {
-    pub(crate) inner: rodbus::client::Channel,
+    pub(crate) inner: FfiChannel,
     pub(crate) runtime: crate::RuntimeHandle,
 }
 
@@ -45,7 +47,7 @@ pub(crate) unsafe fn client_channel_create_tcp(
     );
 
     Ok(Box::into_raw(Box::new(ClientChannel {
-        inner: channel,
+        inner: FfiChannel::new(channel),
         runtime: runtime.handle(),
     })))
 }
@@ -88,7 +90,7 @@ pub(crate) unsafe fn client_channel_create_rtu(
     );
 
     Ok(Box::into_raw(Box::new(ClientChannel {
-        inner: channel,
+        inner: FfiChannel::new(channel),
         runtime: runtime.handle(),
     })))
 }
@@ -137,7 +139,7 @@ pub(crate) unsafe fn client_channel_create_tls(
     );
 
     Ok(Box::into_raw(Box::new(ClientChannel {
-        inner: channel,
+        inner: FfiChannel::new(channel),
         runtime: runtime.handle(),
     })))
 }
@@ -154,11 +156,12 @@ pub(crate) unsafe fn client_channel_read_coils(
     range: crate::ffi::AddressRange,
     callback: crate::ffi::BitReadCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let range = AddressRange::try_from(range.start, range.count)?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.read_coils(range, |res| callback.complete(res))?;
+    channel
+        .inner
+        .read_coils(param.into(), range, |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -168,11 +171,12 @@ pub(crate) unsafe fn client_channel_read_discrete_inputs(
     range: crate::ffi::AddressRange,
     callback: crate::ffi::BitReadCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let range = AddressRange::try_from(range.start, range.count)?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.read_discrete_inputs(range, |res| callback.complete(res))?;
+    channel
+        .inner
+        .read_discrete_inputs(param.into(), range, |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -182,11 +186,12 @@ pub(crate) unsafe fn client_channel_read_holding_registers(
     range: crate::ffi::AddressRange,
     callback: crate::ffi::RegisterReadCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let range = AddressRange::try_from(range.start, range.count)?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.read_holding_registers(range, |res| callback.complete(res))?;
+    channel
+        .inner
+        .read_holding_registers(param.into(), range, |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -196,11 +201,12 @@ pub(crate) unsafe fn client_channel_read_input_registers(
     range: crate::ffi::AddressRange,
     callback: crate::ffi::RegisterReadCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let range = AddressRange::try_from(range.start, range.count)?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.read_input_registers(range, |res| callback.complete(res))?;
+    channel
+        .inner
+        .read_input_registers(param.into(), range, |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -210,10 +216,11 @@ pub(crate) unsafe fn client_channel_write_single_coil(
     bit: crate::ffi::BitValue,
     callback: crate::ffi::WriteCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.write_single_coil(bit.into(), |res| callback.complete(res))?;
+    channel
+        .inner
+        .write_single_coil(param.into(), bit.into(), |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -223,12 +230,11 @@ pub(crate) unsafe fn client_channel_write_single_register(
     register: crate::ffi::RegisterValue,
     callback: crate::ffi::WriteCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let callback = sfio_promise::wrap(callback);
-
-    let mut session = param.build_session(channel);
-    session.write_single_register(register.into(), |res| callback.complete(res))?;
-
+    channel
+        .inner
+        .write_single_register(param.into(), register.into(), |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -239,12 +245,13 @@ pub(crate) unsafe fn client_channel_write_multiple_coils(
     items: *mut crate::BitList,
     callback: crate::ffi::WriteCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let items = items.as_ref().ok_or(ffi::ParamError::NullParameter)?;
     let args = WriteMultiple::from(start, items.inner.clone())?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.write_multiple_coils(args, |res| callback.complete(res))?;
+    channel
+        .inner
+        .write_multiple_coils(param.into(), args, |res| callback.complete(res))?;
     Ok(())
 }
 
@@ -255,28 +262,29 @@ pub(crate) unsafe fn client_channel_write_multiple_registers(
     items: *mut crate::RegisterList,
     callback: crate::ffi::WriteCallback,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
     let items = items.as_ref().ok_or(ffi::ParamError::NullParameter)?;
     let args = WriteMultiple::from(start, items.inner.clone())?;
     let callback = sfio_promise::wrap(callback);
-    let mut session = param.build_session(channel);
-    session.write_multiple_registers(args, |res| callback.complete(res))?;
+    channel
+        .inner
+        .write_multiple_registers(param.into(), args, |res| callback.complete(res))?;
     Ok(())
 }
 
 pub(crate) unsafe fn client_channel_enable(
     channel: *mut crate::ClientChannel,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
-    channel.runtime.block_on(channel.inner.enable())??;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
+    channel.inner.enable()?;
     Ok(())
 }
 
 pub(crate) unsafe fn client_channel_disable(
     channel: *mut crate::ClientChannel,
 ) -> Result<(), ffi::ParamError> {
-    let channel = channel.as_ref().ok_or(ffi::ParamError::NullParameter)?;
-    channel.runtime.block_on(channel.inner.disable())??;
+    let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
+    channel.inner.disable()?;
     Ok(())
 }
 
@@ -285,9 +293,7 @@ pub(crate) unsafe fn client_channel_set_decode_level(
     level: ffi::DecodeLevel,
 ) -> Result<(), ffi::ParamError> {
     let channel = channel.as_mut().ok_or(ffi::ParamError::NullParameter)?;
-    channel
-        .runtime
-        .spawn(channel.inner.set_decode_level(level.into()))?;
+    channel.inner.set_decode_level(level.into())?;
     Ok(())
 }
 
@@ -406,12 +412,21 @@ impl TryFrom<ffi::TlsClientConfig> for rodbus::client::TlsClientConfig {
     }
 }
 
-impl From<FfiSessionError> for ParamError {
-    fn from(err: FfiSessionError) -> Self {
+impl From<FfiChannelError> for ParamError {
+    fn from(err: FfiChannelError) -> Self {
         match err {
-            FfiSessionError::ChannelFull => ParamError::TooManyRequests,
-            FfiSessionError::ChannelClosed => ParamError::Shutdown,
-            FfiSessionError::BadRange(err) => err.into(),
+            FfiChannelError::ChannelFull => ParamError::TooManyRequests,
+            FfiChannelError::ChannelClosed => ParamError::Shutdown,
+            FfiChannelError::BadRange(err) => err.into(),
+        }
+    }
+}
+
+impl From<ffi::RequestParam> for RequestParam {
+    fn from(value: ffi::RequestParam) -> Self {
+        Self {
+            id: UnitId::new(value.unit_id),
+            response_timeout: value.timeout(),
         }
     }
 }
diff --git a/ffi/rodbus-ffi/src/helpers/ext.rs b/ffi/rodbus-ffi/src/helpers/ext.rs
@@ -1,17 +1,5 @@
 use crate::ffi;
-use rodbus::client::RequestParam;
-use rodbus::{BitIterator, RegisterIterator, RequestError, UnitId};
-
-use rodbus::client::FfiSession;
-
-impl ffi::RequestParam {
-    pub(crate) fn build_session(&self, channel: &crate::ClientChannel) -> FfiSession {
-        FfiSession::new(
-            channel.inner.clone(),
-            RequestParam::new(UnitId::new(self.unit_id()), self.timeout()),
-        )
-    }
-}
+use rodbus::{BitIterator, RegisterIterator, RequestError};
 
 impl<'a> sfio_promise::FutureType<Result<BitIterator<'a>, rodbus::RequestError>>
     for ffi::BitReadCallback

diff --git a/ffi/rodbus-schema/Cargo.toml b/ffi/rodbus-schema/Cargo.toml
@@ -2,14 +2,22 @@
 name = "rodbus-schema"
 # this is the version that all the FFI libraries get, since it's in their schema
 version = "1.4.0-M1"
-authors = ["Step Function I/O LLC <[email protected]>"]
-edition = "2021"
 description = "oobindgen schema for Rodbus"
-keywords = ["ffi", "c", "modbus", "industrial", "plc"]
-categories = ["network-programming"]
-repository = "https://github.com/stepfunc/rodbus"
 readme = "../README.md"
 
+# inherit from workspace
+authors.workspace = true
+rust-version.workspace = true
+edition.workspace = true
+license-file.workspace = true
+homepage.workspace = true
+repository.workspace = true
+keywords.workspace = true
+categories.workspace = true
+
+[lints]
+workspace = true
+
 [dependencies]
 oo-bindgen = { workspace = true }
 sfio-tokio-ffi = { workspace = true }

diff --git a/ffi/rodbus-schema/src/lib.rs b/ffi/rodbus-schema/src/lib.rs
@@ -1,3 +1,5 @@
+//! FFI oo-bindgen schema for Rodbus
+
 use std::path::PathBuf;
 
 use crate::common::CommonDefinitions;
@@ -11,6 +13,7 @@ mod server;
 // derived from Cargo.toml
 const VERSION: &str = env!("CARGO_PKG_VERSION");
 
+/// Build the library schema
 pub fn build_lib() -> BackTraced<Library> {
     let info = LibraryInfo {
         description: "Safe and fast Modbus library".to_string(),

diff --git a/rodbus-client/Cargo.toml b/rodbus-client/Cargo.toml
@@ -1,14 +1,22 @@
 [package]
 name = "rodbus-client"
 version = "1.4.0-M1"
-authors = ["Step Function I/O LLC <[email protected]>>"]
-edition = "2021"
 description = "A command line program for making Modbus client requests using the Rodbus crate"
-keywords = ["modbus", "ics", "industrial", "plc", "security"]
-categories = ["network-programming"]
-repository = "https://github.com/stepfunc/rodbus"
 readme = "README.md"
 
+# inherit from workspace
+authors.workspace = true
+rust-version.workspace = true
+edition.workspace = true
+license-file.workspace = true
+homepage.workspace = true
+repository.workspace = true
+keywords.workspace = true
+categories.workspace = true
+
+[lints]
+workspace = true
+
 [[bin]]
 name = "rodbus-client"
 path = "src/main.rs"

diff --git a/rodbus-client/src/main.rs b/rodbus-client/src/main.rs
@@ -1,3 +1,5 @@
+//! Command-line Modbus client
+
 use std::fmt::Formatter;
 use std::net::{AddrParseError, SocketAddr};
 use std::num::ParseIntError;