A basic implementation of fully compliant GMS machine type #783
Conversation
|
|
There was a problem hiding this comment.
Thank you @mindonwarp for your PR:
I have left a few comments inline. From the technical side, a further expert is going to review.
Looking forward to get this merged.
There was a problem hiding this comment.
Hey,
thank you for your contribution. It looks very good. I have some smaller ideas.
Also I think the umatiLib part should be reviewed by @ccvca.
Thank you
Sebastian Friedl
| @@ -114,8 +116,8 @@ class BindVariable { | |||
| if constexpr (std::is_enum<T>::value) { | |||
| return getToVariantFuncForEnumArray(value); | |||
| } else if constexpr ( | |||
| std::is_class<T>::value && | |||
There was a problem hiding this comment.
@ccvca : Can you look at this change?
There was a problem hiding this comment.
This should not be necessary, as LocalizedText is handled as a structured data type.
There was a problem hiding this comment.
Without this the compiler spits out an error message: "error C3861: 'bindStructReflArray': identifier not found" while resolving a binding for a std::vector<LocalizedText_t>. The bindStructReflArray gets called inside the bindStructuredValueByPathArray method. What am I doing wrong? @ccvca
| } | ||
| UA_Variant_setArray(dst, tmp, pVariable->size(), &UA_TYPES[UA_TYPES_LOCALIZEDTEXT]); | ||
| }; | ||
| } |
There was a problem hiding this comment.
This library treats LocalizedText as a StructuredDataType, so it's not a "SimpleValue".
There was a problem hiding this comment.
There was no match for a std::vector<LocalizedText_t> though. Where should this be handled? @ccvca
| @@ -114,8 +116,8 @@ class BindVariable { | |||
| if constexpr (std::is_enum<T>::value) { | |||
| return getToVariantFuncForEnumArray(value); | |||
| } else if constexpr ( | |||
| std::is_class<T>::value && | |||
There was a problem hiding this comment.
This should not be necessary, as LocalizedText is handled as a structured data type.
There was a problem hiding this comment.
Thank you Alen for your contribution and the changes.
Please resolve the review comments from @Kantiran91 and @ccvca and fix the linter errors and rebase the branch to develop.
Merging when finished!
Hi @GoetzGoerisch, thank you. It will have to wait until this evening since I can only do it on my free time. |
|
The linter is complaining about "clang_format" and I can't find any issues in the code. Could it be a glitch? Maybe rerun the linter? |
Fixed in my commit, please incorporate this into the correct commits. |
|
Just a side note: With the release of GMS 1.0 the open62541 PR should be taken into account: |
|
@ccvca could you please to get this ready for merge? |
|
@mindonwarp after speaking with @ccvca we would merge as is, but Christian has to do some architecture adaptation afterwards due to the open discussions? Agreed? |
I'm definitely ok with that. 👍 |
Move common functionality to base class
fix(GMS): Linting
Fix build - don't use C++20 features
I needed an example of how to create a GMS compliant machine so I ended up using my free time to create an example myself.
There is still work to do but I wanted to share this with others who might have need of it.
Please note that I have never worked with this and all I did was mostly done by looking at how the existing code works and by deciphering compiler and debugger error messages.
I hope this implementation is at least correct enough to serve as a base for a better implementation.