From 4ae0f3f381c53f005e9b3779217364e444266c60 Mon Sep 17 00:00:00 2001 From: Kiryl Date: Sun, 6 Sep 2026 20:06:49 -0700 Subject: [PATCH] fix(meade): read and write site longitude as east-negative MeadeProtocol.hpp documents :Sg and :Gg as east-negative -- zero at Greenwich, negative coordinates going east. #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) Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz --- src/core/meade/MeadeParser.hpp | 9 +- src/core/meade/MeadeParserHelpers.cpp | 9 +- src/core/meade/MeadeParserSet.cpp | 60 ++++++- src/core/meade/MeadeProtocol.hpp | 5 +- unit_tests/test_core/meade/test_MeadeGet.cpp | 45 ++++- .../meade/test_MeadeParserHelpers.cpp | 21 ++- unit_tests/test_core/meade/test_MeadeSet.cpp | 162 +++++++++++++++++- 7 files changed, 287 insertions(+), 24 deletions(-) diff --git a/src/core/meade/MeadeParser.hpp b/src/core/meade/MeadeParser.hpp index 8f4739fe..fa2ad4ff 100644 --- a/src/core/meade/MeadeParser.hpp +++ b/src/core/meade/MeadeParser.hpp @@ -182,7 +182,14 @@ struct MeadeLatitude { bool negative; }; -/** @brief Site longitude: magnitude 0..180 in `degrees`, sign in `negative`. */ +/** @brief Site longitude: magnitude 0..180 in `degrees`, sign in `negative`. + * + * EAST-POSITIVE: `negative` means west of Greenwich. The Meade wire is the + * other way round -- :Sg/:Gg are east-negative -- so readLongitude and + * writeLongitude both flip the sign, and they have to stay in step. The + * convention is recorded here because the struct alone cannot show it, which + * is how a flip on both sides at once once went unnoticed. + */ struct MeadeLongitude { uint16_t degrees; uint8_t minutes; diff --git a/src/core/meade/MeadeParserHelpers.cpp b/src/core/meade/MeadeParserHelpers.cpp index 255ca864..3b1b1cb7 100644 --- a/src/core/meade/MeadeParserHelpers.cpp +++ b/src/core/meade/MeadeParserHelpers.cpp @@ -243,7 +243,14 @@ void writeLatitude(MeadeResponse &r, const MeadeLatitude &l) void writeLongitude(MeadeResponse &r, const MeadeLongitude &l) { - writeChar(r, l.negative ? '-' : '+'); + // :Gg is east-negative (MeadeProtocol.hpp), while MeadeLongitude is + // east-positive, so the sign flips on the way out. This has to move with + // readLongitude: if only one side flips, a client sets its site, reads it + // back mirrored, and pushes the mirror straight back on the next connect. + // Greenwich has no side, and a bare '-000*00' reads as a negative zero, so + // it goes out positive. + const bool atGreenwich = (l.degrees == 0) && (l.minutes == 0); + writeChar(r, (l.negative || atGreenwich) ? '+' : '-'); writeUnsignedPadded(r, l.degrees, 3); writeChar(r, '*'); writeUnsignedPadded(r, l.minutes, 2); diff --git a/src/core/meade/MeadeParserSet.cpp b/src/core/meade/MeadeParserSet.cpp index 7ad7d129..e774a2fa 100644 --- a/src/core/meade/MeadeParserSet.cpp +++ b/src/core/meade/MeadeParserSet.cpp @@ -79,18 +79,68 @@ bool readLatitude(Cursor &c, MeadeLatitude &out) return true; } -// Format: "[+-]DDDMM" where sep in {'*', ':'}. +// Unsigned :Sg is the legacy 0..360 count running WESTWARD from Greenwich. +// East-positive is what the mount stores, so negate modulo a full circle (which is +// what `fullCircle - arcminutes` is) and wrap into (-180, 180]. +// The tempting mistake is the other reflection, the one that lands Greenwich on 180 +// — `fullCircle / 2 - arcminutes`, which is what Longitude::ParseFromMeade computes. +// It turns a 121d53' west site into 58d07' east, exactly 180 degrees (12 hours of +// local sidereal time) from where it should be. +// A signed wire value negates the same way, so this is the single mapping for both +// forms: `arcminutes` is the westward count, positive or negative, and never more +// than one full circle from zero. +long westwardToEastPositiveArcminutes(long arcminutes) +{ + const long fullCircle = 360L * 60L; + long east = fullCircle - arcminutes; + while (east > fullCircle / 2) + { + east -= fullCircle; + } + return east; +} + +// Format: "[+-]?DDDMM" where sep in {'*', ':'}. +// +// The sign is optional. INDI omits it — ":Sg121*53#" goes on the wire for a site +// 121d53' WEST — so demanding one answers INDI's site push with "0" and the mount +// silently keeps whatever longitude it already had. +// +// Only the unsigned form is interpreted here. A signed value is passed through +// unchanged; which hemisphere its sign denotes is a separate question that this +// function deliberately does not answer. bool readLongitude(Cursor &c, MeadeLongitude &out) { + // The sign is optional, and both forms mean the same thing. MeadeProtocol.hpp + // documents :Sg/:Gg as east-negative, so the legacy unsigned 0..360 westward + // count that INDI sends is simply the sign == +1 case of the signed form: + // east = wrap(-value) either way, with no branch between them. int sign; + c.optionalSign(sign); + unsigned ddd, mm; - if (!readMandatorySign(c, sign) || !c.digits(3, ddd) || !c.matchIn("*:") || !c.digits(2, mm)) + if (!c.digits(3, ddd) || !c.matchIn("*:") || !c.digits(2, mm)) { return false; } - out.degrees = static_cast(ddd); - out.minutes = static_cast(mm); - out.negative = (sign < 0); + + // Reject anything outside one full circle, which nothing downstream does: + // core::Longitude(int, int, int) never calls checkHours(), and + // EEPROMStore::storeLongitude clamps degrees*100 into an int16, which destroys + // the mod-360 equivalence and persists a genuinely wrong site across reboots. + if ((ddd >= 360) || (mm >= 60)) + { + return false; + } + + const long westward = sign * ((static_cast(ddd) * 60L) + static_cast(mm)); + const long east = westwardToEastPositiveArcminutes(westward); + const bool isWest = (east < 0); + const long magnitude = isWest ? -east : east; + + out.degrees = static_cast(magnitude / 60); + out.minutes = static_cast(magnitude % 60); + out.negative = isWest; return true; } diff --git a/src/core/meade/MeadeProtocol.hpp b/src/core/meade/MeadeProtocol.hpp index b110ca0b..7584432a 100644 --- a/src/core/meade/MeadeProtocol.hpp +++ b/src/core/meade/MeadeProtocol.hpp @@ -154,6 +154,7 @@ // "MM" is the minutes // Remarks: // Note that this is the actual longitude, but east coordinates are negative (opposite of normal cartographic coordinates) +// This is the exact inverse of :Sg, and the two have to stay in step: flipping one alone makes a client read its own site back mirrored // // :Gc# // Description: @@ -339,8 +340,8 @@ // "DDD" is the number of degrees // "MM" is the minutes // Remarks: -// When a sign is provided, longitudes are interpreted as given, with zero at Greenwich but negative coordinates going east (opposite of normal cartographic coordinates) -// When a sign is not provided, longitudes are from 0 to 360 going WEST with 180 at Greenwich. So 369 is 179W and 1 is 179E. 190 would be 10W and 170 would be 10E. +// Longitudes are east-negative: zero at Greenwich, negative coordinates going east (opposite of normal cartographic coordinates) +// The unsigned form is the legacy count running WESTWARD from Greenwich, 0 to 359, which is the same mapping with the sign taken as '+'. So "121*53" is 121d53' west, "301*53" is 58d07' east, and "180*00" is the antimeridian. A full circle ("360*00") is refused rather than wrapped. // // :SGsHH# // Description: diff --git a/unit_tests/test_core/meade/test_MeadeGet.cpp b/unit_tests/test_core/meade/test_MeadeGet.cpp index 1a4f6994..e0d713fc 100644 --- a/unit_tests/test_core/meade/test_MeadeGet.cpp +++ b/unit_tests/test_core/meade/test_MeadeGet.cpp @@ -323,13 +323,16 @@ TEST(MeadeGet, site_latitude_signed_two_digit_deg) EXPECT_STREQ("-12*45#", dispatch("t", h)); } +// :Gg is east-negative and MeadeLongitude is east-positive, so the sign on the +// wire is the opposite of `negative`. This has to stay the exact inverse of +// readLongitude; see the round trips below. TEST(MeadeGet, site_longitude_signed_three_digit_deg) { FakeHandlers h; - h.longitude = {12, 30, false}; - EXPECT_STREQ("+012*30#", dispatch("g", h)); - h.longitude = {122, 45, true}; - EXPECT_STREQ("-122*45#", dispatch("g", h)); + h.longitude = {12, 30, false}; // 12d30' east + EXPECT_STREQ("-012*30#", dispatch("g", h)); + h.longitude = {122, 45, true}; // 122d45' west + EXPECT_STREQ("+122*45#", dispatch("g", h)); } // ---- Sign of zero ----------------------------------------------------- @@ -359,10 +362,10 @@ TEST(MeadeGet, site_latitude_zero_degrees_keeps_south_sign) TEST(MeadeGet, site_longitude_zero_degrees_keeps_sign) { FakeHandlers h; - h.longitude = {0, 5, true}; - EXPECT_STREQ("-000*05#", dispatch("g", h)); - h.longitude = {0, 5, false}; + h.longitude = {0, 5, true}; // 5' west EXPECT_STREQ("+000*05#", dispatch("g", h)); + h.longitude = {0, 5, false}; // 5' east + EXPECT_STREQ("-000*05#", dispatch("g", h)); } // ---- Set -> Get round trips ------------------------------------------- @@ -412,6 +415,34 @@ TEST(MeadeGet, site_longitude_round_trip_preserves_nonzero_degrees) EXPECT_STREQ("+097*34#", setThenGet("g+097*34", "g", h)); } +// The reader and the writer both flip the sign, so the wire value is unchanged +// by a round trip -- which is exactly why a flip on one side alone is invisible +// to a client and has to be caught by the struct-level assertions above. +TEST(MeadeGet, site_longitude_round_trip_is_unchanged_at_the_meridians) +{ + FakeHandlers h; + EXPECT_STREQ("+000*00#", setThenGet("g+000*00", "g", h)); + EXPECT_STREQ("+000*00#", setThenGet("g-000*00", "g", h)); + EXPECT_STREQ("-180*00#", setThenGet("g-180*00", "g", h)); +} + +// The form INDI actually sends: unsigned, counting westward. It comes back in +// the signed form, on the same meridian. +TEST(MeadeGet, site_longitude_unsigned_round_trips_to_the_same_meridian) +{ + FakeHandlers h; + EXPECT_STREQ("+121*53#", setThenGet("g121*53", "g", h)); + EXPECT_EQ(static_cast(121), h.longitude.degrees); + EXPECT_EQ(static_cast(53), h.longitude.minutes); + EXPECT_TRUE(h.longitude.negative); // west, east-positive internally + + FakeHandlers e; + EXPECT_STREQ("-058*07#", setThenGet("g301*53", "g", e)); + EXPECT_EQ(static_cast(58), e.longitude.degrees); + EXPECT_EQ(static_cast(7), e.longitude.minutes); + EXPECT_FALSE(e.longitude.negative); +} + TEST(MeadeGet, utc_offset_signs_and_pads) { FakeHandlers h; diff --git a/unit_tests/test_core/meade/test_MeadeParserHelpers.cpp b/unit_tests/test_core/meade/test_MeadeParserHelpers.cpp index b4259428..92368758 100644 --- a/unit_tests/test_core/meade/test_MeadeParserHelpers.cpp +++ b/unit_tests/test_core/meade/test_MeadeParserHelpers.cpp @@ -110,13 +110,28 @@ TEST(MeadeParserHelpers, write_latitude_emits_sign_for_zero_degrees) EXPECT_STREQ("-00*30#", bytes(r)); } -TEST(MeadeParserHelpers, write_longitude_pads_to_three_digits_and_keeps_sign) +// The struct is east-positive and the wire is east-negative, so the sign +// inverts on the way out: `negative` (west of Greenwich) emits '+'. +TEST(MeadeParserHelpers, write_longitude_pads_to_three_digits_and_inverts_the_sign) { meade::MeadeResponse r; writeLongitude(r, meade::MeadeLongitude {0, 5, true}); - EXPECT_STREQ("-000*05#", bytes(r)); + EXPECT_STREQ("+000*05#", bytes(r)); meade::MeadeResponse r2; writeLongitude(r2, meade::MeadeLongitude {122, 45, false}); - EXPECT_STREQ("+122*45#", bytes(r2)); + EXPECT_STREQ("-122*45#", bytes(r2)); +} + +// Greenwich is on neither side, and "-000*00" would read as a negative zero, +// so the zero meridian always goes out positive. +TEST(MeadeParserHelpers, write_longitude_emits_greenwich_as_positive) +{ + meade::MeadeResponse r; + writeLongitude(r, meade::MeadeLongitude {0, 0, false}); + EXPECT_STREQ("+000*00#", bytes(r)); + + meade::MeadeResponse r2; + writeLongitude(r2, meade::MeadeLongitude {0, 0, true}); + EXPECT_STREQ("+000*00#", bytes(r2)); } diff --git a/unit_tests/test_core/meade/test_MeadeSet.cpp b/unit_tests/test_core/meade/test_MeadeSet.cpp index 46054d99..f1fc0b4c 100644 --- a/unit_tests/test_core/meade/test_MeadeSet.cpp +++ b/unit_tests/test_core/meade/test_MeadeSet.cpp @@ -289,6 +289,8 @@ TEST(MeadeSet, site_latitude_malformed_does_not_call_handler) // ---- Site Longitude (g) ----------------------------------------------- +// :Sg is east-negative, so a '+' on the wire is a WEST longitude and reaches +// the east-positive struct as negative. TEST(MeadeSet, site_longitude_three_digit_degrees) { FakeHandlers h; @@ -296,7 +298,7 @@ TEST(MeadeSet, site_longitude_three_digit_degrees) EXPECT_STREQ("lon", h.lastCall); EXPECT_EQ(static_cast(97), h.lon.degrees); EXPECT_EQ(static_cast(34), h.lon.minutes); - EXPECT_FALSE(h.lon.negative); + EXPECT_TRUE(h.lon.negative); } TEST(MeadeSet, site_longitude_malformed_short_does_not_call_handler) @@ -306,6 +308,156 @@ TEST(MeadeSet, site_longitude_malformed_short_does_not_call_handler) EXPECT_EQ(nullptr, h.lastCall); } +// A '-' on the wire is an EAST longitude, which the east-positive struct holds +// as a positive value. #291 dropped the negation on both sides at once, so the +// convention round-tripped perfectly while being backwards; this assertion is +// on the struct rather than the wire so that a future flip cannot hide the +// same way. +TEST(MeadeSet, site_longitude_signed_negative_is_east) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g-121*53", h)); + EXPECT_STREQ("lon", h.lastCall); + EXPECT_EQ(static_cast(121), h.lon.degrees); + EXPECT_EQ(static_cast(53), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); +} + +// Unsigned longitudes count WESTWARD from Greenwich, 0..360, and are mirrored into +// the east-positive range the mount stores. INDI sends this form: a San Jose site +// at 121d53' west arrives as ":Sg121*53#" and must come back out as -121d53'. +TEST(MeadeSet, site_longitude_unsigned_west_of_greenwich) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g121*53", h)); + EXPECT_STREQ("lon", h.lastCall); + EXPECT_EQ(static_cast(121), h.lon.degrees); + EXPECT_EQ(static_cast(53), h.lon.minutes); + EXPECT_TRUE(h.lon.negative); +} + +// The unsigned form is not a second convention, it is the signed one with the +// sign taken as '+'. These two spellings of the same meridian must agree. +TEST(MeadeSet, site_longitude_unsigned_and_signed_agree) +{ + FakeHandlers u, s; + EXPECT_STREQ("1", dispatch("g121*53", u)); + EXPECT_STREQ("1", dispatch("g+121*53", s)); + EXPECT_EQ(u.lon.degrees, s.lon.degrees); + EXPECT_EQ(u.lon.minutes, s.lon.minutes); + EXPECT_EQ(u.lon.negative, s.lon.negative); +} + +// Past 180 the westward count has gone round to the eastern hemisphere. +TEST(MeadeSet, site_longitude_unsigned_east_of_greenwich) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g301*53", h)); + EXPECT_EQ(static_cast(58), h.lon.degrees); + EXPECT_EQ(static_cast(7), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); +} + +TEST(MeadeSet, site_longitude_unsigned_greenwich_is_zero) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g000*00", h)); + EXPECT_EQ(static_cast(0), h.lon.degrees); + EXPECT_EQ(static_cast(0), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); +} + +// 180 west and 180 east are the same meridian, so either sign would be right. This +// pins the half of the choice the parser makes — it wraps into (-180, 180], keeping +// the antimeridian positive — rather than leaving it for a reader to infer. +TEST(MeadeSet, site_longitude_unsigned_antimeridian_stays_positive) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g180*00", h)); + EXPECT_EQ(static_cast(180), h.lon.degrees); + EXPECT_EQ(static_cast(0), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); +} + +// Top of the accepted range: one arcminute short of a full circle west is one +// arcminute east. +TEST(MeadeSet, site_longitude_unsigned_upper_bound_wraps_to_east) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g359*59", h)); + EXPECT_EQ(static_cast(0), h.lon.degrees); + EXPECT_EQ(static_cast(1), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); +} + +// A west longitude smaller than one degree used to be unrepresentable: the sign +// lived in `degrees`, which is 0 here, so "000*30" (30' WEST) and "359*30" (30' +// east) both came out {0, 30} and were read downstream as 30' EAST -- a silent +// 1-degree error with a "1" reply. The separate `negative` field is what tells +// the two apart. +TEST(MeadeSet, site_longitude_unsigned_sub_degree_west_keeps_its_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g000*30", h)); + EXPECT_STREQ("lon", h.lastCall); + EXPECT_EQ(static_cast(0), h.lon.degrees); + EXPECT_EQ(static_cast(30), h.lon.minutes); + EXPECT_TRUE(h.lon.negative); + + // The east neighbour it used to collide with. + FakeHandlers e; + EXPECT_STREQ("1", dispatch("g359*30", e)); + EXPECT_EQ(static_cast(0), e.lon.degrees); + EXPECT_EQ(static_cast(30), e.lon.minutes); + EXPECT_FALSE(e.lon.negative); +} + +// 360 west is the same meridian as 000, but it is refused rather than wrapped: the +// range check is what stops out-of-circle degrees reaching EEPROMStore, which clamps +// them into an int16 and persists a site that is wrong rather than merely unwrapped. +TEST(MeadeSet, site_longitude_unsigned_full_circle_is_rejected) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("g360*00", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + +// The range check is shared, so it guards the signed path too. +TEST(MeadeSet, site_longitude_signed_out_of_range_does_not_call_handler) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("g+400*00", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + +TEST(MeadeSet, site_longitude_minutes_out_of_range_does_not_call_handler) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("g+121*99", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + +// Degrees this far out would also push the westward arcminute count past INT16_MAX, +// which is why the conversion works in `long` as well as rejecting the input. +TEST(MeadeSet, site_longitude_unsigned_beyond_int16_arcminutes_is_rejected) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("g545*69", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + +// Two-digit degrees are refused on both paths. Pre-#291 DayTime::ParseFromMeade took +// two or three, so this is stricter than the legacy parser for a client that sends +// ":Sg97*34#"; nothing observed on the wire does, MeadeProtocol.hpp documents "DDD", +// and Cursor never backtracks, so accepting either width means hand-rolling the digit +// reads. Relaxing it should relax the signed path at the same time. +TEST(MeadeSet, site_longitude_unsigned_two_digit_degrees_does_not_call_handler) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("g97*34", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + // ---- UTC Offset (G) --------------------------------------------------- TEST(MeadeSet, utc_offset_positive) @@ -430,19 +582,19 @@ TEST(MeadeSet, site_latitude_positive_zero_degrees_keeps_sign) TEST(MeadeSet, site_longitude_negative_zero_degrees_keeps_sign) { FakeHandlers h; - EXPECT_STREQ("1", dispatch("g-000*05", h)); + EXPECT_STREQ("1", dispatch("g-000*05", h)); // 5' EAST on an east-negative wire EXPECT_EQ(static_cast(0), h.lon.degrees); EXPECT_EQ(static_cast(5), h.lon.minutes); - EXPECT_TRUE(h.lon.negative); + EXPECT_FALSE(h.lon.negative); } TEST(MeadeSet, site_longitude_positive_zero_degrees_keeps_sign) { FakeHandlers h; - EXPECT_STREQ("1", dispatch("g+000*05", h)); + EXPECT_STREQ("1", dispatch("g+000*05", h)); // 5' WEST EXPECT_EQ(static_cast(0), h.lon.degrees); EXPECT_EQ(static_cast(5), h.lon.minutes); - EXPECT_FALSE(h.lon.negative); + EXPECT_TRUE(h.lon.negative); } TEST(MeadeSet, utc_offset_negative_zero_is_zero)