<table><tr><td style="">chrisrowland added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D29355">View Revision</a></tr></table><br /><div><div><p>I can't comment on the change to the way that the capture meridian flip state and the mount meridian flip status state machines interact because there's no description of how the capture state machine works, nor what these changes do and why.</p>
<p>It looks as if some additional signals and slots have been added to pass state information between the modules. This is in addition to the polling that both modules do to monitor and respond to each other's state. Is this mixture a good idea?</p>
<p>Could we have some more description of what this very complex code is intending to achieve? And future developers will thank you if someone could document how the state machines should work.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29355#inline-167904">View Inline</a><span style="color: #4b4d51; font-weight: bold;">capture.cpp:3535</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">m_State</span> <span style="color: #aa2211">==</span> <span class="n">CAPTURE_PAUSED</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">// paused after meridian flip</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">secondsLabel</span><span style="color: #aa2211">-></span><span class="n">setText</span><span class="p">(</span><span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Paused..."</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Could do with some curly brackets here to improve readability.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29355#inline-167905">View Inline</a><span style="color: #4b4d51; font-weight: bold;">capture.cpp:3542</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">meridianFlipStage</span> <span style="color: #aa2211">=</span> <span class="n">stage</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">emit</span> <span style="color: #004012">newMeridianFlipStatus</span><span class="p">(</span><span class="n">Mount</span><span style="color: #aa2211">::</span><span class="n">FLIP_NONE</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">break</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What does this do and why?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29355#inline-167902">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mount.cpp:1293-1296</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">currentTelescope</span><span style="color: #aa2211">-></span><span class="n">pierSide</span><span class="p">()</span> <span style="color: #aa2211">==</span> <span class="n">ISD</span><span style="color: #aa2211">::</span><span class="n">Telescope</span><span style="color: #aa2211">::</span><span class="n">PierSide</span><span style="color: #aa2211">::</span><span class="n">PIER_WEST</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">flipDelayHrs</span> <span style="color: #aa2211">=</span> <span class="n">ha</span> <span style="color: #aa2211">+</span> <span class="n">delayHours</span> <span style="color: #aa2211">-</span> <span class="n">offset</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">else</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">flipDelayHrs</span> <span style="color: #aa2211">=</span> <span class="n">rangeHA</span><span class="p">(</span><span class="n">ha</span> <span style="color: #aa2211">+</span> <span style="color: #601200">12</span> <span style="color: #aa2211">+</span> <span class="n">delayHours</span><span class="p">)</span> <span style="color: #aa2211">-</span> <span class="n">offset</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No issues with doing this, I'd missed that it could take some time before the flip is attempted and fails.<br />
How will this test work if the mount doesn't report pier side? pierside could be PIER_UNKNOWN<br />
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.<br />
Maybe it would be better to check for PIER_EAST and swap the if and else statements.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R321 KStars</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D29355">https://phabricator.kde.org/D29355</a></div></div><br /><div><strong>To: </strong>murveit, chrisrowland, mutlaqja<br /><strong>Cc: </strong>kde-edu, narvaez, apol<br /></div>