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

Add a "PVFactory" that supports Tango's protocol #2426

Merged
merged 6 commits into from
Oct 20, 2022

Conversation

endless03
Copy link
Contributor

The PVFactory interface is implemented to support Tango, and complete the read/write test of Tango and VType compatible types(Currently, only scalar is supported). Now, Phoebus can be used as the monitoring and operation interface of Tango.

@kasemir
Copy link
Collaborator

kasemir commented Oct 19, 2022

Well, looks like you didn't have to add too much code, and you navigated the VType handling without too many problems, that's great!

You add a dependency to the tango network library in the core/pv/pom.xml. Note that we'd like to support not only maven but also other build systems. To do that, we collect all dependencies into one lib folder under dependencies/phoebus-target, so we get a well-defined, known, reproducible "target platform", see https://github.com/ControlSystemStudio/phoebus/tree/master/dependencies

In short, please also add the tango network library dependency to the file
https://github.com/ControlSystemStudio/phoebus/blob/master/dependencies/phoebus-target/pom.xml

As for PV names, you introduce tango://att:/devicename/attributename and tango://com:/devicename/com(m)andname.
While that might "work", the names look a bit odd with two :// sections in each name. I'm also not sure if they're valid URIs. We mostly pass the names around as strings and don't strictly enforce them to be URIs, but if they are valid URIs, that might be better in the long run.

Names like these would be a little shorter and valid URIs:
tangoatt:/device/attribute, tangocom:/device/command.
You can implement that by simply registering two PV factories, a TangoAttr_PVFactory for "tangoatt" and a TangoCmd_PVFactory for "tangocom". Or maybe use prefixes "tga" for tango attribute and "tcg" for command, so the prefix length is comparable with "ca", "pva", "sim", ...

@kasemir
Copy link
Collaborator

kasemir commented Oct 19, 2022

The build fails with

.. Could not find artifact org.tango-controls:JTango:jar:9.6.8 in sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots)

Not sure if that's temporary or if the tango lib is not published.

@shroffk
Copy link
Member

shroffk commented Oct 19, 2022

looks like this should be available on maven central... the only thing is that this is listed as a pom...

<dependency>
  <groupId>org.tango-controls</groupId>
  <artifactId>JTango</artifactId>
  <version>10.0.0</version>
  <type>pom</type>
</dependency>

https://search.maven.org/search?q=tango

@endless03
Copy link
Contributor Author

@kasemir I have modified the code according to your comments. The command and attribute implement the PVFactory interface respectively, their prefixes are "tgc" and "tga".
I'm not sure why tango dependency is unavailable. I updated tango version to 9.7.0, which can be used after local testing. I have added dependencies to dependencies/phoebus-target/pom.xml.

Thanks.

@kasemir
Copy link
Collaborator

kasemir commented Oct 20, 2022

While I'm not using Tango and can't say what those who do will prefer, the shorter PV names are likely better.
The build also passes, and you've added the target platform dependency.
@shroffk , do you think the <type>pom</type> is fine, or should that be replaced with those dependencies into that pom which are actually used?

@endless03
Copy link
Contributor Author

@kasemir @shroffk hmm, actually, 9.7.0 is the latest version, not 10.0.0.
图片

@kasemir
Copy link
Collaborator

kasemir commented Oct 20, 2022

Funny. 9.7.0 from May 2022, 10.0.0 from Oct. 2017.
That's why I put the branch and date into the version info of the SNS product, since "version numbers" can be useless.

@shroffk
Copy link
Member

shroffk commented Oct 20, 2022

That is confusing...
But I am glad it works with 9.7.0

Including the pom... which then includes the dependencies to the 3 required bundle should work fine.

@shroffk
Copy link
Member

shroffk commented Oct 20, 2022

Just one thought...
Does it make sense to put everything in one single module... in this core-pv or would it be better to start splitting things out into individual modules.
Since we are using SPI this should be trivial... and might have benefits in terms of managing dependencies (in this case now the pvws project which depends on core-pv will now need tango libraries too.. not a huge issue with might be nice to keep this flexible)

@endless03
Copy link
Contributor Author

@shroffk
I think phoebus as a control system tool, if phoebus can be compatible with more control systems, it will be great !
Thanks.

@shroffk
Copy link
Member

shroffk commented Oct 20, 2022

@endless03
I absolutely agree...
I think it is great to have the tango support included in Phoebus. I am simply suggesting some code/module reorganization.
I would want the default Phoebus product to include core-pv, core-tango, core-mqtt....etc

I also feel that the core-tango module should be part of the same repo too

@endless03
Copy link
Contributor Author

endless03 commented Oct 20, 2022

@shroffk
Sorry, I may not have understood your last comment.

@kasemir
Copy link
Collaborator

kasemir commented Oct 20, 2022

In principle, we could split core-pv-ca, core-pv-pva, core-pv-sim, core-pv-mqtt, core-pv-tango, ... out of core-pv.

That way, you can build a product that talks to sim, ca, pva, mqtt, tango, .. PVs.
But you could also build one that only talks to mqtt, or only tango.

Why would you want to do that? The product would be a little smaller, in case that matters. So far, the different protocols are not in conflict, and you can configure one of them as the default. But if we ever run into issues where one protocol uses the same TCP ports as the other, we would have to include one or the other, not both.

@kasemir
Copy link
Collaborator

kasemir commented Oct 20, 2022

.. but we can do the split later.
Are we for now good with this, using JTango 9.7.0 as the version?
Ready to merge?

@endless03
Copy link
Contributor Author

JTango 9.7.0 can compile and work normally in Phoebus.
I think it would be better if it could be merged.

@kasemir kasemir merged commit db62821 into ControlSystemStudio:master Oct 20, 2022
@kasemir
Copy link
Collaborator

kasemir commented Oct 20, 2022

Alright, merged!

One example for the modular nature of a product: When used with Tango, you want to remove the "PV Tree" from the product. For a PV "X", it expects to also be able to connect to a PV "X.RTYP" to determine the record type, and then based on that it fetches other links. That makes sense for EPICS records, but not Tango devices, so a pure Tango user would simply have no use for it, and it's best removed from the product by simlpy deleting the lib/app-pvtree*jar, or - better - building a site-specific product that doesn't include it.

@endless03
Copy link
Contributor Author

Ok, thanks.

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.

4 participants