HMIProject / open62541

11 stars 6 forks source link

Implement reading server node attributes #150

Closed sgoll closed 3 months ago

sgoll commented 3 months ago

Description

This is an alternative to #146 and proposes a slightly more idiomatic approach that does not require a macro at the call site to implement type-safe reading of node attributes.

When this is implemented, we could use the same approach and refactor AsyncClient::read_attribute() and all related methods in a similar way.

LeanderGlanda commented 3 months ago

@sgoll I don't see any real disadvantages of this. Maybe the only thing is that now you need the _T at the back. A way to convert between the _T struct and the normal attribute id would be nice. Maybe you get a ua::AttributeId passed from somewhere and then you need to convert somehow to be able to read. If this can be solved, I'm sold. As it brings advantages too: There is an option to get a ua::Variant (could be a disadvantage to as it adds a error cause, but I guess that can be handled somehow) and there is no macro that should be accessible by Server.xy but is in the root namespace. Also using a function/method instead of a macro enables better IDE support.

sgoll commented 3 months ago

Maybe the only thing is that now you need the _T at the back.

Yes, I had some mixed feelings about this as well. However, I didn't want to rename the existing constants because that would have introduced an inconsistency between how we handle different enums.

In the end, I hope that the docs are clear enough. Also nothing too bad happens when you use the "wrong" associated constant of ua::AttributeId, you just don't get the expected type back right away.

A way to convert between the _T struct and the normal attribute id would be nice. Maybe you get a ua::AttributeId passed from somewhere and then you need to convert somehow to be able to read. If this can be solved, I'm sold.

I can't follow. When you receive an arbitrary ua::AttributeId, you can already use this directly with read_attribute(), as indicated in the docs and in the examples. This is made possible by the following implementation:

https://github.com/HMIProject/open62541/blob/8a62b7656e1fa01db6dcdc79c43f582422c3fe2a/src/attributes.rs#L54-L60

There really is no way to have dynamic data at runtime resolve into a statically known type at compile time. So, by using &ua::AttributeId, you always get back the underlying ua::Variant as-is.

As it brings advantages too: There is an option to get a ua::Variant (could be a disadvantage to as it adds a error cause, but I guess that can be handled somehow) and there is no macro that should be accessible by Server.xy but is in the root namespace. Also using a function/method instead of a macro enables better IDE support.

👍

LeanderGlanda commented 3 months ago

I just thought about it again and found out that my point does not make sense with an explicit type system. Either you know which ua::AttributeId you get and then use the data type as required, or you don't know which ua::AttributeId you get and then you need something like the ua::Variant type.

So let's use this approach instead of my approach.


I didn't give the idea of creating a struct for every attribute id idea much attention. But seeing this now makes me question why I did not continue looking into this. Anyway, I learnt a little bit of rust macros on the way and now even a seemingly good way to handle this problem, so it was worth the time for me personally.

sgoll commented 3 months ago

I didn't give the idea of creating a struct for every attribute id idea much attention. But seeing this now makes me question why I did not continue looking into this.

If you want to dig deeper, a good place to start would be the Typestate pattern used in Rust, to which this is somewhat related.

Anyway, I learnt a little bit of rust macros on the way and now even a seemingly good way to handle this problem, so it was worth the time for me personally.

👍