Change modbus to use VariableBuilder completely and change interface …

…for setter/getter to make it easier for callers.

samples/demo-server/src/machine.rs

@@ -84,10 +84,10 @@ fn add_machine(address_space: &mut AddressSpace, folder_id: NodeId, name: &str,
     VariableBuilder::new(&counter_id, "Counter", "Counter")
         .property_of(machine_id.clone())
         .has_type_definition(VariableTypeId::PropertyType)
-        .value_getter(move |_, _, _, _, _| -> Result<Option<DataValue>, StatusCode> {
+        .value_getter(AttrFnGetter::new_boxed(move |_, _, _, _, _| -> Result<Option<DataValue>, StatusCode> {
             let value = counter.load(Ordering::Relaxed);
             Ok(Some(DataValue::new(value)))
-        })
+        }))
         .insert(address_space);
 
     machine_id

samples/modbus-server/modbus.conf

@@ -1,9 +1,9 @@
 ---
 slave_address: "127.0.0.1:502"
-read_interval: 1000
+read_interval: 100
 output_coils:
   base_address: 0
-  count: 2
+  count: 30
 input_coils:
   base_address: 0
   count: 20

samples/modbus-server/src/config.rs

@@ -4,6 +4,8 @@ use std::{
     path::Path,
 };
 
+use opcua_server::prelude::*;
+
 use crate::Table;
 
 #[derive(Deserialize, Clone, Copy, PartialEq)]
@@ -22,6 +24,42 @@ pub enum AliasType {
     Double,
 }
 
+impl Into<DataTypeId> for AliasType {
+    fn into(self) -> DataTypeId {
+        match self {
+            Self::Boolean => DataTypeId::Boolean,
+            Self::Byte => DataTypeId::Byte,
+            Self::SByte => DataTypeId::SByte,
+            Self::UInt16 | Self::Default => DataTypeId::UInt16,
+            Self::Int16 => DataTypeId::Int16,
+            Self::UInt32 => DataTypeId::UInt32,
+            Self::Int32 => DataTypeId::Int32,
+            Self::UInt64 => DataTypeId::UInt64,
+            Self::Int64 => DataTypeId::Int64,
+            Self::Float => DataTypeId::Float,
+            Self::Double => DataTypeId::Double,
+        }
+    }
+}
+
+impl Into<VariantTypeId> for AliasType {
+    fn into(self) -> VariantTypeId {
+        match self {
+            Self::Boolean => VariantTypeId::Boolean,
+            Self::Byte => VariantTypeId::Byte,
+            Self::SByte => VariantTypeId::SByte,
+            Self::UInt16 | AliasType::Default => VariantTypeId::UInt16,
+            Self::Int16 => VariantTypeId::Int16,
+            Self::UInt32 => VariantTypeId::UInt32,
+            Self::Int32 => VariantTypeId::Int32,
+            Self::UInt64 => VariantTypeId::UInt64,
+            Self::Int64 => VariantTypeId::Int64,
+            Self::Float => VariantTypeId::Float,
+            Self::Double => VariantTypeId::Double,
+        }
+    }
+}
+
 impl AliasType {
     /// Returns the size of the type in number of registers
     pub fn size_in_words(&self) -> u16 {

samples/modbus-server/src/main.rs

@@ -87,6 +87,8 @@ fn main() {
         std::process::exit(1);
     };
 
+    opcua_console_logging::init();
+
     run(config, m.is_present("run-demo-slave"));
 }
 

samples/modbus-server/src/opcua.rs

@@ -135,27 +135,28 @@ fn add_aliases(runtime: &Arc<RwLock<Runtime>>, modbus: &Arc<Mutex<MODBUS>>, addr
             .add_folder("Aliases", "Aliases", parent_folder_id)
             .unwrap();
 
-        let variables = aliases.into_iter().map(move |alias| {
-            // Alias node ids are just their name in the list
+        // Create variables for all of the aliases
+        aliases.into_iter().for_each(move |alias| {
+            // Create a getter/setter
+            let getter_setter = Arc::new(Mutex::new(AliasGetterSetter::new(runtime.clone(), modbus.clone(), alias.clone())));
+            // Create a variable for the alias
             let node_id = NodeId::new(nsidx, alias.name.clone());
-            let mut v = Variable::new(&node_id, &alias.name, &alias.name, 0u16);
-
-            let writable = alias.writable;
-
-            // Set the reader
-            let getter_setter = Arc::new(Mutex::new(AliasGetterSetter::new(runtime.clone(), modbus.clone(), alias)));
-            v.set_value_getter(getter_setter.clone());
-
-            // Writable aliases need write permission
-            if writable {
-                v.set_access_level(AccessLevel::CURRENT_READ | AccessLevel::CURRENT_WRITE);
-                v.set_user_access_level(UserAccessLevel::CURRENT_READ | UserAccessLevel::CURRENT_WRITE);
-                v.set_value_setter(getter_setter);
-            }
-
-            v
-        }).collect();
-        let _ = address_space.add_variables(variables, &parent_folder_id);
+            let data_type: DataTypeId = alias.data_type.into();
+            let v = VariableBuilder::new(&node_id, &alias.name, &alias.name)
+                .organized_by(&parent_folder_id)
+                .data_type(data_type)
+                .value(0u16)
+                .value_getter(getter_setter.clone());
+
+            let v = if alias.writable {
+                v.value_setter(getter_setter)
+                    .writable()
+            } else {
+                v
+            };
+
+            v.insert(address_space);
+        });
     }
 }
 
@@ -164,62 +165,66 @@ fn make_variables<T>(modbus: &Arc<Mutex<MODBUS>>, address_space: &mut AddressSpa
     where T: 'static + Copy + Send + Sync + Into<Variant>
 {
     // Create variables
-    let variables = (start..end).for_each(|i| {
+    (start..end).for_each(|i| {
         let addr = i as u16;
-
         let name = name_formatter(i);
-
-        let mut v = VariableBuilder::new(&make_node_id(nsidx, table, addr), &name, &name)
-            .value(default_value);
-
         let values = values.clone();
-        v = v.value_getter(move |_node_id, _attribute_id, _numeric_range, _name, _f| -> Result<Option<DataValue>, StatusCode> {
-            let values = values.read().unwrap();
-            let value = *values.get(i - start).unwrap();
-            Ok(Some(DataValue::new(value)))
-        });
+        let v = VariableBuilder::new(&make_node_id(nsidx, table, addr), &name, &name)
+            .organized_by(parent_folder_id)
+            .value(default_value)
+            .value_getter(AttrFnGetter::new_boxed(move |_node_id, _attribute_id, _numeric_range, _name, _f| -> Result<Option<DataValue>, StatusCode> {
+                let values = values.read().unwrap();
+                let value = *values.get(i - start).unwrap();
+                Ok(Some(DataValue::new(value)))
+            }));
 
         // Output tables have setters too
         let v = match table {
+            Table::InputCoils => {
+                v.data_type(DataTypeId::Boolean)
+            }
             Table::OutputCoils => {
                 let modbus = modbus.clone();
-                v.value_setter(move |_node_id: &NodeId, _attribute_id: AttributeId, value: DataValue| -> Result<(), StatusCode> {
-                    // Try to cast to a bool
-                    let value = if let Some(value) = value.value {
-                        value.cast(VariantTypeId::Boolean)
-                    } else {
-                        Variant::Empty
-                    };
-                    if let Variant::Boolean(value) = value {
-                        let modbus = modbus.lock().unwrap();
-                        modbus.write_to_coil(addr, value);
-                        Ok(())
-                    } else {
-                        Err(StatusCode::BadTypeMismatch)
-                    }
-                }).writable()
+                v.data_type(DataTypeId::Boolean)
+                    .value_setter(AttrFnSetter::new_boxed(move |_node_id, _attribute_id, value| {
+                        // Try to cast to a bool
+                        let value = if let Some(value) = value.value {
+                            value.cast(VariantTypeId::Boolean)
+                        } else {
+                            Variant::Empty
+                        };
+                        if let Variant::Boolean(value) = value {
+                            let modbus = modbus.lock().unwrap();
+                            modbus.write_to_coil(addr, value);
+                            Ok(())
+                        } else {
+                            Err(StatusCode::BadTypeMismatch)
+                        }
+                    })).writable()
+            }
+            Table::InputRegisters => {
+                v.data_type(DataTypeId::UInt16)
             }
             Table::OutputRegisters => {
                 let modbus = modbus.clone();
-                v.value_setter(move |_node_id: &NodeId, _attribute_id: AttributeId, value: DataValue| -> Result<(), StatusCode> {                    // Try to cast to a u16
-                    let value = if let Some(value) = value.value {
-                        value.cast(VariantTypeId::UInt16)
-                    } else {
-                        Variant::Empty
-                    };
-                    if let Variant::UInt16(value) = value {
-                        let modbus = modbus.lock().unwrap();
-                        modbus.write_to_register(addr, value);
-                        Ok(())
-                    } else {
-                        Err(StatusCode::BadTypeMismatch)
-                    }
-                }).writable()
+                v.data_type(DataTypeId::UInt16)
+                    .value_setter(AttrFnSetter::new_boxed(move |_node_id, _attribute_id, value| {
+                        let value = if let Some(value) = value.value {
+                            value.cast(VariantTypeId::UInt16)
+                        } else {
+                            Variant::Empty
+                        };
+                        if let Variant::UInt16(value) = value {
+                            let modbus = modbus.lock().unwrap();
+                            modbus.write_to_register(addr, value);
+                            Ok(())
+                        } else {
+                            Err(StatusCode::BadTypeMismatch)
+                        }
+                    })).writable()
             }
-            _ => v
         };
-        v.organized_by(parent_folder_id)
-            .insert(address_space);
+        v.insert(address_space);
     });
 }
 
@@ -290,19 +295,7 @@ impl AliasGetterSetter {
             }
             Table::OutputRegisters => {
                 // Cast to the alias' expected type
-                let variant_type = match data_type {
-                    AliasType::Boolean => VariantTypeId::Boolean,
-                    AliasType::Byte => VariantTypeId::Byte,
-                    AliasType::SByte => VariantTypeId::SByte,
-                    AliasType::UInt16 | AliasType::Default => VariantTypeId::UInt16,
-                    AliasType::Int16 => VariantTypeId::Int16,
-                    AliasType::UInt32 => VariantTypeId::UInt32,
-                    AliasType::Int32 => VariantTypeId::Int32,
-                    AliasType::UInt64 => VariantTypeId::UInt64,
-                    AliasType::Int64 => VariantTypeId::Int64,
-                    AliasType::Float => VariantTypeId::Float,
-                    AliasType::Double => VariantTypeId::Double,
-                };
+                let variant_type: VariantTypeId = data_type.into();
                 let value = value.cast(variant_type);
                 // Write the words
                 let (_, words) = Self::value_to_words(value).map_err(|_| StatusCode::BadUnexpectedError)?;

server/src/address_space/mod.rs

@@ -1,7 +1,10 @@
 //! Provides functionality to create an address space, find nodes, add nodes, change attributes
 //! and values on nodes.
 
-use std::result::Result;
+use std::{
+    result::Result,
+    sync::{Arc, Mutex},
+};
 
 use opcua_types::{AttributeId, DataValue, NodeId, NumericRange, QualifiedName};
 use opcua_types::status_code::StatusCode;
@@ -23,6 +26,10 @@ impl<F> AttributeGetter for AttrFnGetter<F> where F: FnMut(&NodeId, AttributeId,
 
 impl<F> AttrFnGetter<F> where F: FnMut(&NodeId, AttributeId, NumericRange, &QualifiedName, f64) -> Result<Option<DataValue>, StatusCode> + Send {
     pub fn new(getter: F) -> AttrFnGetter<F> { AttrFnGetter { getter } }
+
+    pub fn new_boxed(getter: F) -> Arc<Mutex<AttrFnGetter<F>>> {
+        Arc::new(Mutex::new(Self::new(getter)))
+    }
 }
 
 /// An implementation of attribute setter that can be easily constructed using a mutable function
@@ -38,6 +45,10 @@ impl<F> AttributeSetter for AttrFnSetter<F> where F: FnMut(&NodeId, AttributeId,
 
 impl<F> AttrFnSetter<F> where F: FnMut(&NodeId, AttributeId, DataValue) -> Result<(), StatusCode> + Send {
     pub fn new(setter: F) -> AttrFnSetter<F> { AttrFnSetter { setter } }
+
+    pub fn new_boxed(setter: F) -> Arc<Mutex<AttrFnSetter<F>>> {
+        Arc::new(Mutex::new(Self::new(setter)))
+    }
 }
 
 // A macro for creating builders. Builders can be used for more conveniently creating objects,

server/src/address_space/variable.rs

@@ -8,8 +8,7 @@ use opcua_types::service_types::VariableAttributes;
 
 use crate::{
     address_space::{
-        AccessLevel, AttrFnGetter,
-        AttrFnSetter, base::Base,
+        AccessLevel, base::Base,
         node::{Node, NodeBase},
         UserAccessLevel,
     },
@@ -93,22 +92,18 @@ impl VariableBuilder {
     }
 
     /// Sets a value getter function for the variable. Whenever the value of a variable
-    /// needs to be fetched (e.g. from a monitored item subscription), this function will be called
+    /// needs to be fetched (e.g. from a monitored item subscription), this trait will be called
     /// to get the value.
-    pub fn value_getter<F>(mut self, getter: F) -> Self where
-        F: FnMut(&NodeId, AttributeId, NumericRange, &QualifiedName, f64) -> Result<Option<DataValue>, StatusCode> + Send + 'static
-    {
-        self.node.set_value_getter(Arc::new(Mutex::new(AttrFnGetter::new(getter))));
+    pub fn value_getter(mut self, getter: Arc<Mutex<dyn  AttributeGetter + Send>>) -> Self {
+        self.node.set_value_getter(getter);
         self
     }
 
     /// Sets a value setter function for the variable. Whenever the value of a variable is set via
-    /// a service, this function will be called to set the value. It is up to the implementation
+    /// a service, this trait will be called to set the value. It is up to the implementation
     /// to decide what to do if that happens.
-    pub fn value_setter<F>(mut self, setter: F) -> Self where
-        F: FnMut(&NodeId, AttributeId, DataValue) -> Result<(), StatusCode> + Send + 'static
-    {
-        self.node.set_value_setter(Arc::new(Mutex::new(AttrFnSetter::new(setter))));
+    pub fn value_setter(mut self, setter: Arc<Mutex<dyn AttributeSetter + Send>>) -> Self {
+        self.node.set_value_setter(setter);
         self
     }
 

server/src/services/attribute.rs

@@ -106,6 +106,7 @@ impl AttributeService {
     /// elements or to write ranges of elements of the composite.
     pub fn write(&self, _server_state: Arc<RwLock<ServerState>>, session: Arc<RwLock<Session>>, address_space: Arc<RwLock<AddressSpace>>, request: &WriteRequest) -> SupportedMessage {
         if is_empty_option_vec!(request.nodes_to_write) {
+            debug!("Empty list passed to write {:?}", request);
             self.service_fault(&request.request_header, StatusCode::BadNothingToDo)
         } else {
             let session = trace_read_lock_unwrap!(session);

server/src/services/mod.rs

@@ -7,7 +7,7 @@ trait Service {
     fn name(&self) -> String;
 
     fn service_fault(&self, request_header: &RequestHeader, service_result: StatusCode) -> SupportedMessage {
-        warn!("Service {}, request {} generated a service fault with status code {}", self.name(), request_header.request_handle, service_result);
+        warn!("Service {}, request handle {} generated a service fault with status code {}", self.name(), request_header.request_handle, service_result);
         ServiceFault::new_supported_message(request_header, service_result)
     }
 }