Simplify soft_init logic

kmpeters commented 5 years ago

As @anjohnson points on in https://github.com/epics-modules/motor/issues/110#issuecomment-423321120, this logic in devSoftMotor.cc's soft_init routine is unnecessarily complicated:

long soft_init(int after)
{
    int before_after = (after == 0) ? 0 : 1;

    if (before_after == 0)

And can be simplified:

long soft_init(int after)
{
    if (after)

The simplification may also apply to other motor drivers.

kmpeters commented 5 years ago

The changes that resulted in the current state of the code are documented here:

https://github.com/epics-modules/motor/issues/110#issuecomment-424015707

kmpeters commented 5 years ago

Correction:

long soft_init(int after)
{
    int before_after = (after == 0) ? 0 : 1;

    if (before_after == 0)

Can be simplified as:

long soft_init(int after)
{
    if (after == 0)

@anjohnson, can you confirm that the simplification in https://github.com/epics-modules/motor/issues/110#issuecomment-423321120 is incorrect?

kmpeters commented 5 years ago

The corrected simplification is consistent with the state of the code from many years ago: https://github.com/epics-modules/motor/blame/9559b510a6ed41278bf1774fea40439120688d66/motorApp/SoftMotorSrc/devSoftAux.c#L77

anjohnson commented 5 years ago

long soft_init(int after)
{
    int before_after = (after == 0) ? 0 : 1;

    if (before_after == 0)

soft_init(x)

int before_after = (after == 0) ? 0 : 1; ⇒ before_after = (x == 0) ? 0 : 1 ⇒ before_after = !x ? 0 : 1 ⇒ before_after = !!x

if (before_after == 0) ⇒ if (!!x == 0) ⇒ if (!!!x) ⇒ if (!x)

So yes my original simplification was inverted. What I'm asking is that code should just use the predicate name after or its inverse !after (pronounced not-after) directly in the if statement, so depending on the sense you want should thus read either:

if (!after) {
    /* Stuff to do before record links have been initialized */
}

or

if (after) {
    /* Stuff to do after record links have been initialized */
}

Both ==0 or ?0:1 are synonyms for ! when the LHS is a Boolean value, and reading those expressions is much harder than naming Boolean variables as predicates and using the name directly as a true/false condition. The name before_after isn't a predicate, whereas after is which is why the latter is easier to read.

You can probably guess immediately when the following code should do its thing:

if (thursday) {
    /* I never could get the hang of Thursdays */
}

</rant> — sorry…

MarkRivers commented 5 years ago

Note that the devSoftAux.cc can be significantly simplified in other ways as well.

In soft_init when after=0 it does the following:

Finds the priority of the dbCaTask
Creates a new thread, soft_motor_task, with this priority-1
The soft_motor_task simply waits for an epicsEvent, and then it runs through the list of soft motors and does some initialization on them. It then just sits idle forever.
The epicsEvent is signaled by soft_init when after=1.

This is needlessly complex. I believe it could be changed to the following:

Just take the code in soft_motor_task and move it to soft_init when after=1.
No thread is created, no event needs to be signaled.

Anyone see a reason this would not work?

kmpeters commented 5 years ago

Anyone see a reason this would not work?

I don't see any reason why it wouldn't work, but I think it should be its own github issue, even though it does fall under the very vague title I gave this issue.

timmmooney commented 5 years ago

I think it's not a good idea to attach a CA context to whatever task is executing soft_init(), but I don't know why I think this.

Tim Mooney (mooney@aps.anl.gov) (630)252-5417 Beamline Controls Group (www.aps.anl.gov) Advanced Photon Source, Argonne National Lab

From: Mark Rivers notifications@github.com Sent: Tuesday, October 9, 2018 4:11:10 PM To: epics-modules/motor Cc: Subscribed Subject: Re: [epics-modules/motor] Simplify soft_init logic (#115)

Note that the devSoftAux.cc can be significantly simplified in other ways as well.

In soft_init when after=0 it does the following:

Finds the priority of the dbCaTask
Creates a new thread, soft_motor_task, with this priority-1
The soft_motor_task simply waits for an epicsEvent, and then it runs through the list of soft motors and does some initialization on them. It then just sits idle forever.
The epicsEvent is signaled by soft_init when after=1.

This is needlessly complex. I believe it could be changed to the following:

Just take the code in soft_motor_task and move it to soft_init when after=1.
No thread is created, no event needs to be signaled.

Anyone see a reason this would not work?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/epics-modules/motor/issues/115#issuecomment-428353753, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AGfnl9mXn5wR8PLEmFkR6BB5ZisPa_S7ks5ujRDugaJpZM4W28nf.

epics-modules / motor

Simplify soft_init logic #115