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

Changed Field Fix #82

Merged
merged 6 commits into from
Jul 9, 2024
Merged

Conversation

sveseli
Copy link
Contributor

@sveseli sveseli commented Apr 30, 2024

This PR resolves issue with changed field set in the case where the top level (master) field ("_") is not requested by the client, but the master field callback causes all fields to be marked as updated, rather than only those fields that have actually been modified. The example below shows how the code works with this PR: the PV structure contains 4 fields, with the field 'z' the only one not being updated on the server side, and this is indicated correctly using pvmonitor and printing only fields that changed (after the initial update):

$ pvmonitor -m -r '' -v a
a structure 
    int x 89
    int y 178
    float z 0
    int i 2
a structure 
    int x 69
    int y 138
    int i 3
a structure 
    int x 39
    int y 78
    int i 4

Without this fix, all fields would be marked as updated.

@@ -289,6 +289,7 @@ static void testMasterField(PVRecordPtr const& pvRecord)
cout << "Master PV structure from copy" << endl << *pvMasterField << endl;
cout << "Master PV structure from copy offset " << pvMasterField->getFieldOffset() << endl;
}
testOk1(pvCopy->isMasterFieldRequested());
Copy link
Member

Choose a reason for hiding this comment

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

This only appears to test that PVCopy detects when the master field is requested, showing the changes to PVCopy and pvRecord. How about adding tests for the behavior fix described in the intro to this PR, which appears to have been implemented in MonitorLocal::dataPut()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New set of tests have been added that verify correct behavior of the changed set.

Copy link
Member

@anjohnson anjohnson left a comment

Choose a reason for hiding this comment

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

Thanks.

@anjohnson anjohnson merged commit c070a34 into epics-base:master Jul 9, 2024
15 checks passed
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