mdavidsaver / pvxs

PVA protocol client/server library and utilities.
https://mdavidsaver.github.io/pvxs/
Other
19 stars 25 forks source link

Add pva links to pvxs #57

Closed simon-ess closed 7 months ago

simon-ess commented 9 months ago

Port PVA links from pva2pva.

TODO

AppVeyorBot commented 9 months ago

:x: Build pvxs 1.0.942 failed (commit https://github.com/mdavidsaver/pvxs/commit/7f9f4b70a8 by @simon-ess)

AppVeyorBot commented 9 months ago

:x: Build pvxs 1.0.946 failed (commit https://github.com/mdavidsaver/pvxs/commit/939c51a120 by @simon-ess)

AppVeyorBot commented 9 months ago

:white_check_mark: Build pvxs 1.0.947 completed (commit https://github.com/mdavidsaver/pvxs/commit/c0a961e4da by @simon-ess)

AppVeyorBot commented 9 months ago

:x: Build pvxs 1.0.949 failed (commit https://github.com/mdavidsaver/pvxs/commit/e565a11af6 by @simon-ess)

AppVeyorBot commented 9 months ago

:x: Build pvxs 1.0.951 failed (commit https://github.com/mdavidsaver/pvxs/commit/90447de627 by @simon-ess)

mdavidsaver commented 9 months ago

I have pushed some additional changes. Including a possible workaround for https://github.com/epics-base/epics-base/issues/423 with older Base (currently all released versions...). I have also reworked the test synchronization code. However, I am still seeing some intermittent test failures in GHA runs.

AppVeyorBot commented 9 months ago

:x: Build pvxs 1.0.957 failed (commit https://github.com/mdavidsaver/pvxs/commit/dd5698dd47 by @mdavidsaver)

mdavidsaver commented 8 months ago

Another update. At this point all tests are passing for me. Although, coverage.sh shows there are a couple of cases which I think need testing.

wrt. the db_cancel_event() issue, I think I understand now what was happening. The trigger was a self-cancel, which should not be possible. However, due to an issue with the lifetime, objects bound into lambda functions were no released immediately when the associated subscription was canceled. This should be fixed by 9b099be0d8d548da8ad6072529f5486426c0eac4. I think this will act as a workaround for the Base issue.

The other lingering issue was an intermittent failure in testSevr() of testpvalink. I think I traced this to an issue with the pvaGlobal_t::channels cache. A cache hit which was processed before an additional data update did not call pvaLink::onTypeChange(), so it was using previous Value s.

AppVeyorBot commented 8 months ago

:white_check_mark: Build pvxs 1.0.960 completed (commit https://github.com/mdavidsaver/pvxs/commit/be6492fe26 by @mdavidsaver)

mdavidsaver commented 8 months ago

@simon-ess I think I have this PR is a more or less final form. At this point, the blocker to merging is a resolution of https://github.com/epics-base/epics-base/issues/423, which causes testpvalink to fail.

As a remaining task, I am working on some logic to detect when qsrv.dbd from pva2pva has also been included, and will use this to automatically disable QSRV2 during startup. Of course, then absent QSRV2 would then default to automatically starting in full. (like https://github.com/mdavidsaver/pvxs/pull/58)

AppVeyorBot commented 8 months ago

:x: Build pvxs 1.0.964 failed (commit https://github.com/mdavidsaver/pvxs/commit/5ab45fc0df by @mdavidsaver)

AppVeyorBot commented 7 months ago

:white_check_mark: Build pvxs 1.0.965 completed (commit https://github.com/mdavidsaver/pvxs/commit/ffe5261180 by @mdavidsaver)

george-mcintyre commented 7 months ago

Comments interspersed below.

On 6 Nov 2023, at 17:09, Simon Rose @.***> wrote:

@simon-ess commented on this pull request.

Also, one other oddity I noticed: if you have the following as a db file:

record(ai, "FOO") { field(INP, {pva: {pv: "BAR", proc: "CPP"}}) }

record(aai, "BAR") { field(NELM, 10) } then running pvput BAR "[1,2,3]" with an IOC running softIocPVA the linked ai record fetches the first value. However, with softIocPVX it does not.

In ioc/pvalink_link.cpp https://github.com/mdavidsaver/pvxs/pull/57#discussion_r1383373488:

  • log_debug_printf(_logger, "%s type change V=%c S=%c N=%c S=%c M=%c\n",
  • plink->precord->name,
  • fld_value ? 'Y' : 'N',
  • fld_seconds ? 'Y' : 'N',
  • fld_nanoseconds ? 'Y' : 'N',
  • fld_severity ? 'Y' : 'N',
  • fld_meta ? 'Y' : 'N'); I'm a bit confused by this debug statement: isn't root["value"] the actual value? This debug message seems to be trying to say that the value/timestamp/etc. are being modified, but that's not how I read the code...

In ioc/pvalink_channel.cpp https://github.com/mdavidsaver/pvxs/pull/57#discussion_r1383563635:

  • atomic_lock = ioc::DBManyLock(atomicrecs);
  • atomic_records = std::move(atomic);
  • nonatomic_records = std::move(nonatomic);
  • links_changed = false;
  • }
  • update_seq++;
  • update_evt.signal();
  • log_debug_printf(_logger, "%s Sequence point %u\n", key.first.c_str(), update_seq);
  • }
  • // unlock link
  • if(!atomic_records.empty()) {
  • ioc::DBManyLocker L(atomic_lock); This is perhaps an odd pedantic point, and probably out of scope... but aren't these names semantically backwards? It seems to me that DBManyLock should be doing the action, while DBManyLocker is the thing that does the action.

Not that this affects the code, but it gives me a bit of a hiccup on reading, as it looks like the lock is taken on line 416 (and not obviously ever let go), but it is in fact taken on line 430 (and let go shortly thereafter).

Looking at the Git Blame, it seems that we can blame @george-mcintyre https://github.com/george-mcintyre...

Yeah, like employee and employer, DBManyLock is the actual subject/object - the actual lock object, and DBManyLocker is the think that does the locking. Though I can see how it looks confusing.

— Reply to this email directly, view it on GitHub https://github.com/mdavidsaver/pvxs/pull/57#pullrequestreview-1715243794, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJQ2YXNBLL2AFW6FKIDQTDYDEDTXAVCNFSM6AAAAAA4U5MWA6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJVGI2DGNZZGQ. You are receiving this because you were mentioned.

mdavidsaver commented 7 months ago

Also, one other oddity I noticed ...

You may have meant to set FTVL on the aai record, which despite the name defaults to STRING (imo. a better name would be arrin). So this case is trying to read string[] into DBR_DOUBLE, which is a TODO. Currently string[] can only be read into DBR_STRING.

mdavidsaver commented 7 months ago

Another update. Adds string[] conversions on GET. Also a significant reorganization of the initHooks and atexit sequencing. Now the four sequences (server, single pv, group pv, and pvalink) are combined together into one. I have also extended the dbUnitTest.h helper APIs in pvxs/iochooks.h to (hopefully) allow for external testing w/ QSRV.

At this point, I have no more major TODOs.

mdavidsaver commented 7 months ago

I have found an issue where input links with CPP don't get an initial scan if the associated Channel is already connected and idle. This is annoyingly similar to a previous issue with onTypeChange() (https://github.com/mdavidsaver/pvxs/pull/57#issuecomment-1756704551).

AppVeyorBot commented 7 months ago

:x: Build pvxs 1.0.967 failed (commit https://github.com/mdavidsaver/pvxs/commit/bf8f7fd85f by @mdavidsaver)

simon-ess commented 7 months ago

@george-mcintyre I think that part of my confusion comes from the code in PVXS using the Guard object, but now that I think of it more in that case, Guard is both noun and verb, so maybe I've just been reading it backwards from everyone else the whole time... 🙃

simon-ess commented 7 months ago

Also, one other oddity I noticed ...

You may have meant to set FTVL on the aai record, which despite the name defaults to STRING (imo. a better name would be arrin). So this case is trying to read string[] into DBR_DOUBLE, which is a TODO. Currently string[] can only be read into DBR_STRING.

I think my question is more that I would have expected softIovPVA and softIocPVX to behave the same with the same .db file, but they do not.

mdavidsaver commented 7 months ago

... I would have expected softIovPVA and softIocPVX to behave the same ...

This is the expectation I would like you to have. I was only making a point about why the were different. (this difference should be fixed) Maybe I am the only one who keeps forgetting that aai defaults to STRING...

mdavidsaver commented 7 months ago

... part of my confusion comes from ...

... the established and oft demonstrated fact the I am not as attuned to the subtitles and nuisances of naming ;)

mdavidsaver commented 7 months ago

subtitles and nuisances of naming

Ah, of course I meant nuances...

AppVeyorBot commented 7 months ago

:x: Build pvxs 1.0.972 failed (commit https://github.com/mdavidsaver/pvxs/commit/5548c3f716 by @mdavidsaver)

AppVeyorBot commented 7 months ago

:x: Build pvxs 1.0.976 failed (commit https://github.com/mdavidsaver/pvxs/commit/a35b8cf25e by @mdavidsaver)

mdavidsaver commented 7 months ago

For some reason testqsingle is now failing on all appveyor jobs, but not on the GHA windows jobs.

testqsingle.tap .. 
not ok 89 - Instance leak ServerPvt : 1
not ok 90 - Instance leak ServerSource : 1
not ok 91 - Instance leak StructTop : 1
not ok 92 - Instance leak UDPListener : 1
not ok 93 - Instance leak evbase : 2
not ok 94 - Instance leak evbaseRunning : 2
All 88 subtests passed 

These counts point to a Server being orphaned somehow. The trigger is likely my changing testqsingle to stop and restart the IOC. Why this only manifests on appveyor is at this point a mystery to me.

AppVeyorBot commented 7 months ago

:white_check_mark: Build pvxs 1.0.977 completed (commit https://github.com/mdavidsaver/pvxs/commit/a1d0d2ee59 by @mdavidsaver)

mdavidsaver commented 7 months ago

The (re)initialization issue which was causing leaks and (sometimes) test failures should now be fixed.

mdavidsaver commented 7 months ago

Ok. I think this is good enough. Thanks @simon-ess .