diff --git a/src/core/meade/MeadeParserSet.cpp b/src/core/meade/MeadeParserSet.cpp index e774a2fa..b9cbee91 100644 --- a/src/core/meade/MeadeParserSet.cpp +++ b/src/core/meade/MeadeParserSet.cpp @@ -144,6 +144,40 @@ bool readLongitude(Cursor &c, MeadeLongitude &out) return true; } +// Format: "[+-]H[H]", with anything after the hours left unconsumed, so the +// ":SG+7.0#" that INDI sends on connect parses as +7 -- as it did before the +// refactor, when the handler ran toInt() over "+7.". Requiring a sign and +// exactly two digits makes INDI's UTC-offset push fail with "0" and leaves the +// site offset unset. +bool readUtcOffset(Cursor &c, int &out) +{ + // The sign is required, as MeadeProtocol.hpp specifies -- an unsigned offset + // would be indistinguishable from a client that forgot the sign, and half of + // those are wrong by twice the offset. + int sign; + if (!readMandatorySign(c, sign)) + { + return false; + } + + // One or two digits, unlike the rest of the Set family: INDI sends ":SG+7.0#". + unsigned hh; + if (!c.digits(1, hh)) + { + return false; + } + unsigned secondDigit; + if (c.digits(1, secondDigit)) + { + hh = hh * 10 + secondDigit; + } + + // Anything after the hours is left unconsumed; a fractional offset is read as + // its whole-hour part until the stored offset can hold minutes. + out = sign * static_cast(hh); + return true; +} + // Set ack: "1" on success, "0" on failure. No framing terminator. void writeSetAck(MeadeResponse &r, bool ok) { @@ -290,15 +324,14 @@ void handleMeadeSet(MeadeResponse &r, const char *s, IMeadeSetHandlers &h) case 'G': { - // G
- int sign; - unsigned hours; - if (!readMandatorySign(c, sign) || !c.digits(2, hours)) + // G + int hours; + if (!readUtcOffset(c, hours)) { writeChar(r, '0'); return; } - writeSetAck(r, h.onSetUtcOffset(sign * static_cast(hours))); + writeSetAck(r, h.onSetUtcOffset(hours)); return; } diff --git a/src/core/meade/MeadeProtocol.hpp b/src/core/meade/MeadeProtocol.hpp index 7584432a..ec60086b 100644 --- a/src/core/meade/MeadeProtocol.hpp +++ b/src/core/meade/MeadeProtocol.hpp @@ -343,16 +343,20 @@ // 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# +// :SGsHH.H# // Description: // Set Site UTC Offset // Information: // This sets the offset of the timezone in which the mount is in hours from UTC. // Returns: -// "1" +// "1" if successfully set +// "0" otherwise // Parameters: -// "s" is the sign -// "HH" is the number of hours +// "s" is the sign, and is required +// "HH" is the number of hours, one or two digits +// Remarks: +// The LX200 spec (Revision 2010.10) defines this as ":SGsHH.H#" -- a sign, two-digit hours, one decimal. The single-digit and fractional forms INDI sends are a deviation from it; accepting them is a compatibility choice, not a correctness one. +// Anything following the hours is ignored, so the ":SG+7.0#" that INDI sends is read as +7. The offset is whole hours only, so half-hour zones such as India and Newfoundland cannot yet be expressed; the stored offset is a single signed byte with no unit tag, so widening it is a storage change rather than a parser one. // // :SLHH:MM:SS# // Description: diff --git a/unit_tests/test_core/meade/test_MeadeSet.cpp b/unit_tests/test_core/meade/test_MeadeSet.cpp index f1fc0b4c..00c53e0d 100644 --- a/unit_tests/test_core/meade/test_MeadeSet.cpp +++ b/unit_tests/test_core/meade/test_MeadeSet.cpp @@ -475,10 +475,83 @@ TEST(MeadeSet, utc_offset_negative) EXPECT_EQ(-8, h.utc); } -TEST(MeadeSet, utc_offset_malformed_length_does_not_call_handler) +// The exact bytes INDI puts on the wire when it pushes the site on connect. +TEST(MeadeSet, utc_offset_indi_fractional_form) { FakeHandlers h; - EXPECT_STREQ("0", dispatch("G+5", h)); + EXPECT_STREQ("1", dispatch("G+7.0", h)); + EXPECT_STREQ("utc", h.lastCall); + EXPECT_EQ(7, h.utc); +} + +TEST(MeadeSet, utc_offset_single_digit_positive) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("G+5", h)); + EXPECT_EQ(5, h.utc); +} + +TEST(MeadeSet, utc_offset_single_digit_negative) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("G-3", h)); + EXPECT_EQ(-3, h.utc); +} + +// The sign is required. Pre-#291 the offset went through String::toInt(), which +// accepts an unsigned value, so this is a narrowing rather than a restoration -- +// but a missing sign is far more likely a client bug than a deliberate "+", and +// guessing wrong puts local sidereal time out by twice the offset. +TEST(MeadeSet, utc_offset_unsigned_is_rejected) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("G07", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + +// Half-hour zones (India, Newfoundland) are unrepresentable: onSetUtcOffset takes +// whole hours, so ".5" is left unconsumed and the site lands 30 minutes out. +// Pinned here so the limitation is documented rather than discovered in the field. +TEST(MeadeSet, utc_offset_half_hour_zone_drops_the_fraction) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("G+5.5", h)); + EXPECT_EQ(5, h.utc); +} + +// Nothing range-checks the hours. "+13" is a real offset (Tonga); "-15" is not, +// and is taken all the same. Both pin the current permissive behaviour -- +// whether to reject impossible offsets is deliberately left to a follow-up. +// Note that IMeadeSetHandlers::onSetUtcOffset documents "@param hours Signed +// wire value (-12..+14)"; that range is stated but has never been enforced, +// here or before this parser accepted the unsigned and single-digit forms. +TEST(MeadeSet, utc_offset_two_digit_high_value) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("G+13", h)); + EXPECT_STREQ("utc", h.lastCall); + EXPECT_EQ(13, h.utc); +} + +TEST(MeadeSet, utc_offset_impossible_value_is_accepted) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("G-15", h)); + EXPECT_STREQ("utc", h.lastCall); + EXPECT_EQ(-15, h.utc); +} + +TEST(MeadeSet, utc_offset_sign_without_digits_does_not_call_handler) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("G+", h)); + EXPECT_EQ(nullptr, h.lastCall); +} + +TEST(MeadeSet, utc_offset_non_numeric_does_not_call_handler) +{ + FakeHandlers h; + EXPECT_STREQ("0", dispatch("Gx", h)); EXPECT_EQ(nullptr, h.lastCall); }