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()); +}