search

epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

add new public member updateMasterSetBitSet #53

Closed mrkraimer closed 6 years ago

mrkraimer commented 6 years ago

With this new method PVCopy can be used by the ca provider in pvAccess to better support change and overrun bit sets..

mdavidsaver commented 6 years ago

@mrkraimer Will you be pushing caProvider changes to this branch as well? I'd find this change more compelling if I could see a net reduction in code size.

mrkraimer commented 6 years ago

@mdavidsaver Will you be pushing caProvider changes to this branch as well?

What branch are You taking about.? I would like to just merge this pull request.

mdavidsaver commented 6 years ago

What branch are You taking about.?

Your right, this is was a nonsensical statement.

Will you be creating a pull request against pvAccessCPP which depends on these changes?

mrkraimer commented 6 years ago

mdavidsaver asked. Will you be creating a pull request against pvAccessCPP which depends on these changes?

The answer is yes. Note that pvAccessCPP has an outstanding pull request that I plan to merge tomorrow. After that merge I can push my latest changes in https://github.com/mrkraimer/pvAccessCPP,

Then I can create a new pull request in https://github.com/epics-baser/pvAccessCPP. BUT this will only build if the pull request is merged.

mrkraimer commented 6 years ago

Unless I hear objections I will merge this request later today. I am ready to create a new pull request for changes to the ca provider in pvAccessCPP that need this change to pvCopy.

mdavidsaver commented 6 years ago

Please consider previous comments to be an objection. Do not merge.

On Feb 16, 2018 3:15 AM, "Marty Kraimer" notifications@github.com wrote:

Unless I hear objections I will merge this request later today. I am ready to create a new pull request for changes to the ca provider in pvAccessCPP that need this change to pvCopy.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/epics-base/pvDataCPP/pull/53#issuecomment-366208873, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOVn0sFXXDyoejNjKrUn6P7lp2pq8akks5tVWNagaJpZM4SCvEt .

mrkraimer commented 6 years ago

This pull request does not make any changes except to pvCopy. Since Michael wants to deprecate pvCopy, it sounds like he does not want anything to do with it. Thus I do not understand why he is not willing to have this pull request merged.

pvCopy is used to implement the local provider in pvDatabase.

With the new public method, it is now also used in https://github.com/mrkraimer/pvAccessCPP for the CAProvider. Provider ca now shows the client with fields have changed value for both get and monitor.

Until this pull request is merged I can not issue a pull request in https://github.com/epics-base/pvAccessCPP.

So again I request that this pull request be merged.

mdavidsaver commented 6 years ago

Until this pull request is merged I can not issue a pull request in

Why not? This is exactly what I want you to do before I will consider merging this PR.

mdavidsaver commented 6 years ago

To be clear. My insistence on a second PR is largely separate from whether or not pvCopy will be deprecated (my mind isn't made up). Rather, it is my dislike of "reviewing" changes piecemeal.

mrkraimer commented 6 years ago

I issued the pull request in pvAccessCPP

mrkraimer commented 6 years ago

So how long until this pull request can be merged? Note that just as expected pull request #91 in pvAccessCPP has failed with

../caChannel.cpp: In member function ‘void epics::pvAccess::ca::CAChannelMonitor::subscriptionEvent(event_handler_args&)’: ../caChannel.cpp:1949:21: error: ‘class epics::pvData::PVCopy’ has no member named ‘updateMasterSetBitSet’ pvCopy->updateMasterSetBitSet(pvStructure,activeElement->changedBitSet);

mdavidsaver commented 6 years ago

@mrkraimer Please have a look at my question on epics-base/pvAccessCPP#91 (https://github.com/epics-base/pvAccessCPP/pull/91#discussion_r169437656).

Is there more work to be done here? Why is the provided pvRequest variable not passed to PVCopy::create()? I would naively assume that it should be.

Before I consider merging this PR I'd like to understand why updateMasterSetBitSet() is needed, why the use case isn't handled by the existing API. I'm also curious why this second usage of pvCopy (in caProvider) needs a new method, but apparently won't use any of the existing methods? For this reason I'm waiting for your answer on whether epics-base/pvAccessCPP#91 is complete.

If/when this is all settled, then I'll of course be insisting on some unittest coverage.

mdavidsaver commented 6 years ago

Since Michael wants to deprecate pvCopy, it sounds like he does not want anything to do with it. Thus I do not understand why he is not willing to have this pull request merged.

pvCopy is used to implement the local provider in pvDatabase.

If this PR were for an implementation detail, or an otherwise internal API, then I wouldn't care. However, pvCopy is at present a public API. This is why I would like to move it out of this repo. If it could be make into such a detail of pvDatabaseCPP and/or pvAccessCPP, then we wouldn't have to fight about it!

mrkraimer commented 6 years ago

https://github.com/epics-base/pvAccessCPP/pull/91 Is not merged because, until this pull request is megged, the travis build fails. I have already pointed this out more than once. Not also that the new method is required in order to use pvCopy on the client side.

mdavidsaver commented 6 years ago

Before I consider merging this PR ... I'm waiting for your answer on whether epics-base/pvAccessCPP#91 is complete.

@mrkraimer Is this statement unclear to you? Please answer the questions I've posed on the thread for epics-base/pvAccessCPP#91.

mdavidsaver commented 6 years ago

From https://github.com/epics-base/pvAccessCPP/pull/91#discussion_r169437656

Is there more work to be done here? Why is the provided pvRequest variable not passed to PVCopy::create()? I would naively assume that it should be.

ralphlange commented 6 years ago

I would have thought both of you are too old for this Kindergarden style discussion, but I'm getting doubts.

mdavidsaver commented 6 years ago

sigh... you're right to have doubts. This is clearly not a productive discussion. I guess we'll try to break the impasse on Tuesday.

mrkraimer commented 6 years ago

At today's hangout I agreed to generate a brief description of pvCopy and its public methods. But this is already done via doxygen comments that appear in pvCopy.h

I also agreed to add a test for the new method updateMasterSetBitSet. I have done this but have not pushed it to mrkraimer/pvDataCPP. I will do this tomorrow.

mrkraimer commented 6 years ago

It has been two weeks since I sent my last message and added a test for new method. I do not understand why Michael is still not willing to have this pull request merged.

mdavidsaver commented 6 years ago

I do not understand why Michael is still not willing to have this pull request merged.

I'm sorry Marty, but we're going to have to talk about the design.

For the audience. As I've look into it I've developed a suspicion that these methods are being added as a workaround in lieu of a more thorough reworking of caProvider. Specifically, refineStructure() in caChannel.cpp looks like it does some of the same work as PVCopy::create() in iterating the 'fields' sub-structure of the pvRequest. I would like to see this apparent redundancy eliminated.

mrkraimer commented 6 years ago

I have spent time looking at how the ca provider in pvAccessCPP converts between DBD and pvData. I now see a better way to do the conversion that will not use pvCopy. Thus I am canceling this pull request.