From 68ace3945075903eb512434d209dab6d78053ee3 Mon Sep 17 00:00:00 2001 From: Kiryl Date: Sun, 6 Sep 2026 20:06:49 -0700 Subject: [PATCH 1/2] fix(meade): keep the sign of coordinates whose degrees component is zero Regression from #291. DecCoordinate, MeadeLatitude and MeadeLongitude carried the sign in the sign bit of `degrees`, which cannot represent a negative value whose degrees component is zero. Cursor::signed2() computes -(int)0, which is 0, so the sign was destroyed inside the struct before any handler saw it: :Sd-00*30:00# -> {0, 30, 0} sets +00*30:00 :St-00*30# -> {0, 30} equatorial sites :Sg-000*05# -> {0, 5} central London A one-degree error in a band straddling the celestial equator, and :CM sync writes it into the mount's home reference permanently. The pre-#291 DayTime::ParseFromMeade applied the sign to the whole total and was correct. Replace signed2/signed3 with Cursor::optionalSign(), which reports the sign without folding it into a magnitude, and give the three structs an explicit `negative` field. The readers keep sign and magnitude apart to the end, and the writers and the MeadeCommandProcessor boundary read the sign off the undivided total rather than off a divided degrees component. The accepted grammar is byte-for-byte unchanged -- readMandatorySign() preserves the existing requirement for an explicit sign, so this commit changes only what the parser does with a sign it already accepted. This supersedes #241, which diagnosed the same root cause and proposed the same remedy of carrying the sign as its own channel. Its two target functions, Longitude::formatString() and Longitude::formatStringForMeade(), have had no callers since #291 routed around them, so the idea is applied here where the code now lives. Co-authored-by: Claude --- src/MeadeCommandProcessor.cpp | 39 +++- src/core/meade/MeadeParser.hpp | 22 ++- src/core/meade/MeadeParserHelpers.cpp | 64 ++----- src/core/meade/MeadeParserHelpers.hpp | 21 +- src/core/meade/MeadeParserSet.cpp | 57 ++++-- unit_tests/test_core/meade/test_MeadeGet.cpp | 179 +++++++++++++++++- .../meade/test_MeadeParserHelpers.cpp | 122 ++++++++++++ unit_tests/test_core/meade/test_MeadeSet.cpp | 96 +++++++++- unit_tests/test_core/types/test_latitude.cpp | 12 ++ unit_tests/test_core/types/test_longitude.cpp | 11 ++ 10 files changed, 525 insertions(+), 98 deletions(-) create mode 100644 unit_tests/test_core/meade/test_MeadeParserHelpers.cpp diff --git a/src/MeadeCommandProcessor.cpp b/src/MeadeCommandProcessor.cpp index ecb4d531..75eb3a42 100644 --- a/src/MeadeCommandProcessor.cpp +++ b/src/MeadeCommandProcessor.cpp @@ -104,16 +104,33 @@ meade::DecCoordinate decFrom(const Declination &d) // correction before splitting into components. int deg, min, sec; d.getCelestialDegrees(deg, min, sec); + // getCelestialDegrees folds the sign into `deg`, where anything between 0 + // and -1 degrees comes back as +0. Read the sign off the undivided total. + const long celestialSeconds = Declination::axisToCelestialSeconds(d.getTotalSeconds(), inNorthernHemisphere); return meade::DecCoordinate { - static_cast(deg), + static_cast(deg < 0 ? -deg : deg), static_cast(min), static_cast(sec), + celestialSeconds < 0, }; } Declination decFromWire(meade::DecCoordinate const &d) { - return Declination::fromCelestialDegrees(d.degrees, d.minutes, d.seconds); + // fromCelestialDegrees carries the sign in its `deg` parameter, so a + // coordinate such as "-00*30:00" still arrives there unsigned. The parser + // keeps sign and magnitude apart up to this call. + const int degrees = d.negative ? -static_cast(d.degrees) : static_cast(d.degrees); + return Declination::fromCelestialDegrees(degrees, d.minutes, d.seconds); +} + +// Signed arc-seconds for a magnitude/sign pair. The Latitude and Longitude +// constructors take signed degrees, which cannot express a site between 0 and +// -1 degree, so callers add this total to a zeroed coordinate instead. +long siteSecondsFrom(uint16_t degrees, uint8_t minutes, bool negative) +{ + const long seconds = ((static_cast(degrees) * 60L) + minutes) * 60L; + return negative ? -seconds : seconds; } } // namespace @@ -161,18 +178,24 @@ bool MeadeCommandProcessor::onIsGuiding() meade::MeadeLatitude MeadeCommandProcessor::onSiteLatitude() { const Latitude lat = _mount->latitude(); + // getHours() folds the sign into the degrees component, so a site between + // 0 and -1 degrees reports as +0. Read the sign off the total instead. + const int degrees = lat.getHours(); return meade::MeadeLatitude { - static_cast(lat.getHours()), + static_cast(degrees < 0 ? -degrees : degrees), static_cast(lat.getMinutes()), + lat.getTotalSeconds() < 0, }; } meade::MeadeLongitude MeadeCommandProcessor::onSiteLongitude() { const Longitude lon = _mount->longitude(); + const int degrees = lon.getHours(); return meade::MeadeLongitude { - static_cast(lon.getHours()), + static_cast(degrees < 0 ? -degrees : degrees), static_cast(lon.getMinutes()), + lon.getTotalSeconds() < 0, }; } @@ -300,13 +323,17 @@ bool MeadeCommandProcessor::onSyncCoordinates(meade::DecCoordinate dec, meade::R bool MeadeCommandProcessor::onSetSiteLatitude(meade::MeadeLatitude lat) { - _mount->setLatitude(Latitude(static_cast(lat.degrees), static_cast(lat.minutes), 0)); + Latitude value; + value.addSeconds(siteSecondsFrom(lat.degrees, lat.minutes, lat.negative)); + _mount->setLatitude(value); return true; } bool MeadeCommandProcessor::onSetSiteLongitude(meade::MeadeLongitude lon) { - _mount->setLongitude(Longitude(static_cast(lon.degrees), static_cast(lon.minutes), 0)); + Longitude value; + value.addSeconds(siteSecondsFrom(lon.degrees, lon.minutes, lon.negative)); + _mount->setLongitude(value); return true; } diff --git a/src/core/meade/MeadeParser.hpp b/src/core/meade/MeadeParser.hpp index 4f998ea8..8f4739fe 100644 --- a/src/core/meade/MeadeParser.hpp +++ b/src/core/meade/MeadeParser.hpp @@ -160,23 +160,33 @@ struct RaCoordinate { uint8_t seconds; }; -/** @brief Declination coordinate; `degrees` carries the sign (-180..180). */ +/** + * @brief Declination coordinate: unsigned magnitude plus a separate sign. + * + * The sign is a field of its own rather than the sign bit of `degrees` + * because the Meade wire format has coordinates such as `-00*30:00` whose + * degrees component is zero; folding the sign into `degrees` would round + * those to `+00*30:00`, a one-degree error either side of the equator. + */ struct DecCoordinate { - int16_t degrees; + uint16_t degrees; ///< Magnitude only, 0..180. uint8_t minutes; uint8_t seconds; + bool negative; }; -/** @brief Site latitude; `degrees` is signed (-90..90). */ +/** @brief Site latitude: magnitude 0..90 in `degrees`, sign in `negative`. */ struct MeadeLatitude { - int16_t degrees; + uint16_t degrees; uint8_t minutes; + bool negative; }; -/** @brief Site longitude; `degrees` is signed (-180..180). */ +/** @brief Site longitude: magnitude 0..180 in `degrees`, sign in `negative`. */ struct MeadeLongitude { - int16_t degrees; + uint16_t degrees; uint8_t minutes; + bool negative; }; /** @brief Wall-clock time (24h). The parser handles 12h conversion for `:Ga#`. */ diff --git a/src/core/meade/MeadeParserHelpers.cpp b/src/core/meade/MeadeParserHelpers.cpp index 024a1dbf..255ca864 100644 --- a/src/core/meade/MeadeParserHelpers.cpp +++ b/src/core/meade/MeadeParserHelpers.cpp @@ -64,6 +64,14 @@ bool Cursor::matchIn(const char *set) return false; } +void Cursor::advance() +{ + if (*_p != '\0') + { + ++_p; + } +} + bool Cursor::digits(int n, unsigned &out) { unsigned v = 0; @@ -79,29 +87,14 @@ bool Cursor::digits(int n, unsigned &out) return true; } -bool Cursor::signed2(int &out) -{ - char sign = peek(); - if (sign != '+' && sign != '-') - return false; - ++_p; - unsigned v = 0; - if (!digits(2, v)) - return false; - out = (sign == '-') ? -static_cast(v) : static_cast(v); - return true; -} - -bool Cursor::signed3(int &out) +bool Cursor::optionalSign(int &sign) { - char sign = peek(); - if (sign != '+' && sign != '-') - return false; - ++_p; - unsigned v = 0; - if (!digits(3, v)) - return false; - out = (sign == '-') ? -static_cast(v) : static_cast(v); + const char c = peek(); + sign = (c == '-') ? -1 : 1; + if ((c == '+') || (c == '-')) + { + advance(); + } return true; } @@ -230,13 +223,8 @@ void writeRa(MeadeResponse &r, const RaCoordinate &ra) void writeDec(MeadeResponse &r, const DecCoordinate &d) { - int deg = d.degrees; - writeChar(r, deg < 0 ? '-' : '+'); - if (deg < 0) - { - deg = -deg; - } - writeUnsignedPadded(r, static_cast(deg), 2); + writeChar(r, d.negative ? '-' : '+'); + writeUnsignedPadded(r, d.degrees, 2); writeChar(r, '*'); writeUnsignedPadded(r, d.minutes, 2); writeChar(r, '\''); @@ -246,13 +234,8 @@ void writeDec(MeadeResponse &r, const DecCoordinate &d) void writeLatitude(MeadeResponse &r, const MeadeLatitude &l) { - int deg = l.degrees; - writeChar(r, deg < 0 ? '-' : '+'); - if (deg < 0) - { - deg = -deg; - } - writeUnsignedPadded(r, static_cast(deg), 2); + writeChar(r, l.negative ? '-' : '+'); + writeUnsignedPadded(r, l.degrees, 2); writeChar(r, '*'); writeUnsignedPadded(r, l.minutes, 2); writeTerminator(r); @@ -260,13 +243,8 @@ void writeLatitude(MeadeResponse &r, const MeadeLatitude &l) void writeLongitude(MeadeResponse &r, const MeadeLongitude &l) { - int deg = l.degrees; - writeChar(r, deg < 0 ? '-' : '+'); - if (deg < 0) - { - deg = -deg; - } - writeUnsignedPadded(r, static_cast(deg), 3); + writeChar(r, l.negative ? '-' : '+'); + writeUnsignedPadded(r, l.degrees, 3); writeChar(r, '*'); writeUnsignedPadded(r, l.minutes, 2); writeTerminator(r); diff --git a/src/core/meade/MeadeParserHelpers.hpp b/src/core/meade/MeadeParserHelpers.hpp index 137d6de8..60eb55e3 100644 --- a/src/core/meade/MeadeParserHelpers.hpp +++ b/src/core/meade/MeadeParserHelpers.hpp @@ -23,9 +23,11 @@ namespace meade // --------------------------------------------------------------------------- // Cursor — single-pass input cursor with small grammar primitives // -// Forward-only; never backtracks. Each primitive returns `false` on mismatch -// (cursor is advanced on success). Ideal for fixed-format Meade sub-commands -// like coordinates, times, and dates. +// Forward-only; never backtracks. The matching primitives return `false` on +// mismatch and advance only on success. The two unconditional ones are the +// exception: `advance` returns nothing and `optionalSign` always returns +// `true`. Ideal for fixed-format Meade sub-commands like coordinates, times, +// and dates. // --------------------------------------------------------------------------- class Cursor @@ -43,14 +45,17 @@ class Cursor /// Consume one character if it is any of the chars in `set`. bool matchIn(const char *set); + /// Consume one character unconditionally; a no-op at end of input. + void advance(); + /// Read exactly `n` decimal digits into `out` (big-endian, no separators). bool digits(int n, unsigned &out); - /// Read "+DD" or "-DD" into a signed int. - bool signed2(int &out); - - /// Read "+DDD" or "-DDD" into a signed int. - bool signed3(int &out); + /// Consume a leading '+' or '-' if present and report it in `sign` as -1 + /// or +1 (+1 when absent). Always succeeds — callers that require a sign + /// check `peek()` first. Keeping the sign out of the magnitude is what + /// lets "-00" survive; a signed magnitude cannot hold it. + bool optionalSign(int &sign); private: const char *_p; diff --git a/src/core/meade/MeadeParserSet.cpp b/src/core/meade/MeadeParserSet.cpp index 909624a8..7ad7d129 100644 --- a/src/core/meade/MeadeParserSet.cpp +++ b/src/core/meade/MeadeParserSet.cpp @@ -19,18 +19,34 @@ namespace meade namespace { +// The readers below have always required an explicit sign, and this keeps +// that grammar byte-for-byte unchanged. It is a description of the parser as +// it stands, not of the protocol: MeadeProtocol.hpp documents the sign as +// optional for `:Sg`, where an unsigned value means 0..360 going westward. +// That form is rejected here, exactly as it was before this change. +bool readMandatorySign(Cursor &c, int &sign) +{ + const char first = c.peek(); + if ((first != '+') && (first != '-')) + { + return false; + } + return c.optionalSign(sign); +} + // Format: "[+-]DDMM:SS" where sep in {'*', ':'}. bool readDecCoordinate(Cursor &c, DecCoordinate &out) { - int deg; - unsigned mm, ss; - if (!c.signed2(deg) || !c.matchIn("*:") || !c.digits(2, mm) || !c.match(':') || !c.digits(2, ss)) + int sign; + unsigned dd, mm, ss; + if (!readMandatorySign(c, sign) || !c.digits(2, dd) || !c.matchIn("*:") || !c.digits(2, mm) || !c.match(':') || !c.digits(2, ss)) { return false; } - out.degrees = static_cast(deg); - out.minutes = static_cast(mm); - out.seconds = static_cast(ss); + out.degrees = static_cast(dd); + out.minutes = static_cast(mm); + out.seconds = static_cast(ss); + out.negative = (sign < 0); return true; } @@ -51,28 +67,30 @@ bool readRaCoordinate(Cursor &c, RaCoordinate &out) // Format: "[+-]DDMM" where sep in {'*', ':'}. bool readLatitude(Cursor &c, MeadeLatitude &out) { - int deg; - unsigned mm; - if (!c.signed2(deg) || !c.matchIn("*:") || !c.digits(2, mm)) + int sign; + unsigned dd, mm; + if (!readMandatorySign(c, sign) || !c.digits(2, dd) || !c.matchIn("*:") || !c.digits(2, mm)) { return false; } - out.degrees = static_cast(deg); - out.minutes = static_cast(mm); + out.degrees = static_cast(dd); + out.minutes = static_cast(mm); + out.negative = (sign < 0); return true; } // Format: "[+-]DDDMM" where sep in {'*', ':'}. bool readLongitude(Cursor &c, MeadeLongitude &out) { - int deg; - unsigned mm; - if (!c.signed3(deg) || !c.matchIn("*:") || !c.digits(2, mm)) + int sign; + unsigned ddd, mm; + if (!readMandatorySign(c, sign) || !c.digits(3, ddd) || !c.matchIn("*:") || !c.digits(2, mm)) { return false; } - out.degrees = static_cast(deg); - out.minutes = static_cast(mm); + out.degrees = static_cast(ddd); + out.minutes = static_cast(mm); + out.negative = (sign < 0); return true; } @@ -223,13 +241,14 @@ void handleMeadeSet(MeadeResponse &r, const char *s, IMeadeSetHandlers &h) case 'G': { // G
- int hours; - if (!c.signed2(hours)) + int sign; + unsigned hours; + if (!readMandatorySign(c, sign) || !c.digits(2, hours)) { writeChar(r, '0'); return; } - writeSetAck(r, h.onSetUtcOffset(hours)); + writeSetAck(r, h.onSetUtcOffset(sign * static_cast(hours))); return; } diff --git a/unit_tests/test_core/meade/test_MeadeGet.cpp b/unit_tests/test_core/meade/test_MeadeGet.cpp index 70e1c142..1a4f6994 100644 --- a/unit_tests/test_core/meade/test_MeadeGet.cpp +++ b/unit_tests/test_core/meade/test_MeadeGet.cpp @@ -29,13 +29,13 @@ class FakeHandlers : public meade::IMeadeGetHandlers meade::RaCoordinate currentRa = {1, 2, 3}; meade::RaCoordinate targetRa = {4, 5, 6}; - meade::DecCoordinate currentDec = {7, 8, 9}; - meade::DecCoordinate targetDec = {-10, 11, 12}; + meade::DecCoordinate currentDec = {7, 8, 9, false}; + meade::DecCoordinate targetDec = {10, 11, 12, true}; bool isSlewing = false; bool isTracking = true; bool isGuiding = false; - meade::MeadeLatitude latitude = {47, 30}; - meade::MeadeLongitude longitude = {-12, 30}; + meade::MeadeLatitude latitude = {47, 30, false}; + meade::MeadeLongitude longitude = {12, 30, true}; int utcOffset = -5; meade::MeadeLocalTime localTime = {14, 45, 6}; meade::MeadeLocalDate localDate = {3, 7, 2024}; @@ -153,6 +153,85 @@ const char *dispatch(const char *suffix, FakeHandlers &h) return last.c_str(); } +// Pipes the values the Set family parses back into the Get family's fake, so +// a test can drive `:S...` and read the result out through `:G...`. +// +// This joins the two parser families and nothing else. A real client's bytes +// also pass through MeadeCommandProcessor and the Declination / Latitude / +// Longitude types, which no native test reaches; passing here does not mean +// the mount stores what was sent. +class RoundTripSetHandlers : public meade::IMeadeSetHandlers +{ + public: + explicit RoundTripSetHandlers(FakeHandlers &sink) : _sink(sink) + { + } + + bool onSetTargetDec(meade::DecCoordinate v) override + { + _sink.currentDec = v; + _sink.targetDec = v; + return true; + } + bool onSetSiteLatitude(meade::MeadeLatitude v) override + { + _sink.latitude = v; + return true; + } + bool onSetSiteLongitude(meade::MeadeLongitude v) override + { + _sink.longitude = v; + return true; + } + + bool onSetTargetRa(meade::RaCoordinate) override + { + return true; + } + bool onSetLocalSiderealTime(meade::MeadeLocalTime) override + { + return true; + } + bool onSetHomePoint() override + { + return true; + } + bool onSetHourAngle(uint8_t, uint8_t) override + { + return true; + } + bool onSyncCoordinates(meade::DecCoordinate, meade::RaCoordinate) override + { + return true; + } + bool onSetUtcOffset(int) override + { + return true; + } + bool onSetLocalTime(meade::MeadeLocalTime) override + { + return true; + } + bool onSetLocalDate(meade::MeadeLocalDate) override + { + return true; + } + + private: + FakeHandlers &_sink; +}; + +// Runs one `:S...` suffix through the Set dispatcher into `h`, asserting the +// "1" ack, then returns the bytes the matching `:G...` suffix emits. +const char *setThenGet(const char *setSuffix, const char *getSuffix, FakeHandlers &h) +{ + meade::MeadeResponse ack; + RoundTripSetHandlers sink(h); + meade::handleMeadeSet(ack, setSuffix, sink); + EXPECT_STREQ("1", ack.c_str()); + return dispatch(getSuffix, h); +} + } // namespace TEST(MeadeGet, firmware_version_two_char_command) @@ -188,7 +267,7 @@ TEST(MeadeGet, target_ra_formats_hh_mm_ss) TEST(MeadeGet, current_dec_signed_dms) { FakeHandlers h; - h.currentDec = {47, 30, 15}; + h.currentDec = {47, 30, 15, false}; EXPECT_STREQ("+47*30'15#", dispatch("D", h)); EXPECT_STREQ("currentDec", h.lastCall); } @@ -196,7 +275,7 @@ TEST(MeadeGet, current_dec_signed_dms) TEST(MeadeGet, target_dec_negative) { FakeHandlers h; - h.targetDec = {-12, 45, 0}; + h.targetDec = {12, 45, 0, true}; EXPECT_STREQ("-12*45'00#", dispatch("d", h)); EXPECT_STREQ("targetDec", h.lastCall); } @@ -238,21 +317,101 @@ TEST(MeadeGet, is_guiding_emits_zero_one) TEST(MeadeGet, site_latitude_signed_two_digit_deg) { FakeHandlers h; - h.latitude = {47, 30}; + h.latitude = {47, 30, false}; EXPECT_STREQ("+47*30#", dispatch("t", h)); - h.latitude = {-12, 45}; + h.latitude = {12, 45, true}; EXPECT_STREQ("-12*45#", dispatch("t", h)); } TEST(MeadeGet, site_longitude_signed_three_digit_deg) { FakeHandlers h; - h.longitude = {12, 30}; + h.longitude = {12, 30, false}; EXPECT_STREQ("+012*30#", dispatch("g", h)); - h.longitude = {-122, 45}; + h.longitude = {122, 45, true}; EXPECT_STREQ("-122*45#", dispatch("g", h)); } +// ---- Sign of zero ----------------------------------------------------- +// +// A coordinate whose degrees component is zero still has a hemisphere. The +// magnitude and the sign are separate struct fields precisely so that these +// four replies do not all collapse onto the '+' form. + +TEST(MeadeGet, dec_zero_degrees_keeps_south_sign) +{ + FakeHandlers h; + h.currentDec = {0, 30, 0, true}; + EXPECT_STREQ("-00*30'00#", dispatch("D", h)); + h.currentDec = {0, 30, 0, false}; + EXPECT_STREQ("+00*30'00#", dispatch("D", h)); +} + +TEST(MeadeGet, site_latitude_zero_degrees_keeps_south_sign) +{ + FakeHandlers h; + h.latitude = {0, 30, true}; + EXPECT_STREQ("-00*30#", dispatch("t", h)); + h.latitude = {0, 30, false}; + EXPECT_STREQ("+00*30#", dispatch("t", h)); +} + +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}; + EXPECT_STREQ("+000*05#", dispatch("g", h)); +} + +// ---- Set -> Get round trips ------------------------------------------- +// +// The sign has to survive the wire -> struct -> wire journey, not just one +// leg of it. `-00*30:00` is the case that used to come back as `+00*30'00`. + +TEST(MeadeGet, dec_round_trip_preserves_sign_of_zero) +{ + FakeHandlers h; + EXPECT_STREQ("-00*30'00#", setThenGet("d-00*30:00", "D", h)); + EXPECT_STREQ("+00*30'00#", setThenGet("d+00*30:00", "D", h)); +} + +TEST(MeadeGet, dec_round_trip_preserves_nonzero_degrees) +{ + FakeHandlers h; + EXPECT_STREQ("-12*45'30#", setThenGet("d-12*45:30", "D", h)); + EXPECT_STREQ("+84*03'02#", setThenGet("d+84*03:02", "D", h)); +} + +TEST(MeadeGet, site_latitude_round_trip_preserves_sign_of_zero) +{ + FakeHandlers h; + EXPECT_STREQ("-00*30#", setThenGet("t-00*30", "t", h)); + EXPECT_STREQ("+00*30#", setThenGet("t+00*30", "t", h)); +} + +TEST(MeadeGet, site_latitude_round_trip_preserves_nonzero_degrees) +{ + FakeHandlers h; + EXPECT_STREQ("-45*15#", setThenGet("t-45:15", "t", h)); + EXPECT_STREQ("+47*30#", setThenGet("t+47*30", "t", h)); +} + +TEST(MeadeGet, site_longitude_round_trip_preserves_sign_of_zero) +{ + FakeHandlers h; + EXPECT_STREQ("-000*05#", setThenGet("g-000*05", "g", h)); + EXPECT_STREQ("+000*05#", setThenGet("g+000*05", "g", h)); +} + +TEST(MeadeGet, site_longitude_round_trip_preserves_nonzero_degrees) +{ + FakeHandlers h; + EXPECT_STREQ("-122*45#", setThenGet("g-122*45", "g", h)); + EXPECT_STREQ("+097*34#", setThenGet("g+097*34", "g", h)); +} + 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 new file mode 100644 index 00000000..b4259428 --- /dev/null +++ b/unit_tests/test_core/meade/test_MeadeParserHelpers.cpp @@ -0,0 +1,122 @@ +// Tests for the grammar primitives shared by the Meade family dispatchers +// (`Cursor`) and for the coordinate writers that turn parsed values back into +// wire bytes. +// +// These sit below the family dispatchers: the wire-byte behaviour of `:Sd`, +// `:St` and `:Sg` is covered in test_MeadeSet.cpp / test_MeadeGet.cpp. What is +// pinned here is the primitive that keeps a coordinate's sign in a channel of +// its own, so that a zero magnitude can still be negative. + +#include + +#include "core/meade/MeadeParserHelpers.hpp" + +namespace meade = oat::core::meade; + +namespace +{ + +const char *bytes(const meade::MeadeResponse &r) +{ + return r.c_str(); +} + +} // namespace + +// ---- Cursor::optionalSign --------------------------------------------- + +TEST(MeadeParserHelpers, optional_sign_consumes_minus) +{ + meade::Cursor c("-42"); + int sign = 0; + EXPECT_TRUE(c.optionalSign(sign)); + EXPECT_EQ(-1, sign); + EXPECT_STREQ("42", c.remaining()); +} + +TEST(MeadeParserHelpers, optional_sign_consumes_plus) +{ + meade::Cursor c("+42"); + int sign = 0; + EXPECT_TRUE(c.optionalSign(sign)); + EXPECT_EQ(1, sign); + EXPECT_STREQ("42", c.remaining()); +} + +TEST(MeadeParserHelpers, optional_sign_absent_leaves_cursor_put) +{ + meade::Cursor c("42"); + int sign = 0; + EXPECT_TRUE(c.optionalSign(sign)); + EXPECT_EQ(1, sign); + EXPECT_STREQ("42", c.remaining()); +} + +TEST(MeadeParserHelpers, optional_sign_at_end_of_input) +{ + meade::Cursor c(""); + int sign = 0; + EXPECT_TRUE(c.optionalSign(sign)); + EXPECT_EQ(1, sign); + EXPECT_TRUE(c.atEnd()); +} + +TEST(MeadeParserHelpers, optional_sign_keeps_sign_separate_from_magnitude) +{ + // The whole point of the primitive: "-00" carries a sign that no signed + // two-digit magnitude could hold. + meade::Cursor c("-00"); + int sign = 0; + unsigned mag = 99; + EXPECT_TRUE(c.optionalSign(sign)); + EXPECT_TRUE(c.digits(2, mag)); + EXPECT_EQ(-1, sign); + EXPECT_EQ(0u, mag); +} + +// ---- Cursor::advance --------------------------------------------------- + +TEST(MeadeParserHelpers, advance_moves_one_character) +{ + meade::Cursor c("abc"); + c.advance(); + EXPECT_EQ('b', c.peek()); +} + +TEST(MeadeParserHelpers, advance_at_end_is_a_no_op) +{ + meade::Cursor c(""); + c.advance(); + EXPECT_TRUE(c.atEnd()); +} + +// ---- Coordinate writers ------------------------------------------------ + +TEST(MeadeParserHelpers, write_dec_emits_sign_for_zero_degrees) +{ + meade::MeadeResponse r; + writeDec(r, meade::DecCoordinate {0, 30, 0, true}); + EXPECT_STREQ("-00*30'00#", bytes(r)); + + meade::MeadeResponse r2; + writeDec(r2, meade::DecCoordinate {0, 30, 0, false}); + EXPECT_STREQ("+00*30'00#", bytes(r2)); +} + +TEST(MeadeParserHelpers, write_latitude_emits_sign_for_zero_degrees) +{ + meade::MeadeResponse r; + writeLatitude(r, meade::MeadeLatitude {0, 30, true}); + EXPECT_STREQ("-00*30#", bytes(r)); +} + +TEST(MeadeParserHelpers, write_longitude_pads_to_three_digits_and_keeps_sign) +{ + meade::MeadeResponse r; + writeLongitude(r, meade::MeadeLongitude {0, 5, true}); + EXPECT_STREQ("-000*05#", bytes(r)); + + meade::MeadeResponse r2; + writeLongitude(r2, meade::MeadeLongitude {122, 45, false}); + EXPECT_STREQ("+122*45#", bytes(r2)); +} diff --git a/unit_tests/test_core/meade/test_MeadeSet.cpp b/unit_tests/test_core/meade/test_MeadeSet.cpp index 7c0074ff..46054d99 100644 --- a/unit_tests/test_core/meade/test_MeadeSet.cpp +++ b/unit_tests/test_core/meade/test_MeadeSet.cpp @@ -125,18 +125,20 @@ TEST(MeadeSet, target_dec_happy_path) FakeHandlers h; EXPECT_STREQ("1", dispatch("d+84*03:02", h)); EXPECT_STREQ("targetDec", h.lastCall); - EXPECT_EQ(84, h.dec.degrees); + EXPECT_EQ(static_cast(84), h.dec.degrees); EXPECT_EQ(static_cast(3), h.dec.minutes); EXPECT_EQ(static_cast(2), h.dec.seconds); + EXPECT_FALSE(h.dec.negative); } TEST(MeadeSet, target_dec_negative_with_colon_separator) { FakeHandlers h; EXPECT_STREQ("1", dispatch("d-12:45:30", h)); - EXPECT_EQ(-12, h.dec.degrees); + EXPECT_EQ(static_cast(12), h.dec.degrees); EXPECT_EQ(static_cast(45), h.dec.minutes); EXPECT_EQ(static_cast(30), h.dec.seconds); + EXPECT_TRUE(h.dec.negative); } TEST(MeadeSet, target_dec_handler_failure_returns_zero) @@ -242,7 +244,7 @@ TEST(MeadeSet, sync_coordinates_happy_path) FakeHandlers h; EXPECT_STREQ("1", dispatch("Y+84*03:02.18:34:12", h)); EXPECT_STREQ("sync", h.lastCall); - EXPECT_EQ(84, h.syncDec.degrees); + EXPECT_EQ(static_cast(84), h.syncDec.degrees); EXPECT_EQ(static_cast(3), h.syncDec.minutes); EXPECT_EQ(static_cast(2), h.syncDec.seconds); EXPECT_EQ(static_cast(18), h.syncRa.hours); @@ -264,16 +266,18 @@ TEST(MeadeSet, site_latitude_positive) FakeHandlers h; EXPECT_STREQ("1", dispatch("t+30*29", h)); EXPECT_STREQ("lat", h.lastCall); - EXPECT_EQ(30, h.lat.degrees); + EXPECT_EQ(static_cast(30), h.lat.degrees); EXPECT_EQ(static_cast(29), h.lat.minutes); + EXPECT_FALSE(h.lat.negative); } TEST(MeadeSet, site_latitude_negative_with_colon) { FakeHandlers h; EXPECT_STREQ("1", dispatch("t-45:15", h)); - EXPECT_EQ(-45, h.lat.degrees); + EXPECT_EQ(static_cast(45), h.lat.degrees); EXPECT_EQ(static_cast(15), h.lat.minutes); + EXPECT_TRUE(h.lat.negative); } TEST(MeadeSet, site_latitude_malformed_does_not_call_handler) @@ -290,8 +294,9 @@ TEST(MeadeSet, site_longitude_three_digit_degrees) FakeHandlers h; EXPECT_STREQ("1", dispatch("g+097*34", h)); EXPECT_STREQ("lon", h.lastCall); - EXPECT_EQ(97, h.lon.degrees); + EXPECT_EQ(static_cast(97), h.lon.degrees); EXPECT_EQ(static_cast(34), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); } TEST(MeadeSet, site_longitude_malformed_short_does_not_call_handler) @@ -370,6 +375,85 @@ TEST(MeadeSet, local_date_malformed_does_not_call_handler) EXPECT_EQ(nullptr, h.lastCall); } +// ---- Sign of zero ----------------------------------------------------- +// +// Every wire format in this family puts the sign in front of a degrees field +// that can legitimately be zero. Half a degree south of the equator is +// "-00*30:00", and reading it as "+00*30:00" is a one-degree error. + +TEST(MeadeSet, target_dec_negative_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("d-00*30:00", h)); + EXPECT_EQ(static_cast(0), h.dec.degrees); + EXPECT_EQ(static_cast(30), h.dec.minutes); + EXPECT_EQ(static_cast(0), h.dec.seconds); + EXPECT_TRUE(h.dec.negative); +} + +TEST(MeadeSet, target_dec_positive_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("d+00*30:00", h)); + EXPECT_EQ(static_cast(0), h.dec.degrees); + EXPECT_EQ(static_cast(30), h.dec.minutes); + EXPECT_FALSE(h.dec.negative); +} + +TEST(MeadeSet, sync_coordinates_negative_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("Y-00*30:00.18:34:12", h)); + EXPECT_STREQ("sync", h.lastCall); + EXPECT_EQ(static_cast(0), h.syncDec.degrees); + EXPECT_EQ(static_cast(30), h.syncDec.minutes); + EXPECT_TRUE(h.syncDec.negative); +} + +TEST(MeadeSet, site_latitude_negative_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("t-00*30", h)); + EXPECT_EQ(static_cast(0), h.lat.degrees); + EXPECT_EQ(static_cast(30), h.lat.minutes); + EXPECT_TRUE(h.lat.negative); +} + +TEST(MeadeSet, site_latitude_positive_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("t+00*30", h)); + EXPECT_EQ(static_cast(0), h.lat.degrees); + EXPECT_FALSE(h.lat.negative); +} + +TEST(MeadeSet, site_longitude_negative_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g-000*05", h)); + EXPECT_EQ(static_cast(0), h.lon.degrees); + EXPECT_EQ(static_cast(5), h.lon.minutes); + EXPECT_TRUE(h.lon.negative); +} + +TEST(MeadeSet, site_longitude_positive_zero_degrees_keeps_sign) +{ + FakeHandlers h; + EXPECT_STREQ("1", dispatch("g+000*05", h)); + EXPECT_EQ(static_cast(0), h.lon.degrees); + EXPECT_EQ(static_cast(5), h.lon.minutes); + EXPECT_FALSE(h.lon.negative); +} + +TEST(MeadeSet, utc_offset_negative_zero_is_zero) +{ + FakeHandlers h; + h.utc = 99; // Poison, so the assertion below cannot pass on the default. + EXPECT_STREQ("1", dispatch("G-00", h)); + EXPECT_STREQ("utc", h.lastCall); + EXPECT_EQ(0, h.utc); +} + // ---- Top-level routing ------------------------------------------------ TEST(MeadeSet, unknown_subcommand_returns_zero) diff --git a/unit_tests/test_core/types/test_latitude.cpp b/unit_tests/test_core/types/test_latitude.cpp index 4bf3940a..ca41cf60 100644 --- a/unit_tests/test_core/types/test_latitude.cpp +++ b/unit_tests/test_core/types/test_latitude.cpp @@ -55,3 +55,15 @@ TEST(LatitudeTest, CopyConstructor) Latitude lat2(lat1); EXPECT_FLOAT_EQ(45.0f, lat2.getTotalHours()); } + +TEST(LatitudeTest, AddSecondsKeepsSignBelowOneDegree) +{ + // The (h, m, s) constructor derives the sign from `h`, so it cannot build + // a site half a degree south of the equator. Accumulating signed seconds + // can, and the clamp in checkHours() leaves the value alone. + Latitude lat; + lat.addSeconds(-1800); + EXPECT_EQ(-1800, lat.getTotalSeconds()); + EXPECT_EQ(0, lat.getHours()); + EXPECT_EQ(30, lat.getMinutes()); +} diff --git a/unit_tests/test_core/types/test_longitude.cpp b/unit_tests/test_core/types/test_longitude.cpp index 0312dee8..ac89b00d 100644 --- a/unit_tests/test_core/types/test_longitude.cpp +++ b/unit_tests/test_core/types/test_longitude.cpp @@ -56,3 +56,14 @@ TEST(LongitudeTest, CopyConstructor) Longitude lon2(lon1); EXPECT_FLOAT_EQ(50.0f, lon2.getTotalHours()); } + +TEST(LongitudeTest, AddSecondsKeepsSignBelowOneDegree) +{ + // As for Latitude: a longitude five arc-minutes west of Greenwich has a + // sign but no degrees, so it has to be built from signed seconds. + Longitude lon; + lon.addSeconds(-300); + EXPECT_EQ(-300, lon.getTotalSeconds()); + EXPECT_EQ(0, lon.getHours()); + EXPECT_EQ(5, lon.getMinutes()); +} From 52f7d183d37127bd832a14f47af817cc83957936 Mon Sep 17 00:00:00 2001 From: Kiryl Date: Fri, 18 Sep 2026 19:20:13 -0700 Subject: [PATCH 2/2] fix(meade): carry the declination sign across the wire boundary The parser keeps sign and magnitude apart, but decFromWire flattened the flag back into a signed `deg` for fromCelestialDegrees, and integer 0 has no sign. ":Sd-00*30:00" and ":Sd+00*30:00" both landed on axis seconds 322200; -00*30:00 is 325800. Exactly one degree, silently, for any target or sync inside the first degree south of the celestial equator. Add core::Declination::celestialSecondsFrom, the declination counterpart of the site join, and Declination::fromCelestialSeconds to consume it, so decFromWire composes the two and holds no arithmetic of its own. The join lives in core because the native test environment builds only src/core, src/ports and src/adapters -- src/MeadeCommandProcessor.cpp and src/Declination.cpp are Arduino-dependent and never compiled there, which is why the parser-level tests could pass while the wire boundary was wrong. The new tests pin both the defective composition and the correct one side by side, in both hemispheres. fromCelestialDegrees keeps its comment block describing the limitation and now has no production caller. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz --- src/Declination.cpp | 17 ++++-- src/Declination.hpp | 6 ++ src/MeadeCommandProcessor.cpp | 11 ++-- src/core/types/Declination.cpp | 6 ++ src/core/types/Declination.hpp | 8 +++ .../test_core/types/test_declination.cpp | 61 +++++++++++++++++++ 6 files changed, 100 insertions(+), 9 deletions(-) diff --git a/src/Declination.cpp b/src/Declination.cpp index e43314ce..b4262399 100644 --- a/src/Declination.cpp +++ b/src/Declination.cpp @@ -95,13 +95,22 @@ Declination Declination::fromCelestialDegrees(int deg, int min, int sec) // deg carries the only sign on the wire, so min and sec are unsigned // magnitudes and must move away from zero. joinSeconds is the inverse of the // splitSeconds that getCelestialDegrees uses. - // Declinations between -1 and 0 degrees still cannot round-trip here: they - // arrive as deg == 0, and integer 0 has no sign, so -00*30:00 reads the same - // as +00*30:00. Fixing that means carrying the sign separately from the - // magnitude across this boundary, not changing the join. + // Declinations between -1 and 0 degrees cannot round-trip here: they arrive + // as deg == 0, and integer 0 has no sign, so -00*30:00 reads the same as + // +00*30:00. That is why the wire boundary uses fromCelestialSeconds + // instead -- the sign travels in the total, not in a degrees component. const long wireSecs = core::DayTime::joinSeconds(deg, min, sec); Declination result; result.totalSeconds = core::Declination::celestialToAxisSeconds(wireSecs, inNorthernHemisphere); result.checkHours(); return result; } + +Declination Declination::fromCelestialSeconds(long celestialSeconds) +{ + // The sign lives in the total, so there is no zero-degrees blind spot here. + Declination result; + result.totalSeconds = core::Declination::celestialToAxisSeconds(celestialSeconds, inNorthernHemisphere); + result.checkHours(); + return result; +} diff --git a/src/Declination.hpp b/src/Declination.hpp index 220fb612..41873c1b 100644 --- a/src/Declination.hpp +++ b/src/Declination.hpp @@ -25,6 +25,12 @@ class Declination : public core::Declination // minutes/seconds. static Declination fromCelestialDegrees(int deg, int min, int sec); + // Build from signed celestial arc-seconds. Preferred over + // fromCelestialDegrees at the wire boundary: a signed degrees component + // cannot express a coordinate between 0 and -1 degree. Pair it with + // core::Declination::celestialSecondsFrom to do the join. + static Declination fromCelestialSeconds(long celestialSeconds); + const char *ToDisplayString(char sep1, char sep2) const; static Declination ParseFromMeade(String const &s); diff --git a/src/MeadeCommandProcessor.cpp b/src/MeadeCommandProcessor.cpp index 75eb3a42..810edc5d 100644 --- a/src/MeadeCommandProcessor.cpp +++ b/src/MeadeCommandProcessor.cpp @@ -117,11 +117,12 @@ meade::DecCoordinate decFrom(const Declination &d) Declination decFromWire(meade::DecCoordinate const &d) { - // fromCelestialDegrees carries the sign in its `deg` parameter, so a - // coordinate such as "-00*30:00" still arrives there unsigned. The parser - // keeps sign and magnitude apart up to this call. - const int degrees = d.negative ? -static_cast(d.degrees) : static_cast(d.degrees); - return Declination::fromCelestialDegrees(degrees, d.minutes, d.seconds); + // The parser keeps sign and magnitude apart, and they stay apart all the way + // into the join. Flattening `negative` back into a signed degrees component + // here would lose it again for "-00*30:00", which is the whole point of the + // separate flag. celestialSecondsFrom is the declination counterpart of + // siteSecondsFrom below. + return Declination::fromCelestialSeconds(core::Declination::celestialSecondsFrom(d.degrees, d.minutes, d.seconds, d.negative)); } // Signed arc-seconds for a magnitude/sign pair. The Latitude and Longitude diff --git a/src/core/types/Declination.cpp b/src/core/types/Declination.cpp index de29194a..2404eeb6 100644 --- a/src/core/types/Declination.cpp +++ b/src/core/types/Declination.cpp @@ -74,4 +74,10 @@ long Declination::celestialToAxisSeconds(long celestialSeconds, bool northernHem return northernHemisphere ? hemiArcsecs - celestialSeconds : -hemiArcsecs - celestialSeconds; } +long Declination::celestialSecondsFrom(uint16_t degrees, uint8_t minutes, uint8_t seconds, bool negative) +{ + const long magnitude = (((static_cast(degrees) * 60L) + minutes) * 60L) + seconds; + return negative ? -magnitude : magnitude; +} + } // namespace core diff --git a/src/core/types/Declination.hpp b/src/core/types/Declination.hpp index dda22d1a..de65d3e9 100644 --- a/src/core/types/Declination.hpp +++ b/src/core/types/Declination.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include "DayTime.hpp" namespace core @@ -31,6 +33,12 @@ class Declination : public DayTime static long axisToCelestialSeconds(long axisSeconds, bool northernHemisphere); static long celestialToAxisSeconds(long celestialSeconds, bool northernHemisphere); + // Join a Meade-wire magnitude/sign pair into signed celestial arc-seconds. + // The declination counterpart of the site join in MeadeCommandProcessor: + // a signed degrees component cannot express a coordinate between 0 and -1 + // degree, so the sign has to travel alongside the magnitude down to here. + static long celestialSecondsFrom(uint16_t degrees, uint8_t minutes, uint8_t seconds, bool negative); + // Construct from total (axis) seconds directly, avoiding float rounding. static Declination fromTotalSeconds(long totalSeconds); diff --git a/unit_tests/test_core/types/test_declination.cpp b/unit_tests/test_core/types/test_declination.cpp index 0e72121e..69c27d9c 100644 --- a/unit_tests/test_core/types/test_declination.cpp +++ b/unit_tests/test_core/types/test_declination.cpp @@ -185,3 +185,64 @@ TEST(DeclinationTest, CelestialWireRoundTrip) } } } + +TEST(DeclinationTest, CelestialSecondsFromKeepsTheSignOfZeroDegrees) +{ + // The magnitude is unsigned and the sign is separate, so a coordinate + // inside the first degree south of the equator survives the join. + EXPECT_EQ(-1800L, Declination::celestialSecondsFrom(0, 30, 0, true)); + EXPECT_EQ(1800L, Declination::celestialSecondsFrom(0, 30, 0, false)); + EXPECT_EQ(-59L, Declination::celestialSecondsFrom(0, 0, 59, true)); + + // Exact zero has no sign to keep, either way round. + EXPECT_EQ(0L, Declination::celestialSecondsFrom(0, 0, 0, true)); + EXPECT_EQ(0L, Declination::celestialSecondsFrom(0, 0, 0, false)); + + // Whole degrees still join the way the signed-degrees form did. + EXPECT_EQ(core::DayTime::joinSeconds(-5, 30, 0), Declination::celestialSecondsFrom(5, 30, 0, true)); + EXPECT_EQ(core::DayTime::joinSeconds(89, 59, 59), Declination::celestialSecondsFrom(89, 59, 59, false)); +} + +TEST(DeclinationTest, ZeroDegreesSouthLandsOneDegreeFromWhereSignedDegreesPutIt) +{ + // Pins the defect this pairing exists to close. MeadeCommandProcessor's + // decFromWire used to flatten the parser's sign flag back into a signed + // `deg`, so "-00*30:00" reached the join as joinSeconds(0, 30, 0) -- the + // same value as "+00*30:00", one whole degree away from the truth. + const long viaSignedDegrees = Declination::celestialToAxisSeconds(core::DayTime::joinSeconds(0, 30, 0), true); + const long viaSeparateSign = Declination::celestialToAxisSeconds(Declination::celestialSecondsFrom(0, 30, 0, true), true); + + EXPECT_EQ(322200L, viaSignedDegrees); + EXPECT_EQ(325800L, viaSeparateSign); + EXPECT_EQ(3600L, viaSeparateSign - viaSignedDegrees); + + // Southern mounts have the same blind spot on the same input, mirrored: + // the signed-degrees path is the one that lands 1 degree out. + EXPECT_EQ(-325800L, Declination::celestialToAxisSeconds(core::DayTime::joinSeconds(0, 30, 0), false)); + EXPECT_EQ(-322200L, Declination::celestialToAxisSeconds(Declination::celestialSecondsFrom(0, 30, 0, true), false)); +} + +TEST(DeclinationTest, CelestialSecondsFromRoundTripsThroughTheAxis) +{ + struct WireDec { + uint16_t deg; + uint8_t min; + uint8_t sec; + bool negative; + }; + const WireDec cases[] = { + {0, 30, 0, true}, {0, 30, 0, false}, {0, 0, 1, true}, {5, 30, 0, true}, {24, 23, 0, true}, {89, 59, 59, true}, {89, 59, 59, false}}; + const bool hemispheres[] = {true, false}; + + for (bool north : hemispheres) + { + for (const WireDec &wire : cases) + { + const long celestial = Declination::celestialSecondsFrom(wire.deg, wire.min, wire.sec, wire.negative); + const Declination onAxis(Declination::fromTotalSeconds(Declination::celestialToAxisSeconds(celestial, north))); + + EXPECT_EQ(celestial, Declination::axisToCelestialSeconds(onAxis.getTotalSeconds(), north)) + << "north=" << north << " deg=" << wire.deg << " negative=" << wire.negative; + } + } +}