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

Adding support for non-implicit ca protocls #70

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KaushikMalapati
Copy link

@KaushikMalapati KaushikMalapati commented Feb 11, 2025

So far I've only edited main_window.process_message. It's not obvious to me where else I would need to account for protocol prefixes.

I also am not sure how I can test this, but at the very least I was able to:

  • pass all the pytests
  • pass the pre-commit jobs
  • pass the github workflows
  • run the gui with some of the pcds nalms configs which work exactly the same as far as I can tell.

@KaushikMalapati
Copy link
Author

Maybe these are the only other places that need updates? I looked at all the places where we split by a backslash

alarm_name = alarm_path.split("/")[-1]

item_name = item_path.split("/")[-1]

path_as_list = item_path.split("/")
self.nodes.append(alarm_item)
self.path_to_index[item_path] = len(self.nodes) - 1
if item_name not in self.added_paths:
self.added_paths[item_name] = []
self.added_paths[item_name].append(item_path)
parent_path = "/".join(path_as_list[0:-1])

item_name = item_path.split("/")[-1]

Copy link
Collaborator

@jbellister-slac jbellister-slac left a comment

Choose a reason for hiding this comment

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

This looks good to me!

There's no good way of testing right now without doing it against live data. Mocking out a test environment with docker compose or some such has been something that would be nice to do, but development here is slow enough that it's never been any kind of priority.

This works fine with my pva:// test file, and then I think just passing the pv name into update_model() and remove_item() means the most of the splits in those methods can just go away.

Then path_as_list = item_path.split("/") is all that's left, in which case we can just replace out the protocol before doing the split.

If you'd like to update those it's fine, or I can merge and follow up, good either way.

Thank you!

@KaushikMalapati
Copy link
Author

How do you test this with a file? Do you mean a nalms deployment that has pvs channels?

Let me test it with the new deployment thorsten made. I’ll also look at where I can get rid of splits and path_as_list. Thanks for the feedback

@jbellister-slac
Copy link
Collaborator

How do you test this with a file? Do you mean a nalms deployment that has pvs channels?

Yes, just a local build I have of Phoebus that I can point at the accelerator kafka with an xml file that include a pva:// PV. So basically the same as using the new test deployment from Thorsten.

The changes to the main window file worked just fine. It just shows up multiple times in the tree due to that path_as_list = item_path.split("/") code in alarm_tree_model.

Also I forgot to look at the pydm plugin, but that one I'm happy to take a separate look at in a followup PR.

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.

2 participants