Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom structures client #15

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
296 changes: 223 additions & 73 deletions Cargo.lock

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions async-opcua-client/src/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,13 +463,13 @@ impl Session {
}

/// Return index of supplied namespace url from cache
pub fn get_namespace_index_from_cache(&mut self, url: &str) -> Option<u16> {
pub fn get_namespace_index_from_cache(&self, url: &str) -> Option<u16> {
self.encoding_context.read().namespaces().get_index(url)
}

/// Return index of supplied namespace url
/// by first looking at namespace cache and querying server if necessary
pub async fn get_namespace_index(&mut self, url: &str) -> Result<u16, Error> {
pub async fn get_namespace_index(&self, url: &str) -> Result<u16, Error> {
if let Some(idx) = self.get_namespace_index_from_cache(url) {
return Ok(idx);
};
Expand Down
29 changes: 20 additions & 9 deletions async-opcua-server/src/node_manager/memory/simple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use crate::{
};
use opcua_core::sync::RwLock;
use opcua_types::{
AttributeId, DataValue, MonitoringMode, NodeId, NumericRange, StatusCode, TimestampsToReturn,
Variant,
AttributeId, DataValue, MonitoringMode, NodeClass, NodeId, NumericRange, StatusCode,
TimestampsToReturn, Variant,
};

use super::{
Expand Down Expand Up @@ -363,19 +363,30 @@ impl SimpleNodeManagerImpl {
return;
}
};

let (NodeType::Variable(var), AttributeId::Value) = (node, write.value().attribute_id)
else {
if node.node_class() != NodeClass::Variable
|| write.value().attribute_id != AttributeId::Value
{
write.set_status(StatusCode::BadNotWritable);
return;
};
}

let Some(cb) = cbs.get(var.node_id()) else {
write.set_status(StatusCode::BadNotWritable);
if let Some(cb) = cbs.get(node.as_node().node_id()) {
write.set_status(cb(write.value().value.clone(), &write.value().index_range));
return;
};

write.set_status(cb(write.value().value.clone(), &write.value().index_range));
if let Some(value) = &write.value().value.value {
let res = node
.as_mut_node()
.set_attribute(AttributeId::Value, value.clone());
if let Err(status_code) = res {
write.set_status(status_code);
}
write.set_status(StatusCode::Good);
return;
}

write.set_status(StatusCode::BadNothingToDo);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@einarmo when adding write to example client, I found out I would recevied BadNotWrittable . Looking at the code, it looks like only callbacks where supported for that SimpleNodeManager... So I added that as an exercise
I am doing something completely wrong or that code was really missing? Don't we have tests for that node manager?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused with how you changed the code here. There may have been a bug, but I don't think the old code was so wrong you needed to completely rewrite it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your issue may be that there's an unwritten assumption here. If you define a write callback you should also define a read callback. It's assumed that in this case the value is stored externally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no, reading your code, you added writing to the variable even without a callback. I don't want to support this, because IMO it never makes sense in a real server, only in a toy. In any real server you want all writes to somehow have a permanent effect, which means that you need a callback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I added writing value to address space without callback, so in memory and no persistence.

IMO it never makes sense in a real server, only in a toy.

I have been working with ua in at least 15 years now and we do that all the time. There are so many cases where you do not need persistence.
In that particular case I am writting a PLC emulator and I really do not care about persistence. I want it to restart as per the config

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. Alright.., but I'll ask you to change the first bit of the code back, the line where I checked if it was a Variable node. Your code is semantically mostly the same, but I don't see a reason for the change.

I still think my argument holds. OPC-UA is an interface, if you are interfacing with nothing then there's no real point to allowing writes at all. A PLC simulator would still want to actually simulate the PLC, meaning it would need to handle writes. Deferring that to the interface layer is, IMO, a misuse of the library.

Essentially you're abusing the fact that OPC-UA kinda looks like your PLC to use it to store simulation values, while what OPC-UA is actually used for is to allow access to values in some real system, simulation or not.

But it's fine, the simple node manager is not really the right choice for most real OPC-UA servers anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I checked again and you are right we are in fact not doing that for anything else than CI.
Maybe I can just remove that code and use some CB. In an example we should show best practices. but maybe in that case we should make that method return some clear error or at least log something for the developers.

our code is semantically mostly the same, but I don't see a reason for the change.

that was to make borrow checker happy, since I reused some variables. but it will become obsolete if I remove that anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can make that work, you shouldn't be calling set_attribute anyway, but the more advanced set_value_range, I think...

I gave this some more thought and I think I'm fine with your change. The conflict I think comes from the fact that I consider an OPC-UA server something you build on top of an existing system, to provide an interface. If what you do is instead create something that acts as a client and reacts on changes instead of being directly triggered by writes, it would indeed make sense to store the value.

The users do still have control over whether values should be written, since they have the AccessLevel attribute.

I'm at least open for looking into it more. Perhaps we could disallow writes if a read callback was defined? I'm open for opinions on this.

}

/// Add a callback called on `Write` for the node given by `id`.
Expand Down
30 changes: 28 additions & 2 deletions async-opcua-types/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,43 @@

use thiserror::Error;

use crate::VariantScalarTypeId;
use crate::{StatusCode, VariantScalarTypeId};

/// Rust OpcUa specific errors
#[allow(missing_docs)]
#[derive(Error, Debug)]
pub enum OpcUAError {
pub enum OpcUaError {
#[error("Received an unexpected variant type")]
UnexpectedVariantType {
variant_id: Option<VariantScalarTypeId>,
message: String,
},
#[error("The requested namespace does not exists")]
NamespaceDoesNotExist(String),
#[error("Namespace is out of range of a u16.")]
NamespaceOutOfRange,
#[error("Supplied node resolver was unable to resolve a reference type.")]
UnresolvedReferenceType,
#[error("Path does not match a node.")]
NoMatch,
#[error("Path segment is unusually long and has been rejected.")]
PathSegmentTooLong,
#[error("Number of elements in relative path is too large.")]
TooManyElementsInPath,
#[error("Request returned a StatusCode Error.")]
StatusCodeError(StatusCode),
#[error("Generic Error.")]
Error(crate::Error),
}

impl From<StatusCode> for OpcUaError {
fn from(value: StatusCode) -> Self {
OpcUaError::StatusCodeError(value)
}
}

impl From<crate::Error> for OpcUaError {
fn from(value: crate::Error) -> Self {
OpcUaError::Error(value)
}
}
30 changes: 28 additions & 2 deletions async-opcua-types/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
status_code::StatusCode,
string::UAString,
variant::Variant,
AnonymousIdentityToken, ApplicationDescription, CallMethodRequest, DataTypeId,
AnonymousIdentityToken, ApplicationDescription, CallMethodRequest, DataTypeId, DataValue,
EndpointDescription, Error, ExpandedNodeId, HistoryUpdateType, IdentityCriteriaType,
MessageSecurityMode, MonitoredItemCreateRequest, MonitoringMode, MonitoringParameters,
NumericRange, ObjectId, ReadValueId, ServiceCounterDataType, ServiceFault, SignatureData,
UserNameIdentityToken, UserTokenPolicy, UserTokenType,
UserNameIdentityToken, UserTokenPolicy, UserTokenType, WriteValue,
};

use super::PerformUpdateType;
Expand Down Expand Up @@ -414,3 +414,29 @@
Self::Anonymous
}
}

impl WriteValue {
pub fn new(

Check failure on line 419 in async-opcua-types/src/impls.rs

View workflow job for this annotation

GitHub Actions / build-linux (stable)

missing documentation for an associated function

Check failure on line 419 in async-opcua-types/src/impls.rs

View workflow job for this annotation

GitHub Actions / build-linux (beta)

missing documentation for an associated function
node_id: NodeId,
attribute_id: AttributeId,
index_range: UAString,
value: DataValue,
) -> Self {
Self {
node_id,
attribute_id: attribute_id as u32,
index_range,
value,
}
}

/// return a WriteValue with AttributeId::Value, which is the most common case
pub fn value_attr(node_id: NodeId, val: Variant) -> Self {
Self {
node_id,
attribute_id: AttributeId::Value as u32,
index_range: UAString::null(),
value: val.into(),
}
}
}
6 changes: 3 additions & 3 deletions async-opcua-types/src/namespaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use hashbrown::HashMap;

use crate::{errors::OpcUAError, ExpandedNodeId, NodeId, Variant};
use crate::{errors::OpcUaError, ExpandedNodeId, NodeId, Variant};

/// Utility for handling assignment of namespaces on server startup.
#[derive(Debug, Default, Clone)]
Expand All @@ -28,15 +28,15 @@ impl NamespaceMap {

/// Create a new namespace map from a vec of variant as we get when reading
/// the namespace array from the server
pub fn new_from_variant_array(array: &[Variant]) -> Result<Self, OpcUAError> {
pub fn new_from_variant_array(array: &[Variant]) -> Result<Self, OpcUaError> {
let known_namespaces: HashMap<String, u16> = array
.iter()
.enumerate()
.map(|(idx, v)| {
if let Variant::String(s) = v {
Ok((s.value().clone().unwrap_or(String::new()), idx as u16))
} else {
Err(OpcUAError::UnexpectedVariantType {
Err(OpcUaError::UnexpectedVariantType {
variant_id: v.scalar_type_id(),
message: "Namespace array on server contains invalid data".to_string(),
})
Expand Down
2 changes: 1 addition & 1 deletion async-opcua-types/src/node_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ impl PartialEq<DataTypeId> for NodeId {
}
}

static NEXT_NODE_ID_NUMERIC: AtomicUsize = AtomicUsize::new(0);
static NEXT_NODE_ID_NUMERIC: AtomicUsize = AtomicUsize::new(1);

impl Default for NodeId {
fn default() -> Self {
Expand Down
81 changes: 54 additions & 27 deletions async-opcua-types/src/relative_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use log::error;
use regex::Regex;

use crate::{
errors::OpcUaError,
node_id::{Identifier, NodeId},
qualified_name::QualifiedName,
string::UAString,
Expand All @@ -28,7 +29,7 @@ impl RelativePath {
/// Converts a string into a relative path. Caller must supply a `node_resolver` which will
/// be used to look up nodes from their browse name. The function will reject strings
/// that look unusually long or contain too many elements.
pub fn from_str<CB>(path: &str, node_resolver: &CB) -> Result<RelativePath, RelativePathError>
pub fn from_str<CB>(path: &str, node_resolver: &CB) -> Result<RelativePath, OpcUaError>
where
CB: Fn(u16, &str) -> Option<NodeId>,
{
Expand Down Expand Up @@ -66,14 +67,14 @@ impl RelativePath {
}
if token.len() > Self::MAX_TOKEN_LEN {
error!("Path segment seems unusually long and has been rejected");
return Err(RelativePathError::PathSegmentTooLong);
return Err(OpcUaError::PathSegmentTooLong);
}
}

if !token.is_empty() {
if elements.len() == Self::MAX_ELEMENTS {
error!("Number of elements in relative path is too long, rejecting it");
return Err(RelativePathError::TooManyElements);
return Err(OpcUaError::TooManyElementsInPath);
}
elements.push(RelativePathElement::from_str(&token, node_resolver)?);
}
Expand All @@ -84,6 +85,50 @@ impl RelativePath {
}
}

impl From<&[QualifiedName]> for RelativePath {
fn from(value: &[QualifiedName]) -> Self {
let elements = value
.iter()
.map(|qn| RelativePathElement {
reference_type_id: ReferenceTypeId::HierarchicalReferences.into(),
is_inverse: false,
include_subtypes: true,
target_name: qn.clone(),
})
.collect();
Self {
elements: Some(elements),
}
}
}
// Cannot use
//impl<T: AsRef<str>> TryFrom<T> for RelativePath {
// for some strange reasons so implementing all thee manually here
//
impl TryFrom<&str> for RelativePath {
type Error = OpcUaError;

fn try_from(value: &str) -> Result<Self, Self::Error> {
RelativePath::from_str(value, &RelativePathElement::default_node_resolver)
}
}

impl TryFrom<&String> for RelativePath {
type Error = OpcUaError;

fn try_from(value: &String) -> Result<Self, Self::Error> {
RelativePath::from_str(value, &RelativePathElement::default_node_resolver)
}
}

impl TryFrom<String> for RelativePath {
type Error = OpcUaError;

fn try_from(value: String) -> Result<Self, Self::Error> {
RelativePath::from_str(&value, &RelativePathElement::default_node_resolver)
}
}

impl<'a> From<&'a RelativePathElement> for String {
fn from(element: &'a RelativePathElement) -> String {
let mut result = element
Expand Down Expand Up @@ -226,10 +271,7 @@ impl RelativePathElement {
/// * `<!NonHierarchicalReferences>foo`
/// * `<#!2:MyReftype>2:blah`
///
pub fn from_str<CB>(
path: &str,
node_resolver: &CB,
) -> Result<RelativePathElement, RelativePathError>
pub fn from_str<CB>(path: &str, node_resolver: &CB) -> Result<RelativePathElement, OpcUaError>
where
CB: Fn(u16, &str) -> Option<NodeId>,
{
Expand Down Expand Up @@ -270,7 +312,7 @@ impl RelativePathElement {
node_resolver(namespace, browse_name)
} else {
error!("Namespace {} is out of range", namespace);
return Err(RelativePathError::NamespaceOutOfRange);
return Err(OpcUaError::NamespaceOutOfRange);
}
} else {
node_resolver(0, browse_name)
Expand All @@ -280,7 +322,7 @@ impl RelativePathElement {
"Supplied node resolver was unable to resolve a reference type from {}",
path
);
return Err(RelativePathError::UnresolvedReferenceType);
return Err(OpcUaError::UnresolvedReferenceType);
}
(reference_type_id.unwrap(), include_subtypes, is_inverse)
}
Expand All @@ -293,7 +335,7 @@ impl RelativePathElement {
})
} else {
error!("Path {} does not match a relative path", path);
Err(RelativePathError::NoMatch)
Err(OpcUaError::NoMatch)
}
}

Expand Down Expand Up @@ -340,21 +382,6 @@ impl RelativePathElement {
}
}

#[derive(Debug, Clone)]
/// Error returned from parsing a relative path.
pub enum RelativePathError {
/// Namespace is out of range of a u16.
NamespaceOutOfRange,
/// Supplied node resolver was unable to resolve a reference type.
UnresolvedReferenceType,
/// Path does not match a relative path.
NoMatch,
/// Path segment is unusually long and has been rejected.
PathSegmentTooLong,
/// Number of elements in relative path is too large.
TooManyElements,
}

impl<'a> From<&'a RelativePath> for String {
fn from(path: &'a RelativePath) -> String {
if let Some(ref elements) = path.elements {
Expand Down Expand Up @@ -398,7 +425,7 @@ fn unescape_browse_name(name: &str) -> String {
/// * 0:foo
/// * bar
///
fn target_name(target_name: &str) -> Result<QualifiedName, RelativePathError> {
fn target_name(target_name: &str) -> Result<QualifiedName, OpcUaError> {
static RE: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"((?P<nsidx>[0-9+]):)?(?P<name>.*)").unwrap());
if let Some(captures) = RE.captures(target_name) {
Expand All @@ -410,7 +437,7 @@ fn target_name(target_name: &str) -> Result<QualifiedName, RelativePathError> {
"Namespace {} for target name is out of range",
namespace.as_str()
);
return Err(RelativePathError::NamespaceOutOfRange);
return Err(OpcUaError::NamespaceOutOfRange);
}
} else {
0
Expand Down
21 changes: 21 additions & 0 deletions samples/custom-structures-client/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
[package]
name = "opcua-structure-client"
version = "0.13.0" # OPCUARustVersion
authors = ["Rust-OpcUa contributors"]
edition = "2021"

[dependencies]
pico-args = "0.5"
tokio = { version = "1.36.0", features = ["full"] }
log = { workspace = true }

[dependencies.async-opcua]
path = "../../async-opcua"
version = "0.13.0" # OPCUARustVersion
features = ["client", "console-logging"]
default-features = false

[features]
default = ["json", "xml"]
json = ["async-opcua/json"]
xml = ["async-opcua/xml"]
6 changes: 6 additions & 0 deletions samples/custom-structures-client/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
To run this sample:

1. Launch `samples/demo-server` as that server exposes custom enums and variables
2. Run as `cargo run`

The client connects to the server, reads a variable and disconnects.
Loading
Loading