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

[Chris Rowland]

chrisrowland added a comment.


  I've submitted a diff with most of the changes requested incorporated.
  
  i'm sticking on the QString issue I think it' the right way to go.

INLINE COMMENTS

> TallFurryMan wrote in capture.cpp:6831
> 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?

I don't see a need to change this.
QString is used to return strings everywhere else, see getSequencequeueStatus() etc.

I have chosen to use the enum names as the string because these then match what I see in the code.  To a large extent logs are for the developer, to help them debug the code remotely so using the same name in the code and the log is friendly.

> TallFurryMan wrote in mount.cpp:620
> 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 >?

Yes, I thought about it and I think it's OK in the South, but will need checking of course.  It's using hour angle comparisons and they should be hemisphere agnostic.

I'm not inclined to be that worried about the difference between < and <=, it only matters for less than a millisecond.

More to the point the code for the PIER_EAST case is broken because of problems with angle comparisons.  I've refactored the check and it now seems OK

> TallFurryMan wrote in mount.cpp:653-654
> This test is potentially broken by, plus suffers from a race condition from the setTrackEnabled() call at line 636.

the isTracking line needs moving down to where it's used.

The code turning tracking off is copied from the previous altitude limits code above.
The code from isTracking downwards is almost unchanged from what was there before, I've set the flipDelayHours to 0 and added some debug messages.
So I don't think it's any more broken than it was before, I'd like to leve it for now.

> TallFurryMan wrote in mount.cpp:1123
> 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?

Changed auto to bool, a slight increase in readability.

I've changed the if statement as you suggest, not sure it makes much difference.

Yes, the status is expected only to be updated if the slew command is successful.

> chrisrowland wrote in mount.cpp:1343
> Not sure what the various times available to me are, a clue would help.

I guess the simulation time is in KStars::Instance()->data().

This time is used to measure the actual clock time that the mount takes to complete a meridian flip slew.  If it's less than the minimum (20 seconds) the flip has failed.
Not sure that using ...data()->ut() would help.

> TallFurryMan wrote in mount.cpp:1353
> This log should not be sent if the slew is not successful, should it?

Probably not, refactored this if statement

> TallFurryMan wrote in mount.cpp:1864
> Same remark as the similar function above.

Same reply!

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/20200423/eac5faac/attachment.html>