Skip to content

Conversation

sveseli
Copy link
Contributor

@sveseli sveseli commented Jul 25, 2024

This PR updates condition for determining master field added in PR #82. A number of new tests have been added for the use case where PV request specifies individual fields.

@anjohnson
Copy link
Member

Was this a necessary condition that you missed before?

Can you explain why here the master field's name is an empty string, but in #82 it was a single underscore "_"?

@sveseli
Copy link
Contributor Author

sveseli commented Jul 25, 2024

Yes, I simply missed this when testing, and discovered the issue a few days ago. We indicate request for the top level structure (master field) in the request string by "_" as a convention that we decided a while ago when we discussed interface for the initial version of the data distributor plugin, but I do not think that "_" is actually set as the top level record name.

@anjohnson
Copy link
Member

I don't claim to understand how the master field is used, but #82 did refer to it and pvCopy.cpp does look for it in the pvRequest. Please confirm there's no incompatibility between that and the code here (I don't need a detailed explanation, just for you to confirm they do both make sense).

@sveseli
Copy link
Contributor Author

sveseli commented Jul 25, 2024

There should be no incompatibility, and I added tests to make sure the problem I saw was addressed.

@anjohnson
Copy link
Member

@sveseli should this have been merged? Forgotten...

@sveseli
Copy link
Contributor Author

sveseli commented Apr 4, 2025

Yes, it should have been merged. I believe we talked about it.

@anjohnson
Copy link
Member

So I apparently merged #82 last July, so that was included in the EPICS 7.0.9 release, but not this PR. Is that missing code a significant problem for APS at all?

This does need a line in the Release Notes about the change.

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