Merge ~dirk.zimoch/epics-base:named-events-backward-compatibility into ~epics-core/epics-base/+git/epics-base:3.15
- Git
- lp:~dirk.zimoch/epics-base
- named-events-backward-compatibility
- Merge into 3.15
Status: | Merged |
---|---|
Approved by: | Andrew Johnson |
Approved revision: | 2b4a9632b7e21a60b80e1454e44db41d8128ad06 |
Merged at revision: | a2ae07dfcdaf172dbef1db692714dfbfb0e3d5ec |
Proposed branch: | ~dirk.zimoch/epics-base:named-events-backward-compatibility |
Merge into: | ~epics-core/epics-base/+git/epics-base:3.15 |
Diff against target: |
400 lines (+225/-40) 7 files modified
documentation/RELEASE_NOTES.html (+8/-0) src/ioc/db/dbScan.c (+47/-39) src/ioc/db/dbScan.h (+1/-1) src/std/rec/test/Makefile (+9/-0) src/std/rec/test/epicsRunRecordTests.c (+2/-0) src/std/rec/test/scanEventTest.c (+142/-0) src/std/rec/test/scanEventTest.db (+16/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ralph Lange | Approve | ||
Andrew Johnson | Approve | ||
mdavidsaver | Needs Fixing | ||
Review via email: mp+337850@code.launchpad.net |
This proposal supersedes a proposal from 2018-02-13.
Commit message
Description of the change
Make named soft events backward compatible to calculated numeric events (e.g by calc records).
Every event name that represents a number and that rounds down to an integer 1 ... 255 is a numeric events. If it rounds to 0 or is not finite (nan, +/-inf) it is no event.
Andrew Johnson (anj) wrote : Posted in a previous version of this proposal | # |
mdavidsaver (mdavidsaver) wrote : Posted in a previous version of this proposal | # |
I haven't had a chance to do any run tests, but from reading the code it looks reasonable.
The only (minor) issue I see is that the test code for eventRecord needs to be moved under src/std/rec/test/
Dirk Zimoch (dirk.zimoch) wrote : Posted in a previous version of this proposal | # |
Oh, I didn‘t know that.
Dirk
> Am 16.02.2018 um 17:17 schrieb Andrew Johnson <email address hidden>:
>
> Hi Dirk,
>
> You can commit to a branch after you have submitted a Merge Request for it and the new commits will automatically appear in the Merge Request (check this one out, it has your b2d6b67 commit in it). Submitting a *new* MR implies there's something major that is different than before, maybe you rebased the branch. Please don't do resubmits for minor additional changes like this, they are unnecessary and send confusing signals to the reviewers.
>
> I will take a look at this when I get a chance.
>
> Thanks,
>
> - Andrew
> --
> https:/
> You are the owner of ~dirk.zimoch/
Dirk Zimoch (dirk.zimoch) wrote : Posted in a previous version of this proposal | # |
I had put it in db/test because my intent was to test the db event code,
not to test the records. The records are just the tool to test that the
db code works as intended. But if there is a problem having records in
db/test I see no problem moving it.
Dirk
On 16.02.2018 17:38, mdavidsaver wrote:
> I haven't had a chance to do any run tests, but from reading the code it looks reasonable.
>
> The only (minor) issue I see is that the test code for eventRecord needs to be moved under src/std/rec/test/
>
> Diff comments:
>
>> diff --git a/src/ioc/
>> index 215545f..77e485a 100644
>> --- a/src/ioc/
>> +++ b/src/ioc/
>> @@ -138,6 +138,21 @@ testHarness_SRCS += dbPutGetTest.db
>> TESTFILES += ../dbPutGetTest.db
>> TESTS += testPutGetTest
>>
>> +TARGETS += $(COMMON_
>> +DBDDEPENDS_FILES += scanEventTest.
>> +scanEventTest_DBD += menuGlobal.dbd
>> +scanEventTest_DBD += menuConvert.dbd
>> +scanEventTest_DBD += menuScan.dbd
>> +scanEventTest_DBD += eventRecord.dbd
>> +scanEventTest_DBD += devEventSoft.dbd
>> +scanEventTest_DBD += calcRecord.dbd
>> +scanEventTest_DBD += dfanoutRecord.dbd
>> +TESTPROD_HOST += scanEventTest
>> +scanEventTest_SRCS += scanEventTest.c
>> +scanEventTest_SRCS += scanEventTest_
>> +scanEventTest_LIBS = dbRecStd
>> +TESTS += scanEventTest
>
> Tests using libdbRecStd and base.dbd need to be under src/std/rec/test/ (cf. compressTest.c). So the test code needs to move.
>
>> +
>> # This runs all the test programs in a known working order:
>> testHarness_SRCS += epicsRunDbTests.c
>>
>
>
mdavidsaver (mdavidsaver) wrote : | # |
> if there is a problem having records in db/test
cf src/Makefile. ioc/db/test can be build before libdbRecStd (containing record support) is linked, which will fail. Try "make distclean" then "make -C src ioc/db/test". Testing with parallel make will also catch issues like this.
> DIRS += ioc
> ioc_DEPEND_DIRS = libCom ca/client
>
> DIRS += ioc/db/test
> ioc/db/
>
> DIRS += std
> std_DEPEND_DIRS = ioc libCom/RTEMS
>
> DIRS += std/rec/test
> std/rec/
Dirk Zimoch (dirk.zimoch) wrote : | # |
I see...
Dirk Zimoch (dirk.zimoch) wrote : | # |
> Try "make distclean" then "make -C src ioc/db/test".
Do you mean "make -C src/ioc/db/test" ?
$ make -C src std/rec/test/
make: Entering directory `[my top dir]/epics-
make: Nothing to be done for `std/rec/test/'.
make: Leaving directory `[my top dir]/epics-
$ make -C src/ioc/db/test/
make: Entering directory `[my top dir]/epics-
perl -CSD [my top dir]/epics-
Can't open perl script "[my top dir]/epics-
So it seems this does not work anyway.. ???
Dirk Zimoch (dirk.zimoch) wrote : | # |
I have moved the test.
Andrew Johnson (anj) wrote : | # |
The new Release Notes entry should go at the top of the file (where it says "Insert new items immediately below here ..."). I don't think it needs to be quite so explicit about the new algorithm, a brief description in words would suffice (I won't insist you change the text if you don't want to, but see the question below). It should mention that the post_event() routine has been undeprecated again since there may be quite a lot of older software that still calls this entry-point so we don't intend to remove it.
Looking at your event number compatibility logic, if I've understood the code properly it seems that I can legally create an event named "-1" or "256", although neither could be triggered by post_event(), but the names "Inf" and "Nan" are seen as illegal and will be rejected. That doesn't seem terribly logical to me, why couldn't they be allowed as purely string names?
I haven't tried pulling the code yet, I'm assuming it builds and works as promised (I will check that on Linux at least when merging).
Dirk Zimoch (dirk.zimoch) wrote : | # |
> Am 02.03.2018 um 21:31 schrieb Andrew Johnson <email address hidden>:
>
> The new Release Notes entry should go at the top of the file (where it says "Insert new items immediately below here ..."). I don't think it needs to be quite so explicit about the new algorithm, a brief description in words would suffice (I won't insist you change the text if you don't want to, but see the question below). It should mention that the post_event() routine has been undeprecated again since there may be quite a lot of older software that still calls this entry-point so we don't intend to remove it.
I‘ll change it. (On Monday)
>
> Looking at your event number compatibility logic, if I've understood the code properly it seems that I can legally create an event named "-1" or "256", although neither could be triggered by post_event(), but the names "Inf" and "Nan" are seen as illegal and will be rejected. That doesn't seem terribly logical to me, why couldn't they be allowed as purely string names?
Ralph suggested that.
>
> I haven't tried pulling the code yet, I'm assuming it builds and works as promised (I will check that on Linux at least when merging).
> --
> https:/
> You are the owner of ~dirk.zimoch/
Ralph Lange (ralph-lange) wrote : | # |
Clarification.
> > Am 02.03.2018 um 21:31 schrieb Andrew Johnson <email address hidden>:
> >
> > Looking at your event number compatibility logic, if I've understood the
> code properly it seems that I can legally create an event named "-1" or "256",
> although neither could be triggered by post_event(), but the names "Inf" and
> "Nan" are seen as illegal and will be rejected. That doesn't seem terribly
> logical to me, why couldn't they be allowed as purely string names?
>
> Ralph suggested that.
Sorry - I must have been not very clear...
The issue - as I understood it - was implementing a mapping from double numbers (use case was a calc record) to db event names.
Dirk mentioned which double numbers he would see as illegal and not map to a valid db event name, and I suggested (or wanted to) that double numbers that are not finite (negative and positive infinity and the not-a-number value) should also not map to a db event name.
Given that the original numeric db event codes were [1-255], I don't see where infinite values would map to.
Dirk Zimoch (dirk.zimoch) wrote : | # |
> Am 03.03.2018 um 17:21 schrieb Ralph Lange <email address hidden>:
>
> Clarification.
>
>>> Am 02.03.2018 um 21:31 schrieb Andrew Johnson <email address hidden>:
>>>
>>> Looking at your event number compatibility logic, if I've understood the
>> code properly it seems that I can legally create an event named "-1" or "256",
>> although neither could be triggered by post_event(), but the names "Inf" and
>> "Nan" are seen as illegal and will be rejected. That doesn't seem terribly
>> logical to me, why couldn't they be allowed as purely string names?
>>
>> Ralph suggested that.
>
> Sorry - I must have been not very clear...
>
> The issue - as I understood it - was implementing a mapping from double numbers (use case was a calc record) to db event names.
>
> Dirk mentioned which double numbers he would see as illegal and not map to a valid db event name, and I suggested (or wanted to) that double numbers that are not finite (negative and positive infinity and the not-a-number value) should also not map to a db event name.
> Given that the original numeric db event codes were [1-255], I don't see where infinite values would map to.
>
Hmmm... As this fix is about backward compatibility with numeric events 0-255, non-finite values do not really need special treatment. If an event „Inf“ is posted but no record waits for it, nothing happens. But if there is a record waiting for „Inf“ events, we have an even cooler feature. The original purpose of the fix was to trigger event 2 with 2.000000, nothing more. Maybe I simply remove the non-finite handling. But then with the same reasoning 0 needs no special treatment as „no event“. Simply have no records on event „0“. — Or have some if you want. Why not? But maybe do not trigger them with post_event()?
What about spaces? It probably causes more confusion than benefit to distinguish event „ “ from „ “. At the moment leading and trailing blanks are removed. So both are equal to the empty string, which is „no event“ for good reason.
What about other numbers as suggested recently? I am not really fond of the idea to extend the post_event() functionality.
BTW: The whole problem started with the double to string conversion using fixed 6 decimals instead of .PREC. But to change that may cause even more incompatibilities.
How backward compatible do we want/need to be?
> --
> https:/
> You are the owner of ~dirk.zimoch/
Dirk Zimoch (dirk.zimoch) wrote : | # |
On 03.03.2018 17:21, Ralph Lange wrote:
> Given that the original numeric db event codes were [1-255], I don't see where infinite values would map to.
Infinite values would not map to numeric events and thus not be
available to post_event() anyway. But as named events they would be
available as to postEvent().
I re-enable them now.
Before, 0 was no event, because it was the default value for EVNT
fields. Now the default value is the empty string. Thus I will make "0"
a valid event.
But let's agree on some questions first:
* Should "0" be a different event from "0.000000" like now "256" is
different from "256.000000"?
* Or should any (short, long, signed, unsigned ?) number map to an
integer, but only 1...255 would be available to post_event().
* Or should even more numbers be available to post_event()?
* Is is correct to round down any such number (like 2.999999) to integer
(that would be the full backward compatibility), or should only actual
integer numbers like "2.000000" be treated this way (what I actually
expect calculated event numbers to look like)?
Dirk
Ralph Lange (ralph-lange) wrote : | # |
Uh-oh.
As for my original concern, I misunderstood how this was going to be implemented.
I'm fine with mapping to appropriate strings that would be ignored (no records waiting) by default and could be used in an exception handling fashion if desired.
As for the numeric conversions, I would try to be reasonable and minimally invasive, e.g.
If and only if number is in the interval [1...256[ remove the fractional part.
Convert in the %f-variant of the %g style: don't show insignificant zeroes to the right of the decimal point, and don't include the decimal point on whole numbers.
Removing trailing and leading whitespace is fine.
Dirk Zimoch (dirk.zimoch) wrote : | # |
My original fix was supposed to be minimally invasive: an integer 0-255 followed by ".000000" was treated as a numeric event, with 0 being no event. Then change requests came in.
> Convert in the %f-variant of the %g style: don't show insignificant zeroes to the right of the decimal point, and don't include the decimal point on whole numbers.
I tested three cases for the conversion from a calc[out] with VAL=2.0:
1. event.INP -> calcout.VAL (or calc)
This is no problem. calcout.PREC is used to convert the value, so I get "2" with the default PREC=0.
2. event.INP -> calcout.VAL CP
This uses precision=6! I get "2.000000"
I find this a bit strange because caget -d0 gives me "2".
3. calcout.OUT -> event.VAL
This uses precision=6 as well. I get "2.000000".
Maybe the problem should really be fixed in the conversion double->string. But this may break other stuff. I will investigate a bit...
Dirk Zimoch (dirk.zimoch) wrote : | # |
Giving the event record a get_precision() function that sets precision=0 helps with case 3. But does that make sense? When converting a double to a string, the conversion should use PREC of the source record, not of the target record.
Andrew Johnson (anj) wrote : | # |
This now looks reasonable to me (I haven't built or tested it yet though).
Ralph have Dirk's latest changes resolved your concerns?
Preview Diff
1 | diff --git a/documentation/RELEASE_NOTES.html b/documentation/RELEASE_NOTES.html | |||
2 | index 33f12c4..d19664e 100644 | |||
3 | --- a/documentation/RELEASE_NOTES.html | |||
4 | +++ b/documentation/RELEASE_NOTES.html | |||
5 | @@ -16,6 +16,14 @@ | |||
6 | 16 | 16 | ||
7 | 17 | <!-- Insert new items immediately below here ... --> | 17 | <!-- Insert new items immediately below here ... --> |
8 | 18 | 18 | ||
9 | 19 | <h3>Fix problem with numeric soft events</h3> | ||
10 | 20 | <p>Changing from numeric to named soft events introduced an incompatibility | ||
11 | 21 | when a numeric event 1-255 is converted from a DOUBLE, e.g. from a calc record. | ||
12 | 22 | The <tt>post_event()</tt> API is not marked deprecated any more. | ||
13 | 23 | |||
14 | 24 | <p>Also <code>scanpel</code> has been modified to accept a glob pattern for | ||
15 | 25 | event name filtering and to show events with no connected records as well.</p> | ||
16 | 26 | |||
17 | 19 | <h3>Add osiSockOptMcastLoop_t and osiSockTest</h3> | 27 | <h3>Add osiSockOptMcastLoop_t and osiSockTest</h3> |
18 | 20 | 28 | ||
19 | 21 | <p>Added a new OS-independent typedef for multicast socket options, and a test | 29 | <p>Added a new OS-independent typedef for multicast socket options, and a test |
20 | diff --git a/src/ioc/db/dbScan.c b/src/ioc/db/dbScan.c | |||
21 | index e0c48d4..e9dc178 100644 | |||
22 | --- a/src/ioc/db/dbScan.c | |||
23 | +++ b/src/ioc/db/dbScan.c | |||
24 | @@ -111,8 +111,8 @@ static char *priorityName[NUM_CALLBACK_PRIORITIES] = { | |||
25 | 111 | typedef struct event_list { | 111 | typedef struct event_list { |
26 | 112 | CALLBACK callback[NUM_CALLBACK_PRIORITIES]; | 112 | CALLBACK callback[NUM_CALLBACK_PRIORITIES]; |
27 | 113 | scan_list scan_list[NUM_CALLBACK_PRIORITIES]; | 113 | scan_list scan_list[NUM_CALLBACK_PRIORITIES]; |
30 | 114 | struct event_list *next; | 114 | struct event_list *next; |
31 | 115 | char event_name[MAX_STRING_SIZE]; | 115 | char eventname[1]; /* actually arbitrary size */ |
32 | 116 | } event_list; | 116 | } event_list; |
33 | 117 | static event_list * volatile pevent_list[256]; | 117 | static event_list * volatile pevent_list[256]; |
34 | 118 | static epicsMutexId event_lock; | 118 | static epicsMutexId event_lock; |
35 | @@ -249,11 +249,6 @@ void scanAdd(struct dbCommon *precord) | |||
36 | 249 | event_list *pel; | 249 | event_list *pel; |
37 | 250 | 250 | ||
38 | 251 | eventname = precord->evnt; | 251 | eventname = precord->evnt; |
39 | 252 | if (strlen(eventname) >= MAX_STRING_SIZE) { | ||
40 | 253 | recGblRecordError(S_db_badField, (void *)precord, | ||
41 | 254 | "scanAdd: too long EVNT value"); | ||
42 | 255 | return; | ||
43 | 256 | } | ||
44 | 257 | prio = precord->prio; | 252 | prio = precord->prio; |
45 | 258 | if (prio < 0 || prio >= NUM_CALLBACK_PRIORITIES) { | 253 | if (prio < 0 || prio >= NUM_CALLBACK_PRIORITIES) { |
46 | 259 | recGblRecordError(-1, (void *)precord, | 254 | recGblRecordError(-1, (void *)precord, |
47 | @@ -317,24 +312,17 @@ void scanDelete(struct dbCommon *precord) | |||
48 | 317 | recGblRecordError(-1, (void *)precord, | 312 | recGblRecordError(-1, (void *)precord, |
49 | 318 | "scanDelete detected illegal SCAN value"); | 313 | "scanDelete detected illegal SCAN value"); |
50 | 319 | } else if (scan == menuScanEvent) { | 314 | } else if (scan == menuScanEvent) { |
51 | 320 | char* eventname; | ||
52 | 321 | int prio; | 315 | int prio; |
53 | 322 | event_list *pel; | 316 | event_list *pel; |
54 | 323 | scan_list *psl = 0; | 317 | scan_list *psl = 0; |
55 | 324 | 318 | ||
56 | 325 | eventname = precord->evnt; | ||
57 | 326 | prio = precord->prio; | 319 | prio = precord->prio; |
58 | 327 | if (prio < 0 || prio >= NUM_CALLBACK_PRIORITIES) { | 320 | if (prio < 0 || prio >= NUM_CALLBACK_PRIORITIES) { |
59 | 328 | recGblRecordError(-1, (void *)precord, | 321 | recGblRecordError(-1, (void *)precord, |
60 | 329 | "scanDelete detected illegal PRIO field"); | 322 | "scanDelete detected illegal PRIO field"); |
61 | 330 | return; | 323 | return; |
62 | 331 | } | 324 | } |
69 | 332 | do /* multithreading: make sure pel is consistent */ | 325 | pel = eventNameToHandle(precord->evnt); |
64 | 333 | pel = pevent_list[0]; | ||
65 | 334 | while (pel != pevent_list[0]); | ||
66 | 335 | for (; pel; pel=pel->next) { | ||
67 | 336 | if (strcmp(pel->event_name, eventname) == 0) break; | ||
68 | 337 | } | ||
70 | 338 | if (pel && (psl = &pel->scan_list[prio])) | 326 | if (pel && (psl = &pel->scan_list[prio])) |
71 | 339 | deleteFromList(precord, psl); | 327 | deleteFromList(precord, psl); |
72 | 340 | } else if (scan == menuScanI_O_Intr) { | 328 | } else if (scan == menuScanI_O_Intr) { |
73 | @@ -422,14 +410,12 @@ int scanpel(const char* eventname) /* print event list */ | |||
74 | 422 | int prio; | 410 | int prio; |
75 | 423 | event_list *pel; | 411 | event_list *pel; |
76 | 424 | 412 | ||
82 | 425 | do /* multithreading: make sure pel is consistent */ | 413 | for (pel = pevent_list[0]; pel; pel = pel->next) { |
83 | 426 | pel = pevent_list[0]; | 414 | if (!eventname || epicsStrGlobMatch(pel->eventname, eventname)) { |
84 | 427 | while (pel != pevent_list[0]); | 415 | printf("Event \"%s\"\n", pel->eventname); |
80 | 428 | for (; pel; pel = pel->next) { | ||
81 | 429 | if (!eventname || strcmp(pel->event_name, eventname) == 0) { | ||
85 | 430 | for (prio = 0; prio < NUM_CALLBACK_PRIORITIES; prio++) { | 416 | for (prio = 0; prio < NUM_CALLBACK_PRIORITIES; prio++) { |
86 | 431 | if (ellCount(&pel->scan_list[prio].list) == 0) continue; | 417 | if (ellCount(&pel->scan_list[prio].list) == 0) continue; |
88 | 432 | sprintf(message, "Event \"%s\" Priority %s", pel->event_name, priorityName[prio]); | 418 | sprintf(message, " Priority %s", priorityName[prio]); |
89 | 433 | printList(&pel->scan_list[prio], message); | 419 | printList(&pel->scan_list[prio], message); |
90 | 434 | } | 420 | } |
91 | 435 | } | 421 | } |
92 | @@ -480,18 +466,51 @@ event_list *eventNameToHandle(const char *eventname) | |||
93 | 480 | int prio; | 466 | int prio; |
94 | 481 | event_list *pel; | 467 | event_list *pel; |
95 | 482 | static epicsThreadOnceId onceId = EPICS_THREAD_ONCE_INIT; | 468 | static epicsThreadOnceId onceId = EPICS_THREAD_ONCE_INIT; |
99 | 483 | 469 | double eventnumber = 0; | |
100 | 484 | if (!eventname || eventname[0] == 0) | 470 | size_t namelength; |
101 | 485 | return NULL; | 471 | |
102 | 472 | if (!eventname) return NULL; | ||
103 | 473 | while (isspace((unsigned char)eventname[0])) eventname++; | ||
104 | 474 | if (!eventname[0]) return NULL; | ||
105 | 475 | namelength = strlen(eventname); | ||
106 | 476 | while (isspace((unsigned char)eventname[namelength-1])) namelength--; | ||
107 | 477 | |||
108 | 478 | /* Backward compatibility with numeric events: | ||
109 | 479 | Treat any string that represents a double with an | ||
110 | 480 | integer part between 0 and 255 the same as the integer | ||
111 | 481 | because it is most probably a conversion from double | ||
112 | 482 | like from a calc record. | ||
113 | 483 | */ | ||
114 | 484 | if (epicsParseDouble(eventname, &eventnumber, NULL) == 0) | ||
115 | 485 | { | ||
116 | 486 | if (eventnumber >= 0 && eventnumber < 256) | ||
117 | 487 | { | ||
118 | 488 | if (eventnumber < 1) | ||
119 | 489 | return NULL; /* 0 is no event */ | ||
120 | 490 | if ((pel = pevent_list[(int)eventnumber]) != NULL) | ||
121 | 491 | return pel; | ||
122 | 492 | } | ||
123 | 493 | else | ||
124 | 494 | eventnumber = 0; /* not a numeric event between 1 and 255 */ | ||
125 | 495 | } | ||
126 | 486 | 496 | ||
127 | 487 | epicsThreadOnce(&onceId, eventOnce, NULL); | 497 | epicsThreadOnce(&onceId, eventOnce, NULL); |
128 | 488 | epicsMutexMustLock(event_lock); | 498 | epicsMutexMustLock(event_lock); |
129 | 489 | for (pel = pevent_list[0]; pel; pel=pel->next) { | 499 | for (pel = pevent_list[0]; pel; pel=pel->next) { |
131 | 490 | if (strcmp(pel->event_name, eventname) == 0) break; | 500 | if (strncmp(pel->eventname, eventname, namelength) == 0 |
132 | 501 | && pel->eventname[namelength] == 0) | ||
133 | 502 | break; | ||
134 | 491 | } | 503 | } |
135 | 492 | if (pel == NULL) { | 504 | if (pel == NULL) { |
138 | 493 | pel = dbCalloc(1, sizeof(event_list)); | 505 | pel = dbCalloc(1, sizeof(event_list) + namelength); |
139 | 494 | strcpy(pel->event_name, eventname); | 506 | if (eventnumber > 0) |
140 | 507 | { | ||
141 | 508 | /* backward compatibility: make all numeric events look like integers */ | ||
142 | 509 | sprintf(pel->eventname, "%i", (int)eventnumber); | ||
143 | 510 | pevent_list[(int)eventnumber] = pel; | ||
144 | 511 | } | ||
145 | 512 | else | ||
146 | 513 | strncpy(pel->eventname, eventname, namelength); | ||
147 | 495 | for (prio = 0; prio < NUM_CALLBACK_PRIORITIES; prio++) { | 514 | for (prio = 0; prio < NUM_CALLBACK_PRIORITIES; prio++) { |
148 | 496 | callbackSetUser(&pel->scan_list[prio], &pel->callback[prio]); | 515 | callbackSetUser(&pel->scan_list[prio], &pel->callback[prio]); |
149 | 497 | callbackSetPriority(prio, &pel->callback[prio]); | 516 | callbackSetPriority(prio, &pel->callback[prio]); |
150 | @@ -501,12 +520,6 @@ event_list *eventNameToHandle(const char *eventname) | |||
151 | 501 | } | 520 | } |
152 | 502 | pel->next=pevent_list[0]; | 521 | pel->next=pevent_list[0]; |
153 | 503 | pevent_list[0]=pel; | 522 | pevent_list[0]=pel; |
154 | 504 | { /* backward compatibility */ | ||
155 | 505 | char* p; | ||
156 | 506 | long e = strtol(eventname, &p, 0); | ||
157 | 507 | if (*p == 0 && e > 0 && e <= 255) | ||
158 | 508 | pevent_list[e] = pel; | ||
159 | 509 | } | ||
160 | 510 | } | 523 | } |
161 | 511 | epicsMutexUnlock(event_lock); | 524 | epicsMutexUnlock(event_lock); |
162 | 512 | return pel; | 525 | return pel; |
163 | @@ -527,13 +540,8 @@ void postEvent(event_list *pel) | |||
164 | 527 | /* backward compatibility */ | 540 | /* backward compatibility */ |
165 | 528 | void post_event(int event) | 541 | void post_event(int event) |
166 | 529 | { | 542 | { |
167 | 530 | event_list* pel; | ||
168 | 531 | |||
169 | 532 | if (event <= 0 || event > 255) return; | 543 | if (event <= 0 || event > 255) return; |
174 | 533 | do { /* multithreading: make sure pel is consistent */ | 544 | postEvent(pevent_list[event]); |
171 | 534 | pel = pevent_list[event]; | ||
172 | 535 | } while (pel != pevent_list[event]); | ||
173 | 536 | postEvent(pel); | ||
175 | 537 | } | 545 | } |
176 | 538 | 546 | ||
177 | 539 | 547 | ||
178 | 540 | static void ioscanOnce(void *arg) | 548 | static void ioscanOnce(void *arg) |
179 | diff --git a/src/ioc/db/dbScan.h b/src/ioc/db/dbScan.h | |||
180 | index 28d2e13..d310271 100644 | |||
181 | --- a/src/ioc/db/dbScan.h | |||
182 | +++ b/src/ioc/db/dbScan.h | |||
183 | @@ -50,7 +50,7 @@ epicsShareFunc void scanCleanup(void); | |||
184 | 50 | 50 | ||
185 | 51 | epicsShareFunc EVENTPVT eventNameToHandle(const char* event); | 51 | epicsShareFunc EVENTPVT eventNameToHandle(const char* event); |
186 | 52 | epicsShareFunc void postEvent(EVENTPVT epvt); | 52 | epicsShareFunc void postEvent(EVENTPVT epvt); |
188 | 53 | epicsShareFunc void post_event(int event) EPICS_DEPRECATED; | 53 | epicsShareFunc void post_event(int event); |
189 | 54 | epicsShareFunc void scanAdd(struct dbCommon *); | 54 | epicsShareFunc void scanAdd(struct dbCommon *); |
190 | 55 | epicsShareFunc void scanDelete(struct dbCommon *); | 55 | epicsShareFunc void scanDelete(struct dbCommon *); |
191 | 56 | epicsShareFunc double scanPeriod(int scan); | 56 | epicsShareFunc double scanPeriod(int scan); |
192 | diff --git a/src/std/rec/test/Makefile b/src/std/rec/test/Makefile | |||
193 | index 5322c8e..70b6e17 100644 | |||
194 | --- a/src/std/rec/test/Makefile | |||
195 | +++ b/src/std/rec/test/Makefile | |||
196 | @@ -37,6 +37,15 @@ testHarness_SRCS += analogMonitorTest_registerRecordDeviceDriver.cpp | |||
197 | 37 | TESTFILES += $(COMMON_DIR)/analogMonitorTest.dbd ../analogMonitorTest.db | 37 | TESTFILES += $(COMMON_DIR)/analogMonitorTest.dbd ../analogMonitorTest.db |
198 | 38 | TESTS += analogMonitorTest | 38 | TESTS += analogMonitorTest |
199 | 39 | 39 | ||
200 | 40 | TARGETS += $(COMMON_DIR)/scanEventTest.dbd | ||
201 | 41 | DBDDEPENDS_FILES += scanEventTest.dbd$(DEP) | ||
202 | 42 | scanEventTest_DBD += base.dbd | ||
203 | 43 | TESTPROD_HOST += scanEventTest | ||
204 | 44 | scanEventTest_SRCS += scanEventTest.c | ||
205 | 45 | scanEventTest_SRCS += scanEventTest_registerRecordDeviceDriver.cpp | ||
206 | 46 | TESTFILES += $(COMMON_DIR)/scanEventTest.dbd ../scanEventTest.db | ||
207 | 47 | TESTS += scanEventTest | ||
208 | 48 | |||
209 | 40 | TARGETS += $(COMMON_DIR)/regressTest.dbd | 49 | TARGETS += $(COMMON_DIR)/regressTest.dbd |
210 | 41 | DBDDEPENDS_FILES += regressTest.dbd$(DEP) | 50 | DBDDEPENDS_FILES += regressTest.dbd$(DEP) |
211 | 42 | regressTest_DBD += base.dbd | 51 | regressTest_DBD += base.dbd |
212 | diff --git a/src/std/rec/test/epicsRunRecordTests.c b/src/std/rec/test/epicsRunRecordTests.c | |||
213 | index 092c0a6..1a7133c 100644 | |||
214 | --- a/src/std/rec/test/epicsRunRecordTests.c | |||
215 | +++ b/src/std/rec/test/epicsRunRecordTests.c | |||
216 | @@ -14,6 +14,7 @@ | |||
217 | 14 | 14 | ||
218 | 15 | int analogMonitorTest(void); | 15 | int analogMonitorTest(void); |
219 | 16 | int arrayOpTest(void); | 16 | int arrayOpTest(void); |
220 | 17 | int scanEventTest(void); | ||
221 | 17 | 18 | ||
222 | 18 | void epicsRunRecordTests(void) | 19 | void epicsRunRecordTests(void) |
223 | 19 | { | 20 | { |
224 | @@ -22,6 +23,7 @@ void epicsRunRecordTests(void) | |||
225 | 22 | runTest(analogMonitorTest); | 23 | runTest(analogMonitorTest); |
226 | 23 | 24 | ||
227 | 24 | runTest(arrayOpTest); | 25 | runTest(arrayOpTest); |
228 | 26 | runTest(scanEventTest); | ||
229 | 25 | 27 | ||
230 | 26 | epicsExit(0); /* Trigger test harness */ | 28 | epicsExit(0); /* Trigger test harness */ |
231 | 27 | } | 29 | } |
232 | diff --git a/src/std/rec/test/scanEventTest.c b/src/std/rec/test/scanEventTest.c | |||
233 | 28 | new file mode 100644 | 30 | new file mode 100644 |
234 | index 0000000..595264a | |||
235 | --- /dev/null | |||
236 | +++ b/src/std/rec/test/scanEventTest.c | |||
237 | @@ -0,0 +1,142 @@ | |||
238 | 1 | /*************************************************************************\ | ||
239 | 2 | * Copyright (c) 2018 Paul Scherrer Institut | ||
240 | 3 | * EPICS BASE is distributed subject to a Software License Agreement found | ||
241 | 4 | * in file LICENSE that is included with this distribution. | ||
242 | 5 | \*************************************************************************/ | ||
243 | 6 | |||
244 | 7 | /* | ||
245 | 8 | * Author: Dirk Zimoch <dirk.zimoch@psi.ch> | ||
246 | 9 | */ | ||
247 | 10 | |||
248 | 11 | #include <string.h> | ||
249 | 12 | #include "dbStaticLib.h" | ||
250 | 13 | #include "dbAccessDefs.h" | ||
251 | 14 | #include "dbUnitTest.h" | ||
252 | 15 | #include "testMain.h" | ||
253 | 16 | #include "osiFileName.h" | ||
254 | 17 | #include "epicsThread.h" | ||
255 | 18 | #include "dbScan.h" | ||
256 | 19 | |||
257 | 20 | void scanEventTest_registerRecordDeviceDriver(struct dbBase *); | ||
258 | 21 | |||
259 | 22 | /* test name to event number: | ||
260 | 23 | num = 0 is no event, | ||
261 | 24 | num > 0 is numeric event | ||
262 | 25 | num < 0 is string event (use same unique number for aliases) | ||
263 | 26 | */ | ||
264 | 27 | const struct {char* name; int num;} events[] = { | ||
265 | 28 | /* No events */ | ||
266 | 29 | {NULL, 0}, | ||
267 | 30 | {"", 0}, | ||
268 | 31 | {" ", 0}, | ||
269 | 32 | {"0", 0}, | ||
270 | 33 | {"0.000000", 0}, | ||
271 | 34 | {"-0.00000", 0}, | ||
272 | 35 | {"0.9", 0}, | ||
273 | 36 | /* Numeric events */ | ||
274 | 37 | {"2", 2}, | ||
275 | 38 | {"2.000000", 2}, | ||
276 | 39 | {"2.5", 2}, | ||
277 | 40 | {" 2.5 ", 2}, | ||
278 | 41 | {"+0x02", 2}, | ||
279 | 42 | {"3", 3}, | ||
280 | 43 | /* Named events */ | ||
281 | 44 | {"info 1", -1}, | ||
282 | 45 | {" info 1 ", -1}, | ||
283 | 46 | {"-0.9", -2}, | ||
284 | 47 | {"-2", -3}, | ||
285 | 48 | {"-2.000000", -4}, | ||
286 | 49 | {"-2.5", -5}, | ||
287 | 50 | {" -2.5 ", -5}, | ||
288 | 51 | {"nan", -6}, | ||
289 | 52 | {"NaN", -7}, | ||
290 | 53 | {"-NAN", -8}, | ||
291 | 54 | {"-inf", -9}, | ||
292 | 55 | {"inf", -10}, | ||
293 | 56 | }; | ||
294 | 57 | |||
295 | 58 | MAIN(scanEventTest) | ||
296 | 59 | { | ||
297 | 60 | int i, e; | ||
298 | 61 | int aliases[512] ; | ||
299 | 62 | int expected_count[512]; | ||
300 | 63 | #define INDX(i) 256-events[i].num | ||
301 | 64 | #define MAXEV 5 | ||
302 | 65 | |||
303 | 66 | testPlan(NELEMENTS(events)*2+(MAXEV+1)*5); | ||
304 | 67 | |||
305 | 68 | memset(aliases, 0, sizeof(aliases)); | ||
306 | 69 | memset(expected_count, 0, sizeof(expected_count)); | ||
307 | 70 | |||
308 | 71 | if (dbReadDatabase(&pdbbase, "scanEventTest.dbd", | ||
309 | 72 | "." OSI_PATH_LIST_SEPARATOR ".." OSI_PATH_LIST_SEPARATOR | ||
310 | 73 | "../O.Common" OSI_PATH_LIST_SEPARATOR "O.Common", NULL)) | ||
311 | 74 | testAbort("Database description 'scanEventTest.dbd' not found"); | ||
312 | 75 | |||
313 | 76 | scanEventTest_registerRecordDeviceDriver(pdbbase); | ||
314 | 77 | for (i = 0; i < NELEMENTS(events); i++) { | ||
315 | 78 | char substitutions[256]; | ||
316 | 79 | sprintf(substitutions, "N=%d,EVENT=%s", i, events[i].name); | ||
317 | 80 | if (dbReadDatabase(&pdbbase, "scanEventTest.db", | ||
318 | 81 | "." OSI_PATH_LIST_SEPARATOR "..", substitutions)) | ||
319 | 82 | testAbort("Error reading test database 'scanEventTest.db'"); | ||
320 | 83 | } | ||
321 | 84 | testIocInitOk(); | ||
322 | 85 | testDiag("Test if eventNameToHandle() strips spaces and handles numeric events"); | ||
323 | 86 | for (i = 0; i < NELEMENTS(events); i++) { | ||
324 | 87 | EVENTPVT pev = eventNameToHandle(events[i].name); | ||
325 | 88 | /* test that some names are not events (num=0) */ | ||
326 | 89 | if (events[i].num == 0) | ||
327 | 90 | testOk(pev == NULL, "\"%s\" -> no event", events[i].name); | ||
328 | 91 | else | ||
329 | 92 | { | ||
330 | 93 | expected_count[INDX(i)]++; /* +1 for postEvent */ | ||
331 | 94 | if (events[i].num > 0) | ||
332 | 95 | { | ||
333 | 96 | testOk(pev != NULL, "\"%s\" -> numeric event %d", events[i].name, events[i].num); | ||
334 | 97 | expected_count[INDX(i)]++; /* +1 for post_event */ | ||
335 | 98 | } | ||
336 | 99 | else | ||
337 | 100 | { | ||
338 | 101 | /* test that some strings resolve the same event (num!=0) */ | ||
339 | 102 | if (!aliases[INDX(i)]) | ||
340 | 103 | { | ||
341 | 104 | aliases[INDX(i)] = i; | ||
342 | 105 | testOk(pev != NULL, "\"%s\" -> new named event", events[i].name); | ||
343 | 106 | } | ||
344 | 107 | else | ||
345 | 108 | { | ||
346 | 109 | testOk(pev == eventNameToHandle(events[aliases[INDX(i)]].name), | ||
347 | 110 | "\"%s\" alias for \"%s\"", events[i].name, events[aliases[INDX(i)]].name); | ||
348 | 111 | } | ||
349 | 112 | } | ||
350 | 113 | } | ||
351 | 114 | post_event(events[i].num); /* triggers numeric events only */ | ||
352 | 115 | postEvent(pev); | ||
353 | 116 | } | ||
354 | 117 | |||
355 | 118 | testDiag("Check calculated numeric events (backward compatibility)"); | ||
356 | 119 | for (e = 0; e <= MAXEV; e++) { | ||
357 | 120 | testdbPutFieldOk("eventnum", DBR_LONG, e); | ||
358 | 121 | testdbGetFieldEqual("e1", DBR_LONG, e); | ||
359 | 122 | testdbGetFieldEqual("e2", DBR_LONG, e); | ||
360 | 123 | testdbPutFieldOk("e3", DBR_LONG, e); | ||
361 | 124 | testdbPutFieldOk("e3.PROC", DBR_LONG, 1); | ||
362 | 125 | for (i = 0; i < NELEMENTS(events); i++) | ||
363 | 126 | if (e > 0 && e < 256 && events[i].num == e) { /* numeric events */ | ||
364 | 127 | expected_count[INDX(i)]+=3; /* +1 for eventnum->e1, +1 for e2<-eventnum, +1 for e3 */ | ||
365 | 128 | break; | ||
366 | 129 | } | ||
367 | 130 | } | ||
368 | 131 | /* Allow records to finish processing */ | ||
369 | 132 | epicsThreadSleep(0.1); | ||
370 | 133 | testDiag("Check if events have been processed the expected number of times"); | ||
371 | 134 | for (i = 0; i < NELEMENTS(events); i++) { | ||
372 | 135 | char pvname[100]; | ||
373 | 136 | sprintf(pvname, "c%d", i); | ||
374 | 137 | testDiag("Event \"%s\" expected %d times", events[i].name, expected_count[INDX(i)]); | ||
375 | 138 | testdbGetFieldEqual(pvname, DBR_LONG, expected_count[INDX(i)]); | ||
376 | 139 | } | ||
377 | 140 | |||
378 | 141 | return testDone(); | ||
379 | 142 | } | ||
380 | diff --git a/src/std/rec/test/scanEventTest.db b/src/std/rec/test/scanEventTest.db | |||
381 | 0 | new file mode 100644 | 143 | new file mode 100644 |
382 | index 0000000..9118823 | |||
383 | --- /dev/null | |||
384 | +++ b/src/std/rec/test/scanEventTest.db | |||
385 | @@ -0,0 +1,16 @@ | |||
386 | 1 | record(calc, "c$(N)") { | ||
387 | 2 | field(SCAN, "Event") | ||
388 | 3 | field(EVNT, "$(EVENT)") | ||
389 | 4 | field(CALC, "VAL+1") | ||
390 | 5 | } | ||
391 | 6 | record(dfanout, "eventnum") { | ||
392 | 7 | field(OUTA, "e1 PP") | ||
393 | 8 | field(FLNK, "e2") | ||
394 | 9 | } | ||
395 | 10 | record(event, "e1") { | ||
396 | 11 | } | ||
397 | 12 | record(event, "e2") { | ||
398 | 13 | field(INP, "eventnum") | ||
399 | 14 | } | ||
400 | 15 | record(event, "e3") { | ||
401 | 16 | } |
Hi Dirk,
You can commit to a branch after you have submitted a Merge Request for it and the new commits will automatically appear in the Merge Request (check this one out, it has your b2d6b67 commit in it). Submitting a *new* MR implies there's something major that is different than before, maybe you rebased the branch. Please don't do resubmits for minor additional changes like this, they are unnecessary and send confusing signals to the reviewers.
I will take a look at this when I get a chance.
Thanks,
- Andrew