D29355: Fix issue with Meridian Flip Retry

[Hy Murveit]

murveit marked 4 inline comments as done.
murveit added a comment.


  Chris,
  
  I agree whole-heartedly with the need for doc for the state machines, but I am not on top of them, nor have I made any real changes to them.  If you like, we can collaborate on some doc for mount and capture, but I think that's outside the scope of this PR.
  
  I do want to explain what I did with respect to the new emit in capture.cpp.  When capture emits newMeridianFlipStatus, mount's meridianFlipStatusChanged() method gets called. Those two things are tied together by the "connect" in ekos/manager.cpp line 3369. I believe that is the only effect of that emit.
  
  Unfortunately, mount's meridianFlipStatusChanged() method also got called inside mount.cpp at line 805. That is why I added the ...Internal method(), so that the internal mount call wouldn't execute the 4 new lines I added 1378-1384, but rather would do what it used to do.
  
  So the net effect of the new capture emit, is simply to update the new Mount::m_CaptureMFStatus class variable.  I believe that's all it changes.
  
  Now, the new class variable is just used in one place, line 1225, where it may delay mount moving from FLIP_NONE to FLIP_PLANNED.  The reason I delay this, is because if mount moves to FLIP_PLANNED, it emits newMeridianFlipStatus(FLIP_PLANNED), which (via the connect in manager.cpp line 3371) calls Capture::meridianFlipStatusChanged(FLIP_PLANNED), and that will ignore the message unless the capture state is MF_NONE. So now mount doesn't send it until capture won't ignore it. If mount sent it earlier, the flip wouldn't happen, as I described in my email bug report.  I did try to comment this with the comment on line 1223. If you feel this comment isn't clear enough, please suggest how you'd like it worded, assuming it is clear to you now.
  
  Like you, I don't fully understand the mount/capture state machines enough to make significant changes to them, so I tried to keep things as they were, except for this one delay in change of state.

INLINE COMMENTS

> chrisrowland wrote in capture.cpp:3535
> Could do with some curly brackets here to improve readability.

I removed the dead code, and the IMHO obvious comment.
I think without them, the readability is fine, but if you want, I can add braces here.

> chrisrowland wrote in capture.cpp:3542
> What does this do and why?

Please see my general comment on this update.

> chrisrowland wrote in mount.cpp:1293-1296
> No issues with doing this, I'd missed that it could take some time before the flip is attempted and fails.
> How will this test work if the mount doesn't report pier side?  pierside could be PIER_UNKNOWN
> From what I can see it will use ha + 12 which is the case for the flip as the mount is crossing the meridian under the pole at about 12h.
> Maybe it would be better to check for PIER_EAST and swap the if and else statements.

I did what you suggested and reversed if/else and tested for east. As you say, unknown doesn't get here.

REPOSITORY
  R321 KStars

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

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