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
Open

Custom structures client #15

wants to merge 11 commits into from

Conversation

oroulet
Copy link
Member

@oroulet oroulet commented Jan 1, 2025

No description provided.

@oroulet oroulet changed the base branch from master to custom_example January 1, 2025 10:46
panic!("Unexpected variant type");
};
//dbg!(&val);
//let val: DynamicStructure = *val.into_inner_as().unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

that one works fine

//dbg!(&val);
//let val: DynamicStructure = *val.into_inner_as().unwrap();
//dbg!(val.values());
let val: ErrorData = *val.into_inner_as().unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

is that possible at all? how? @einarmo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just taking the value stored in the extension object and downcasting it to the type it really is. Getting it as ErrorData would require somehow deserializing from a DynamicStructure to a specific struct. I suppose something like that could be implemented, but not trivially.

into_inner_as will only return Some if the type stored inside the extension object is the type you're calling it with. It won't do any clever conversions.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to actually deserialize to an ErrorData you need to create a type loader for that type, like you did on the server side. DynamicStructure is primarily intended to be used when you can't do that. For example if you are creating a generic client like UAExpert, and you want to support custom structures, you need some way to deserialize from OPC-UA binary using only runtime information.

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 I tried to look around in code but could not find how to deserialize to ErrorData. Any pointer?

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 If you know something I would really appreciate. I really feel I am overlooking something not that hard here that should should be documented

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. You have two real options here.

First, you can do like you do on the server and create a TypeLoader that supports ErrorData instead of using DynamicStructure.

Your other option is to do something like this:

ErrorData {
    message: dyn_structure.get_value_by_name("Message").unwrap().clone().try_cast_to().unwrap(),
    error_id: dyn_structure.get_value_by_name("ErrorId").unwrap().clone().try_cast_to().unwrap(),
    ...
}

In theory we could make a macro to implement that automatically, and maybe have a way to consume a DynamicStructure and return its values so we can drop the clone.

Part of your problem here is that you are really meant to use only one of the two methods:

  • Either you know the types you need at compile time, so you make a TypeLoader to convert directly to those.
  • Or you don't, so you use DynamicStructure.

You are kinda trying to do both at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. made decoding works now.
Next is to look at encoding
but all these hardcoded node ids is not that great. need a closer look at it later

Base automatically changed from custom_example to master January 8, 2025 06:11
@oroulet oroulet force-pushed the custom-struc-client branch from 53da4a6 to de3cb20 Compare January 8, 2025 08:07
@oroulet oroulet changed the title Draft: Custom struc client Custom struc client Jan 8, 2025
@oroulet oroulet force-pushed the custom-struc-client branch 2 times, most recently from e54d13d to d29c249 Compare January 8, 2025 08:15
@oroulet
Copy link
Member Author

oroulet commented Jan 8, 2025

@einarmo ready now I think.

@svanharmelen svanharmelen changed the title Custom struc client Custom structures client Jan 8, 2025
Copy link
Contributor

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

Al lot of nitpicking, but I think good readable code is important (especially in an example, but in general as well).

samples/custom-structures-client/README.md Outdated Show resolved Hide resolved
samples/custom-structures-client/README.md Outdated Show resolved Hide resolved
opcua-types/src/impls.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
samples/custom-structures-client/src/main.rs Outdated Show resolved Hide resolved
@oroulet
Copy link
Member Author

oroulet commented Jan 8, 2025

Al lot of nitpicking, but I think good readable code is important (especially in an example, but in general as well).

@svanharmelen not problem. I like feedback ;-)

@oroulet oroulet force-pushed the custom-struc-client branch from 761d55d to 9a32f59 Compare January 8, 2025 12:05
@oroulet oroulet requested a review from svanharmelen January 8, 2025 20:25
@svanharmelen
Copy link
Contributor

Noticed the changes (looks much better already), but there are still an number (4 or 5) of unresolved comments. Could you check those as well? Thanks!

@oroulet
Copy link
Member Author

oroulet commented Jan 9, 2025

OK I see that maybe I should show how to do automatic serialization to rust objects too or remove code related to that here. I jsut need to find out how to do that...

@oroulet oroulet force-pushed the custom-struc-client branch from 84e5064 to 541695b Compare January 11, 2025 11:40
@oroulet oroulet force-pushed the custom-struc-client branch from 541695b to 1dd962c Compare January 11, 2025 12:31
.build(session)
.await
.unwrap();
let type_tree = Arc::new(type_tree);
Copy link
Member Author

Choose a reason for hiding this comment

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

Part of your problem here is that you are really meant to use only one of the two methods:
Either you know the types you need at compile time, so you make a TypeLoader to convert directly to those.
Or you don't, so you use DynamicStructure.

I want to document both things. so now I splittet the example in two binaries s it clearer.

If you want to actually deserialize to an ErrorData you need to create a type loader for that type, like you did on the server side.

that is the part I do not get. On the server side I do not create any TypeLoader. I create the necessary data type and encoding nodes with the correct typedefinition. How do I create a TypeLoader on client side? @einarmo

Copy link
Contributor

@einarmo einarmo Jan 11, 2025

Choose a reason for hiding this comment

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

Ahh... Right, to decode the type you need a TypeLoader, you really should have that on the server as well.

Again, this isn't particularly easy to do by hand, and can probably be improved, you can use a static TypeLoaderInstance. See async-opcua-types/src/generated/types/mod.rs for an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Essentially you need to register a callback for each type you want to be able to deserialize, and match those based on the namespace of the encoding ID or data type ID you receive.

@oroulet oroulet force-pushed the custom-struc-client branch from 1dd962c to 4a1e02f Compare January 12, 2025 14:56
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants