diff --git a/src/core/meade/MeadeParserSet.cpp b/src/core/meade/MeadeParserSet.cpp index 909624a8..51f5ebb8 100644 --- a/src/core/meade/MeadeParserSet.cpp +++ b/src/core/meade/MeadeParserSet.cpp @@ -62,15 +62,89 @@ 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. +// `arcminutes` is the wire value, so at most one full circle. +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) { + const bool hasSign = (c.peek() == '+') || (c.peek() == '-'); + int deg; unsigned mm; - if (!c.signed3(deg) || !c.matchIn("*:") || !c.digits(2, mm)) + if (hasSign) + { + if (!c.signed3(deg)) + { + return false; + } + } + else + { + // Exactly three digits, matching the signed path and the "DDD" that + // MeadeProtocol.hpp documents. Pre-#291 DayTime::ParseFromMeade also took + // two; widening that again belongs with the signed path, not here. + unsigned ddd; + if (!c.digits(3, ddd)) + { + return false; + } + deg = static_cast(ddd); + } + if (!c.matchIn("*:") || !c.digits(2, mm)) { return false; } + + // 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. + // This is not a -180..180 check — a signed +200 still passes. + if ((deg >= 360) || (deg <= -360) || (mm >= 60)) + { + return false; + } + + if (!hasSign) + { + const long east = westwardToEastPositiveArcminutes(deg * 60L + static_cast(mm)); + const bool west = (east < 0); + const long magnitude = west ? -east : east; + const long degrees = magnitude / 60; + + // KNOWN LIMITATION: MeadeLongitude carries the sign only in `degrees`, so a + // west longitude of under one degree (0d01' through 0d59' west) loses it + // here — `degrees` is 0, and the arcminutes end up stored as if east. The fix + // is to let the struct hold the sign separately; that is not this change. + deg = static_cast(west ? -degrees : degrees); + mm = static_cast(magnitude % 60); + } + out.degrees = static_cast(deg); out.minutes = static_cast(mm); return true; diff --git a/src/core/meade/MeadeProtocol.hpp b/src/core/meade/MeadeProtocol.hpp index b110ca0b..2158530b 100644 --- a/src/core/meade/MeadeProtocol.hpp +++ b/src/core/meade/MeadeProtocol.hpp @@ -340,7 +340,7 @@ // "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. +// When a sign is not provided, the value is the legacy count running WESTWARD from Greenwich, 0 to 359. 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_MeadeSet.cpp b/unit_tests/test_core/meade/test_MeadeSet.cpp index 7c0074ff..35974606 100644 --- a/unit_tests/test_core/meade/test_MeadeSet.cpp +++ b/unit_tests/test_core/meade/test_MeadeSet.cpp @@ -301,6 +301,139 @@ TEST(MeadeSet, site_longitude_malformed_short_does_not_call_handler) EXPECT_EQ(nullptr, h.lastCall); } +// Pins the signed path exactly as #291 left it: the value is passed through, so a +// '-' reaches the mount as a west longitude. Whether that is the right reading of +// the sign is disputed (see :Sg in MeadeProtocol.hpp, whose Remarks still say a +// negative sign means east) and is not settled by these tests — this case exists so +// that any later change to the sign convention has to move a green assertion. +TEST(MeadeSet, site_longitude_signed_negative_passes_through_unchanged) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g-121*53", h)); + EXPECT_STREQ("lon", h.lastCall); + EXPECT_EQ(-121, h.lon.degrees); + EXPECT_EQ(static_cast(53), h.lon.minutes); +} + +// 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(-121, h.lon.degrees); + EXPECT_EQ(static_cast(53), h.lon.minutes); +} + +// 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(58, h.lon.degrees); + EXPECT_EQ(static_cast(7), h.lon.minutes); +} + +TEST(MeadeSet, site_longitude_unsigned_greenwich_is_zero) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g000*00", h)); + EXPECT_EQ(0, h.lon.degrees); + EXPECT_EQ(static_cast(0), h.lon.minutes); +} + +// 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(180, h.lon.degrees); + EXPECT_EQ(static_cast(0), h.lon.minutes); +} + +// 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(0, h.lon.degrees); + EXPECT_EQ(static_cast(1), h.lon.minutes); +} + +// KNOWN LIMITATION, pinned so that fixing it has to move a green assertion. +// MeadeLongitude carries the sign only in `degrees`, so a west longitude smaller +// than one degree cannot be represented: "000*30" (30' WEST) and "359*30" (30' east) +// both come out {0, 30}, and downstream core::DayTime(0, 30, 0) reads that as 30' +// EAST. The wire reply is still "1", so the error is silent. It is a sign flip, not a +// truncation, so the worst case is "000*59": stored 118' — nearly 2 degrees, about +// 7.9 minutes of local sidereal time — from the truth. +// The fix is to let the struct hold the sign separately. +TEST(MeadeSet, site_longitude_unsigned_sub_degree_west_loses_its_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g000*30", h)); + EXPECT_STREQ("lon", h.lastCall); + EXPECT_EQ(0, h.lon.degrees); + EXPECT_EQ(static_cast(30), h.lon.minutes); + + // The genuinely-east neighbour that collides with it. + FakeHandlers e; + EXPECT_STREQ("1", dispatch("g359*30", e)); + EXPECT_EQ(0, e.lon.degrees); + EXPECT_EQ(static_cast(30), e.lon.minutes); +} + +// 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)