meade offset removed conditionally #168

anjok · 2022-02-10T12:11:34Z

whole thing is explained here: #166

openastrotech-bot · 2022-02-10T12:12:04Z

Looks like your PR has code that needs to be changed in order to meet our coding standards!
Here are your options:

Apply the patch that was generated by the job
1. Click details under the failing clang-format check
2. Click the Artifacts dropdown in the top right
3. Download + unzip the clang-format-diff.patch file into the OpenAstroTracker-Firmware repo
4. Run git apply clang-format-diff.patch to make the changes
5. Commit and push up the formatted code
Run clang-format locally
1. Run the command bash -c 'shopt -s nullglob; for i in ./{.,src,src/inc,boards/*}/*.{c,cpp,h,hpp}; do clang-format -i $i; done'
2. Commit and push up the formatted code

julianneswinoga

FYI in order for all the workflow checks to pass you also need to update Changelog.md and Version.h

src/Longitude.cpp

julianneswinoga · 2022-02-12T19:06:39Z

src/Longitude.cpp


+// define INDI in the local configuration if you want it to work
+#ifndef INDI
+    #define INDI 0


I would rename this #define to be a little more specific. INDI_LONGITUDE_HACK maybe?

Actually... it's the opposite, at least from my understanding. What it does is preserve the current parsing on Gg12 -> 180 - 12 for users of the current ASCOM driver, when the correct value should be - 12. The parsing assumes that KStars/Meade/ASCOM will have a 180 - x offset added when in fact it does not.

The whole thing is explained here (and I should referenced this in the subject): #166

I just named it INDI (which is a lame name, agreed, maybe INDI_SUPPORT) because I have a feeling there might be more stuff coming in to support KStars and I didn't want flags for all of them.

We can re-evaluate at a later date if more INDI-specific changes are needed. More specific is better in the current case.

anjok · 2022-02-13T05:26:23Z

I don't get the needed change for changelog and version. Why is this on me to do that? How would I know which version this is going into?

Normally, you'd merge some PRs to develop, then decide it's time for a release and do the changelog and version in a commit there...

anjok · 2022-02-13T07:21:55Z

Added version stuff, renamed and moved define and force-pushed.

julianneswinoga · 2022-02-13T16:47:48Z

I don't get the needed change for changelog and version. Why is this on me to do that? How would I know which version this is going into?

Normally, you'd merge some PRs to develop, then decide it's time for a release and do the changelog and version in a commit there...

It's up to the author how they would like to describe their changes in the changelog. It's not auto-generated from commits (which I'm assuming is what you are thinking about) because commits are directed more at developers, where the changelog is directed more at users.
For changes like this the next version is simply a minor version increment. We try to follow https://semver.org/ convention
For full releases the changes are thus very easy to create simply by looking at the prose changes in Changelog.md 😄

julianneswinoga · 2022-02-13T16:49:39Z

Added version stuff, renamed and moved define and force-pushed.

It would be appreciated if you didn't squash+force-push while a PR is under review (i.e. only do that right before the PR is going to be merged). It makes it very hard to follow incremental changes and review them accordingly.

julianneswinoga · 2022-02-13T16:53:38Z

Changelog.md

+**V1.10.8 - Updates**
+- Fix Meade parsing from INDI clients.
+
 **V1.10.6 - Updates**


It looks like you need to fetch and rebase your changes onto develop, you created your branch quite a while ago:

# Add an upstream URL # This URL is for ssh, change it if you use HTTPS git remote add upstream git@github.com:OpenAstroTech/OpenAstroTracker-Firmware.git git fetch -p upstream git rebase -i origin/develop # Interactive rebase # Fix merge conflicts git push -f # Force push is needed because the commits have changed

julianneswinoga · 2022-02-13T16:57:41Z

src/Longitude.cpp

 #include "Longitude.hpp"

+#if INDI_SUPPORT == 1
+    #define LONGITUDE_OFFSET 0L


Could you also add in

#pragma message("Note: INDI offset used, firmware will not function correctly with ASCOM!")

? Just so that we can have traceability if a user is having problems.

julianneswinoga · 2022-02-13T16:59:24Z

src/Longitude.cpp


+// define INDI in the local configuration if you want it to work
+#ifndef INDI
+    #define INDI 0


We can re-evaluate at a later date if more INDI-specific changes are needed. More specific is better in the current case.

julianneswinoga · 2022-02-13T17:00:27Z

Configuration_adv.hpp

+
+// define INDI_SUPPORT in the local configuration if you want it to work
+// currently this will fix Meade longitude parsing
+#ifndef INDI_SUPPORT


Please move this to the Misc Configuration section of this file

JakobPetersson · 2022-03-21T15:59:12Z

@cameronswinoga Should I create a new PR with these changes if i wish to see them merged?

anjok · 2022-03-21T20:14:12Z

You also need to fix the sign of the utc offset in the meadeparser

anjok · 2022-03-21T20:24:12Z

Like so, but you also need to move the flag to Configuration_avd.hpp (or just skip these conditionals alltogether and just fix it, as this is only about windows users not needing to update the ASCOM driver) :

 void Mount::setLocalUtcOffset(int offset)
 {
+    #if INDI
+    offset = -offset;
+    #endif
     _localUtcOffset = offset;

JakobPetersson · 2022-03-25T11:36:46Z

Fixes for this are included in #181.
I suggest we close this and move forward there.

ClutchplateDude · 2022-03-25T15:42:18Z

Fixes made in #181

anjok force-pushed the longitude branch from 0d41e09 to 5593050 Compare February 10, 2022 12:15

julianneswinoga requested changes Feb 12, 2022

View reviewed changes

anjok force-pushed the longitude branch from 5593050 to 5a9e837 Compare February 13, 2022 07:17

meade offset removed conditionally

409c967

anjok force-pushed the longitude branch from 5a9e837 to 409c967 Compare February 13, 2022 07:21

julianneswinoga reviewed Feb 13, 2022

View reviewed changes

julianneswinoga requested changes Feb 13, 2022

View reviewed changes

JakobPetersson mentioned this pull request Mar 21, 2022

Protocol INDI corrections #180

Closed

ClutchplateDude closed this Mar 25, 2022

meade offset removed conditionally #168

meade offset removed conditionally #168

Uh oh!

Conversation

anjok commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openastrotech-bot commented Feb 10, 2022

Uh oh!

julianneswinoga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julianneswinoga Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

anjok Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

julianneswinoga Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

anjok commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anjok commented Feb 13, 2022

Uh oh!

julianneswinoga commented Feb 13, 2022

Uh oh!

julianneswinoga commented Feb 13, 2022

Uh oh!

julianneswinoga Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

julianneswinoga Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

julianneswinoga Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

julianneswinoga Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

JakobPetersson commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anjok commented Mar 21, 2022

Uh oh!

anjok commented Mar 21, 2022

Uh oh!

JakobPetersson commented Mar 25, 2022

Uh oh!

ClutchplateDude commented Mar 25, 2022

Uh oh!

Uh oh!

anjok commented Feb 10, 2022 •

edited

Loading

anjok commented Feb 13, 2022 •

edited

Loading

JakobPetersson commented Mar 21, 2022 •

edited

Loading