-
-
Notifications
You must be signed in to change notification settings - Fork 851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue705(part of the issue): Added Altitude and Azimuth to TelescopeTCP class #3626
Open
huntflex
wants to merge
5
commits into
Stellarium:master
Choose a base branch
from
huntflex:Issue705
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+74
−7
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
408c3fb
figured out conversion to alt az
huntflex 1d4ec9b
Added altitude and azimuth to TelescopeTCP. The interpretation of the…
huntflex 1bb8588
Fixed clang warnings
huntflex cd961d5
Typo fixed
huntflex 3529f73
Fixed codefactor comment
huntflex File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The necessity for refraction correction should depend on atmosphere visibility. It has nothing to do with JNow setting. What is your motivation to connect them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not sure what it is used for. The reason I have this check is because I saw in the TelescopeGoTo method that it was used to correct the J2000 position. See line 300-304:
Is there a way to check for atmosphere visilibility. And if so should it also be used in the screenshot above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, when driving a telescope from a terrestrial location, it is safe to assume (the real, physical) atmosphere (in which we breathe) is on.
If you have "pure" J2000 data from a catalog/ephemeris (without refraction correction), You must convert them to JNow equatorial position without refraction correction (core->j2000ToEquinoxEqu(j2000Pos, RefractionOff) when the telescope expects that (and applies refraction correction internally).
Then, to compute geometrical Alt-Az position for a goto capable device, you compute alt-az position (core->j2000ToAltAz(positionJ2000, refractionMode);) If your telescope expects AltAz coordinates to be corrected for refraction, you should compute with RefractionOn. If your telescope expects AltAz position to be without refraction correction (because it will do that by itself!), you use RefractionOff.
I have never used an alt-az GOTO telescope, so I am not familiar with these protocols. Maybe this needs yet another config option ("Goto is AzAlt and expects/does not expect refraction correction") Does such a device track the object after the goto slew? Why not just send RA/Dec to a properly configured/set up mount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that my own Eq6-r pro does refraction correction based on the elevation you input. I don't know about other mounts.
Seeing that the class I'm extending is the TelescopeTCP class which is most likely used for (I asumme??) for a homemade telescope control system maybe it would be wise to have these options under the Telescope properties when you set up the host.
For example:
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the Boolean JNow setting will have to be extended to a 3-value J2000/JNow/AltAz, and an additional Boolean RefractionCorrection would govern the independent extra. @alex-w ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-w I wanted to extend the relevant telescope classes with altitude azimuth. I thought a natural (and easier place) to begin with would be the telescopeTCP class. As you know the TelescopeTCP class is the one instantiated whenever a user configures a telescope with a "remote computer". I assumed that one would also want access to alt/az in this kind of setup since a "remote computer" might not be limited to an equatorial mount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the mount can be AltAz-only and I fear these mounts will not understand some parts of protocol. Plus EQ/AltAz-mount requires setting the mode. In both cases changes of protocols can be for first item only - commnication directly through a serial port. The second item is incompatible now (I really don't know modern use cases for old Stellarium protocol for mounts - a TelescopeServer).
What about real hardware - is it working in AltAz mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be useful for DIY projects with self-made controllers (e.g. Arduino, self-made axis decoder/motor protocol, ...). But I have never used the telescopeTCP stuff, so I cannot say how relevant this still is.
The docs in TelescopeClient.hpp indicate a "Stellarium telescope control protocol" to be included in "telescope server software". Where is this all? (Yes, Google found a 1-page file from J.G. on some "free" site.) Is/Was that a reference implementation of something useful? Then it should probably be archived in a repo here. And somebody who was involved should write the design docs to explain it all. (OK, dreaming...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first version of TelescopeServer is located in SourceForge (SVN: https://sourceforge.net/p/stellarium/code/HEAD/tree/trunk/), the second version is in GitHub: https://github.com/johannesgajdosik/TelescopeServer2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm wondering if this is the right course of action with this patch. Whether we should continue with this PR?