Skip to content

fix(meade): accept the unsigned site longitude INDI sends - #304

Merged
ClutchplateDude merged 1 commit into
OpenAstroTech:developfrom
rbhbokka:fix/meade-unsigned-site-longitude
Oct 5, 2026
Merged

ClutchplateDude merged 1 commit into
OpenAstroTech:developfrom
rbhbokka:fix/meade-unsigned-site-longitude

Conversation

@rbhbokka

@rbhbokka rbhbokka commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Regression from #291. INDI's site push is silently ignored, so every INDI/KStars session runs with whatever longitude the mount already had. The PR originally raised a protocol-convention question for maintainers; the history settles it (see The signed form and The convention question, resolved) and the code below implements the answer.

The defect

readLongitude() uses Cursor::signed3(), which makes the sign mandatory. INDI sends :Sg121*53# unsigned for a site 121°53′ west, so the command is answered 0 and the mount silently keeps whatever longitude it had.

Captured off the wire on an INDI connect to an OpenAstroExplorer:

:SL10:42:31  :SC09/05/26  :SG+7.0  :Sg121*53  :St+37*20

An unsigned Meade site longitude is the legacy count running westward from Greenwich, so it folds into the east-positive range the mount stores by negating modulo a full circle.

Not by 180 - value. That is the other reflection — the one that puts Greenwich at 180 — and it is what the (now dead) Longitude::ParseFromMeade computes and what MeadeProtocol.hpp described. It turns a 121°53′ west site into 58°07′ east: exactly 180°, i.e. 12 hours of local sidereal time, from where it belongs, which mispoints every subsequent GOTO.

None of this is visible in readback — :Gg# applies the inverse of whatever the setter did, so a wrong convention round-trips perfectly. It was verified instead by comparing :XGL# against an independently computed LST.

Also fixed

Range validation. digits(3, …) accepted 0..999 and the wrap was a single if, not a loop, so :Sg600*00# yielded −240°. Nothing downstream caught it: core::Longitude(int, int, int) never calls checkHours(), and EEPROMStore::storeLongitude clamps degrees * 100 into an int16 (±327.67°) — that clamp destroys the mod-360 equivalence and persists a genuinely wrong site across reboots. Values outside one full circle, and minutes above 59, are now refused.

Signed overflow on AVR. The arcminute arithmetic is widened to long. int is 16-bit on ATmega2560, so deg * 60 + mm exceeded INT16_MAX from :Sg545*69# onward — undefined behaviour in an -O2 build, and it did diverge between 16- and 32-bit targets. The range check also prevents reaching it; both are in.

The signed form — restored, not passed through

An earlier draft of this change passed signed values through as develop had them. What landed instead restores the pre-#291 convention:

  • Pre-refactor: Enhance Meade command parsing and response handling #291 Longitude::ParseFromMeade negated the signed form — result.totalSeconds = -dt.getTotalSeconds(), with a comment citing LX200 spec 2010.10 ("if a sign is present it is straight up longitude with east as negative") — and Longitude::formatStringForMeade negated again on the way out. The wire was east-negative on both sides.
  • refactor: Enhance Meade command parsing and response handling #291 dropped the negation on both sides at once, so the flipped convention round-tripped perfectly and nothing caught it.
  • The mount's internal core::Longitude has been east-positive throughout ("negative going west, positive going east"); only the wire mapping flipped.

readLongitude therefore folds the signed form through the same westward mapping as the unsigned one — +121*53 and 121*53 are the same meridian — and writeLongitude flips the sign back on the way out. The struct-level assertions (site_longitude_signed_negative_is_east) pin the hemisphere on the struct rather than the wire, so a future flip on one side alone cannot hide behind a clean round trip the way #291's did.

#296 asserts the opposite convention for the signed form; merging this supersedes that half of it.

The sub-degree sign — fixed by the stack

An earlier draft still lost the sign of a west longitude under one degree, because MeadeLongitude then carried the sign only in the sign bit of degrees and -0 is unrepresentable. #305 (merged) gave the struct a separate negative field and this PR's reader uses it: 000*30 (30′ west) and 359*30 (30′ east) now decode to different sites, pinned by site_longitude_unsigned_sub_degree_west_keeps_its_sign.

The convention question — resolved

MeadeProtocol.hpp documented the unsigned form as "0 to 360 going WEST with 180 at Greenwich. So 369 is 179W and 1 is 179E." No live code implements that, and it disagrees with what INDI puts on the wire: the pre-#291 code itself pointed at the INDI driver's definition (0..360 westward), and the captured traffic matches it. This PR updates that block to match observed client behaviour and the pre-#291 signed behaviour; the maintainer merge of this stack ratifies it.

Verification

pio test -e native: 281 pass at the original stacked tip (269 on then-develop + 12 added, none removed or weakened). After the post-#305 rebase, CI on the single-commit branch reports 323/323 native test cases passing. An exhaustive sweep of all 21,600 unsigned wire values decodes exactly. Builds clean under -Werror for oaeboardv1 and ramps; avr-g++ -mmcu=atmega2560 -Werror -Wconversion clean on the parser.

Rebase note

Originally stacked on #305. After #305 merged, rebased onto develop as a single commit containing only the :Sg changes. #303 (:SG) stacks on top of this and needs the same one-commit rebase before it merges.

@ClutchplateDude ClutchplateDude left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Meade protocol docs are contradictory with longitude, especially getter vs. setter.
The setter is defined as :SgDDD*MM and so does NOT accept a sign (it also give no indication what numbers are returned).
The getter is defined as :Gg# and is documented as returning sDDD*MM#, where East is negative.

I think we should have :Sg accept a (optional) sign.

@rbhbokka
rbhbokka force-pushed the fix/meade-unsigned-site-longitude branch from 3648272 to 6bd9591 Compare September 19, 2026 02:25
@rbhbokka

rbhbokka commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor Author

The getter is defined as :Gg# and is documented as returning sDDD*MM#, where East is negative.

Agreed, and it makes the change smaller rather than bigger. Under east-negative the signed and unsigned forms turn out to be the same mapping, so the two branches collapse into one reader with an optional sign, and the old 0..360 westward count is just the case where the sign is +. I checked every one of the 64,800 possible wire values and they never disagree.

That also gets rid of the sub-degree limitation I flagged in the description, since the sign now lives in MeadeLongitude::negative instead of in degrees.

I had to flip the getter in the same commit. :Gg replies east-positive today, so if only the setter moved, a client would send :Sg+121*53#, we'd store it correctly, then hand back -121*53#, which it reads as 121°53' East. That's 16h15m of sidereal time out, and INDI pushes the mirrored value straight back on the next connect and we save it. So writeLongitude negates too, with Greenwich pinned to +000*00#.

MeadeProtocol.hpp already said east-negative for both commands, for what it's worth. #291 dropped the negation on both sides at once, which is why nothing caught it: a wrong convention round-trips perfectly. I've written the convention into the MeadeLongitude comment so the type itself says which way round it is.

Two things that probably want a release note, since east-positive is in released firmware (v1.13.20) and not just develop:

  • Any client that adapted to the current :Gg will mirror its site once after upgrading.
  • A mount whose site was set under current firmware already has the mirrored value in EEPROM, and this doesn't fix that. Those need the site pushed again.

One other thing: #296 shouldn't merge alongside this. It flips the sign in MeadeCommandProcessor while this flips it in the parser, so they merge cleanly and then negate twice. I've left a note over there.

Rebased onto #305 and marked ready for review.

@rbhbokka

rbhbokka commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@ClutchplateDude :Sg now takes an optional sign, and :Gg returns east-negative so the value read back matches what was set.

MeadeProtocol.hpp documents :Sg and :Gg as east-negative -- zero at
Greenwich, negative coordinates going east. OpenAstroTech#291 dropped the negation on
both sides at once, so the wire convention silently inverted while every
readback still round-tripped perfectly.

Restore it on both sides in one commit. readLongitude negates into the
east-positive struct; writeLongitude negates back out. Moving only one
side would be worse than either convention: a client would set its site,
read back the mirror, and push the mirror in on the next connect, where
it persists to EEPROM.

Under east-negative the signed and the unsigned forms are the same
mapping -- east = wrap(-value) either way -- so the two branches collapse
into one reader with an optional sign, and the legacy 0..360 westward
count INDI sends is just the sign == '+' case. That also retires the
sub-degree-west limitation: the sign now travels in MeadeLongitude's
`negative` field rather than in `degrees`, so "000*30" (30' west) and
"359*30" (30' east) are no longer the same struct.

Greenwich goes out as "+000*00#": it is on neither side, and "-000*00"
reads as a negative zero.

The struct comment now records which convention the value is in. Nothing
in the type could show it before, which is how a flip on both sides at
once went unnoticed.

NOTE: this changes released behaviour. Firmware through v1.13.20 replies
to :Gg east-positive, so a client that adapted to that will mirror its
site once. A mount whose site was set under that firmware also holds the
mirrored value in EEPROM, which this does not correct -- the site has to
be pushed again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz
@ClutchplateDude
ClutchplateDude force-pushed the fix/meade-unsigned-site-longitude branch from 6bd9591 to 4ae0f3f Compare October 5, 2026 22:45
@ClutchplateDude
ClutchplateDude merged commit 1c56888 into OpenAstroTech:develop Oct 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants