D29008: Use the mount Pier Side property to manage Pier Flip

[Eric Dejouhanet]

TallFurryMan added a comment.


  A few (pedantic) comments from me. Overall, this seems nice although I wasn't able to test. I am under the impression that something is missing for the new options, but I may be wrong.
  The most annoying point for me is the broken test after stopping to track on HA limit. And also the system time instead of the simulation time, which will make things difficult to test.

INLINE COMMENTS

> capture.cpp:6831
>  
> +QString Capture::MFStageString(MFStage stage)
> +{

Prototype `char const * Capture::MFStageString(MFStage stage) const` is more appropriate?
That function has no interest in providing a QString object.
Also, these strings are used in logs, so you may want to output something more friendly?

> mount.cpp:78
>  
> +    maxHaLimit->setValue(Options::maximumHaLimit());
> +    connect(maxHaLimit, SIGNAL(editingFinished()), this, SLOT(saveLimits()));

Pedantic, but move your setValue with others at line 72, or group setValue/connect calls for minAltLimit, maxAltLimit and maxHaLimit.

> mount.cpp:117
> +    connect(enableHaLimitCheck, SIGNAL(toggled(bool)), this, SLOT(enableHourAngleLimits(bool)));
> +    //enableHaLimitCheck->setChecked(Options::enableHaLimit());
> +    haLimitEnabled = enableHaLimitCheck->isChecked();

This comment makes the whole flip procedure initially dependent on the UI default state, this seems quite unexpected!

> mount.cpp:604
> +            // get hour angle limit
> +            double haLimit = 25.0;      // set an impossible value so no check if not set
> +            bool haLimitReached = false;

std::numeric <double>::max maybe?
But that would make the log weirder than it already is when pierSide() doesn't report a useful value, so OK.
Or duplicate your log inside the switch/case and drop the variable.

> mount.cpp:620
> +                    break;
> +            }
> +

The block above seems OK in the southern hemisphere. Always difficult to wrap my head around it :)
Is the limit inclusive, or defined as the HA value which triggers a flip? Pedantic again, but >= instead of >?

> mount.cpp:653-654
>  
>          ISD::Telescope::Status currentStatus = currentTelescope->status();
>          bool isTracking = (currentStatus == ISD::Telescope::MOUNT_TRACKING);
>          if (m_Status != currentStatus)

This test is potentially broken by, plus suffers from a race condition from the setTrackEnabled() call at line 636.

> mount.cpp:991
> +{
> +    //Only enable if it was already enabled before and the minAltLimit is currently disabled.
> +    if (haLimitEnabled && maxHaLimit->isEnabled() == false)

Wrong comment? minAltLimit?

> mount.cpp:1123
> +    // a slew to the current position can return so quickly that the status isn't updated
> +    auto ret = currentTelescope->Slew(currentTargetPosition);
> +    if (ret)

I notice the use of `auto` here to replace `bool`. Technically I can't find any definitive reason against it, except maybe the fact that there is a `if` immediately after. Thus if Slew() changes its prototype, that `auto` may hide some issues. But, well, pedantic again. `if Slew() { m_Status = ... ; return true; }` would seem clearer to me.

Also, m_Status is supposed to change from line 653 as a mirror of the telescope status. Is it expected that it will update here, and only when the slew is successful?

> mount.cpp:1137
>  bool Mount::checkMeridianFlip(dms lst)
>  {
> +    // checks if a flip is possible

This function misses translations.

> mount.cpp:1257
> +                // pointing state change check only for mounts that report pier side
> +                if (currentTelescope->pierSide() == ISD::Telescope::PIER_UNKNOWN)
> +                {

Pedantic, you test "other than PIER_WEST and PIER_EAST" elsewhere, and PIER_UNKNOWN here.

> mount.cpp:1343
>  
> +    minMeridianFlipEndTime = QDateTime::currentDateTimeUtc().addSecs(minMeridianFlipDurationSecs);
> +

This must use the KStars simulation time, not the system time.

> mount.cpp:1353
> +    }
> +    appendLogText("Meridian flip slew started...");
>      return result;

This log should not be sent if the slew is not successful, should it?

> mount.cpp:1864
>  
> +QString Mount::meridianFlipStatusString(MeridianFlipStatus status)
> +{

Same remark as the similar function above.

> mount.cpp:1886
> +
> +QString Mount::pierSideStateString()
> +{

Same remark as the similar function above.

> mount.ui:9-10
> +    <y>0</y>
> +    <width>743</width>
> +    <height>519</height>
> +   </rect>

This should not change, although it has no impact on runtime.

> mount.ui:371
> +            </property>
> +            <item row="2" column="1">
> +             <widget class="QRadioButton" name="meridianFlipDegreesR">

This should not move I think.

REPOSITORY
  R321 KStars

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

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