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

opcua: Fixes issues with computing "information" based on path #1270

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

wilterdinkrobert
Copy link
Contributor

Bugfix

Timeseries with nodeid paths containing \\ in the string part failed with re.error bad escape \... at position .... The fullmatch(path, config) would compile the path part as a regex, but cannot be assumed to be correctly escaped (hence the re.error). Upon closer inspection the rather complicated code can be removed completely by storing the information with the subscription when it is first made. Functionality from configuration pov should be unchanged/equivalent.

Test configuration:

        "timeseries": [
          {
            "key": "MachineRt_Error_Status",
            "path": "${ns=2;s=MachineRt\\Error\\Status}"
          },{
            "key": "MachineRt_Cycle_Status",
            "path": "${ns=2;s=MachineRt\\Cycle\\Status}"
          }]

Tested with several small OPCUA servers.

@imbeacon
Copy link
Member

Hi @wilterdinkrobert,
Thank you for your interest and participating in the project life. I have checked the difference and could you confirm that configuration like the following one will still work correctly? Or the same path, but from the Root object on the server?

        "timeseries": [
          {
            "key": "MachineRt_Error_Status",
            "path": "${MachineRt\\.Error\\.Status}"
          },{
            "key": "MachineRt_Cycle_Status",
            "path": "${MachineRt\\.Cycle\\.Status}"
          }]

@wilterdinkrobert
Copy link
Contributor Author

wilterdinkrobert commented Dec 28, 2023

Yes, I did test the partial browse_path. I've now also tested with the full browse_path (from Root) and encountered some more bugs/inconsistencies (not related to the previous commit). Could you please confirm that the tests in 8f67463 match the intended behavior?

@imbeacon
Copy link
Member

Yes, looks good.

@wilterdinkrobert
Copy link
Contributor Author

@imbeacon Thanks for confirming. I just pushed the last cleanup commit. I think it's ready for merging now, what do you think?

@imbeacon imbeacon merged commit 05afa6c into thingsboard:master Dec 29, 2023
3 checks passed
@wilterdinkrobert wilterdinkrobert deleted the bugfix/remove-regex branch January 8, 2024 08:36
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