Use the mount Pier Side property to manage Pier Flip
ClosedPublic

Authored by chrisrowland on Apr 20 2020, 11:05 AM.

Details

Summary
  1. Enter a commit message. #
  2. Changes: #
  3. kstars/ekos/capture/capture.cpp
  4. kstars/ekos/capture/capture.h
  5. kstars/ekos/mount/mount.cpp
  6. kstars/ekos/mount/mount.h
  7. kstars/ekos/mount/mount.ui
  8. kstars/kstars.kcfg

PierSide is used if it is available to manage pier flip.

Test Plan

Run with mount simulators and real mounts if possible to check:
Will a mount with PierSide flip?
If the mount flip is delayed and the flip attempt does not change
pier side is it retried after 4 minutes?
If the mount flips early and a slew close to the meridian is already
on the E->W side is no flip attempt made?
A flip should also be done at an hour angle of 12h as the mount tracks a
circumpolar object passing under the pole.

If the mount does not report PierSide the flip should still work as before but
there will be no retry if the flip fails.

The new Telescope Simulator can be used but will need the simulator options
enabled.

Diff Detail

Repository
R321 KStars
Branch
UsePierSideForFlip (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25823
Build 25841: arc lint + arc unit
chrisrowland created this revision.Apr 20 2020, 11:05 AM
Restricted Application added a project: KDE Edu. · View Herald TranscriptApr 20 2020, 11:05 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
chrisrowland requested review of this revision.Apr 20 2020, 11:05 AM

Thank you Chris for the PR, this makes meridian flips much more reliable!

@TallFurryMan @wreissenberger Please review the differential and share your thoughts.

I did a first test with the simulator, everything seems to work smoothly.

One small thing: if the max HA limit is enabled and the limit has been reached, the message about aborting motion appears each time when the mount status is refreshed.

I would like to test it tomorrow night in the field. Today there is no chance, it's clear but awfully windy.

  • Wolfgang

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.

kstars/ekos/capture/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?

kstars/ekos/mount/mount.cpp
80

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

116

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

603

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.

619

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 >?

650–651

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

988

Wrong comment? minAltLimit?

1120

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?

1136

This function misses translations.

1256

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

1342

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

1355

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

1865

Same remark as the similar function above.

1887

Same remark as the similar function above.

kstars/ekos/mount/mount.ui
9–10

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

371

This should not move I think.

mutlaqja added inline comments.Apr 22 2020, 6:00 AM
kstars/ekos/capture/capture.cpp
6831

Isn't this getting casted to QString? I saw we avoid C strings in the code. Perhaps the only change required here is more friendly names.

TallFurryMan added inline comments.Apr 22 2020, 9:15 AM
kstars/ekos/capture/capture.cpp
6831

It is getting cast of course, but my point is that the function has no reason to return a QString when what it returns is a pooled char pointer. When the caller function wants to use the raw string (see QTest's fixture identifiers for instance) it must re-convert the QString back to StdString and retrieve the C internal pointer to the array. Lazy conversion is always best.

Thanks for the feedback,
I've skimmed through this and put in some comments, primarily about the QString issue that TFM raised.

The hour angle limit code is not fnished and I had paused this in order to get the flip management code out but will see what I can do at least to make it work as an emergency halt.

In a full implementation I would like to be able to pause tracking before the meridian and wait until the target had moved pat the flip limit, then resume by doing a flip.

I'll go through this in detail now, with the code and respond in more detail.

will be interesting to see how this works...

chrisrowland marked 4 inline comments as done.Apr 23 2020, 11:40 AM

My first attempt to add inline comments failed, try again.

kstars/ekos/capture/capture.cpp
6831

This is my C# .NET background coming out, I don't see a problem with returning a string as what it is - a string.

Not sure what would be a more friendly name, MeridianFlipStagestring() maybe?

kstars/ekos/mount/mount.cpp
603

No change required, certainly not going to duplicate the log statement three times, there's too much copy and paste code anyway.

1136

what do you mean? If to different languages I have no idea how to do this.

1256

By design, a mount that does not implement pier side will return PIER_UNKNOWN, that's the only other value in the enum.

I don't want a mount that doesn't report pier side to fail because it will keep retrying, it needs to behave as now where it tries once only.

1342

Not sure what the various times available to me are, a clue would help.

kstars/ekos/mount/mount.ui
371

Are these changes in the UI under my control? the sizes maybe but the auto generated code moving isn't something I can control.

I'm sorry if I sounded harsh in my comments. The last thing I want is a flamewar between C# and C++ ;)

kstars/ekos/capture/capture.cpp
6831

Yeah C# relieved people from many language weirdnesses I suppose.
The friendly name didn't relate to the function name: my point was that the strings returned by the function were used in logs, thus could be more friendly to the end-user. But this is completely minor.

kstars/ekos/mount/mount.cpp
1136

This is also minor, but it could have been the opportunity to wrap the strings with i18n() so that they are registered as translatable.
After all, they appear in the UI.

1342

KStarsData::Instance()->clock()->utc() as done at line 1326.

kstars/ekos/mount/mount.ui
371

They should not be part of your commit actually, as you did not change this yourself.

Also, the summary will appear as commit in the kstars history, so you may want to rewrite it.

chrisrowland marked 23 inline comments as done.

Fixes as requested

Details are in the inline comments, mainly improvements to the Ha limit
detection.

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.

kstars/ekos/capture/capture.cpp
6831

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.

kstars/ekos/mount/mount.cpp
619

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

650–651

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.

1120

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.

1342

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.

1355

Probably not, refactored this if statement

1865

Same reply!

mutlaqja accepted this revision.Apr 24 2020, 5:54 AM
This revision is now accepted and ready to land.Apr 24 2020, 5:54 AM

Thanks everyone for this.

One thing, I made some minor changes which I missed committing using 
arc diff.  They are adding the i18n to mark things for translation and
the change to to clock that Eric asked for.

Can I run the arc diff now and get these in the distribution?

Chris

apol added a comment.Apr 24 2020, 11:12 AM

I'm not sure why you added me here but yes, you sure can add new things to a patch.

Further changes as requested

i18n added to user facing strings so they can be translated.
Clock changed to Kstars one as requested.

One small answer on the concurrent use of m_Status. Just for theory.

kstars/ekos/mount/mount.cpp
650–651

SetTrackEnabled at 636, added by you, produces a asynchronous change on the mount status. You are then checking if the status changed at 651 using a volatile m_Status. Therefore, the result of your test is undefined. If you want to make it deterministic, you need to copy the value of m_Status before any action from your side that would change it.

Now, the update of m_Status is handled by a signal connection, so there may be a good chance that the current function will be executed before the update function is. There's also statistically a good chance that the mount interaction takes longer that this function code. But it all depends on the Qt thread model, and how dbus events are really managed.

wreissenberger accepted this revision.Apr 28 2020, 3:44 AM

Tests last night where positive, the meridian flip went flawless.

Alright, so I'll merge this in and we'll give it more testing.

mutlaqja closed this revision.Apr 29 2020, 2:08 PM

I'll merge this myself directly, so closing this.