D18235: Meridian flips handled by Mount

[Eric Dejouhanet]

TallFurryMan added a comment.


  That's great, agreed. I added a few (as usual) pedantic comments. My setup is out of order while I install the fixed obs, so I don't know when I will be able to test.

INLINE COMMENTS

> capture.cpp:3144
> +
> +        if (m_State == CAPTURE_IDLE || m_State == CAPTURE_ABORTED || m_State == CAPTURE_COMPLETE)
> +        {

OK so here, Mount is telling Capture it needs to flip, and capture either replies it is OK to flip now, or delays. Correct? So MF_REQUESTED can be set multiple times.

> capture.cpp:3329
>                  }
>                  else if (!strcmp(tagXMLEle(ep), "MeridianFlip"))
>                  {

What happens when the XML doesn't embed meridian flip information? We retain the previous state?

> capture.cpp:4597
> +
> +    if (reply.error().type() != QDBusError::NoError)
> +        qCCritical(KSTARS_EKOS_CAPTURE) << QString("Warning: setting meridian flip request received DBUS error: %1").arg(QDBusError::errorString(reply.error().type()));

We'll perhaps need a UI feedback here. No issues.

> mount.cpp:46
>  #define ABORT_DISPATCH_LIMIT 3
> +#define MF_DELAY             12000
>  

Rationale?

> mount.cpp:502
> +                setInitialHA((sgn == '-' ? -1: 1) * ha.Hours());
> +                delete currentTargetPosition;
> +                currentTargetPosition = new SkyPoint(telescopeCoord.ra(), telescopeCoord.dec());

Small interval where currentTargetPosition is undefined.

> mount.cpp:886
> +
> +        if (initialHA() >= 0 || meridianFlipCheckBox->isChecked() == false)
> +        {

There might be something here. If the mount parking position is misplaced, it may happen that initialHA is positive in kstars, but that the mount is still on the wrong side. Do we avoid flipping still? Arguable.

> mount.cpp:903
> +        {
> +            setMeridianFlipStatus(MF_PLANNED);
> +        }

Even if the flip checkbox is unset?

> mount.cpp:945
> +    setMeridianFlipStatus(MF_RUNNING);
> +    slew(currentTargetPosition->ra().Hours(), currentTargetPosition->dec().Degrees());
>      return true;

We'd require error management here.

> mount.h:59
>  
> +    typedef enum { MF_IDLE, MF_PLANNED, MF_WAITING, MF_ACCEPTED, MF_RUNNING, MF_COMPLETED, MF_ERROR } MeridianFlipStatus;
>  

Aren't those name contexts clashing with Capture? It could be easy to mix them up.

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D18235

To: wreissenberger, mutlaqja, TallFurryMan
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190114/4668e40d/attachment.html>