diff --git a/src/Declination.cpp b/src/Declination.cpp index 202f99b9..e43314ce 100644 --- a/src/Declination.cpp +++ b/src/Declination.cpp @@ -92,7 +92,14 @@ void Declination::getCelestialDegrees(int °, int &min, int &sec) const Declination Declination::fromCelestialDegrees(int deg, int min, int sec) { - const long wireSecs = ((60L * deg) + min) * 60L + 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. + const long wireSecs = core::DayTime::joinSeconds(deg, min, sec); Declination result; result.totalSeconds = core::Declination::celestialToAxisSeconds(wireSecs, inNorthernHemisphere); result.checkHours(); diff --git a/src/core/types/DayTime.cpp b/src/core/types/DayTime.cpp index 35d9fad4..a44e4b4b 100644 --- a/src/core/types/DayTime.cpp +++ b/src/core/types/DayTime.cpp @@ -15,9 +15,7 @@ DayTime::DayTime(const DayTime &other) DayTime::DayTime(int h, int m, int s) { - long sgn = sign(h); - h = abs(h); - totalSeconds = sgn * ((60L * h + m) * 60L + s); + totalSeconds = joinSeconds(h, m, s); } DayTime::DayTime(float timeInHours) @@ -85,6 +83,11 @@ void DayTime::splitSeconds(long secs, int &h, int &m, int &s) h *= sign(secs); } +long DayTime::joinSeconds(int h, int m, int s) +{ + return sign(h) * ((60L * labs(h) + m) * 60L + s); +} + void DayTime::set(int h, int m, int s) { DayTime dt(h, m, s); diff --git a/src/core/types/DayTime.hpp b/src/core/types/DayTime.hpp index 4bdd5a9e..98c0638d 100644 --- a/src/core/types/DayTime.hpp +++ b/src/core/types/DayTime.hpp @@ -35,6 +35,13 @@ class DayTime // Split signed seconds into (signed) hours plus unsigned minutes/seconds. static void splitSeconds(long secs, int &h, int &m, int &s); + // Join hours/minutes/seconds into signed seconds. The sign is taken from h + // alone; m and s are added to the magnitude as given, so a negative m or s + // subtracts from it, exactly as the DayTime(int, int, int) constructor has + // always done. Inverts splitSeconds for every value except -3599..-1 + // seconds, where splitSeconds reports h == 0 and the sign is already lost. + static long joinSeconds(int h, int m, int s); + virtual void set(int h, int m, int s); void set(const DayTime &other); diff --git a/unit_tests/test_core/types/test_daytime.cpp b/unit_tests/test_core/types/test_daytime.cpp index 8cf4c9ae..736efcd5 100644 --- a/unit_tests/test_core/types/test_daytime.cpp +++ b/unit_tests/test_core/types/test_daytime.cpp @@ -241,3 +241,63 @@ TEST(DayTimeTest, FormatStringCustomSecsNegative) dt.formatString(buf, "{d}:{m}:{s}", &customSecs); EXPECT_STREQ("-100:30:15", buf); } + +// --------------------------------------------------------------------------- +// joinSeconds — the inverse of splitSeconds, and the shared implementation of +// the DayTime(int, int, int) constructor. +// --------------------------------------------------------------------------- + +TEST(DayTimeTest, JoinSeconds) +{ + EXPECT_EQ(0L, DayTime::joinSeconds(0, 0, 0)); + EXPECT_EQ(3600L, DayTime::joinSeconds(1, 0, 0)); + EXPECT_EQ(5445L, DayTime::joinSeconds(1, 30, 45)); + EXPECT_EQ(-5445L, DayTime::joinSeconds(-1, 30, 45)); +} + +TEST(DayTimeTest, JoinSecondsIsUsedByHMSConstructor) +{ + EXPECT_EQ(DayTime::joinSeconds(1, 30, 45), DayTime(1, 30, 45).getTotalSeconds()); + EXPECT_EQ(DayTime::joinSeconds(-2, 31, 18), DayTime(-2, 31, 18).getTotalSeconds()); + // Callers that pass negative minutes/seconds keep subtracting from the + // magnitude, as NegativeConstructor and GetTimeRefNegative above rely on. + EXPECT_EQ(DayTime::joinSeconds(-2, -30, -15), DayTime(-2, -30, -15).getTotalSeconds()); +} + +TEST(DayTimeTest, JoinSecondsInvertsSplitSeconds) +{ + // Skips (-3600, 0): splitSeconds reports h == 0 there, which drops the sign + // (see JoinSecondsCannotSignSubHourValues). + for (long secs = -180L * 3600L; secs <= 180L * 3600L; secs += 907L) + { + if ((secs < 0L) && (secs > -3600L)) + { + continue; + } + int h, m, s; + DayTime::splitSeconds(secs, h, m, s); + EXPECT_EQ(secs, DayTime::joinSeconds(h, m, s)) << "secs=" << secs; + } +} + +TEST(DayTimeTest, JoinSecondsCannotSignSubHourValues) +{ + // Integer -0 is 0, so a value between -1 and 0 hours (or degrees) has no + // way to express its sign through h. Both spellings join to the same + // positive value; representing -00:30:00 needs a separate sign, not a + // different join. + EXPECT_EQ(1800L, DayTime::joinSeconds(0, 30, 0)); + EXPECT_EQ(1800L, DayTime::joinSeconds(-0, 30, 0)); +} + +TEST(DayTimeTest, JoinSecondsNegativeDegreesMoveAwayFromZero) +{ + // Declinations off the Meade wire: the sign sits on the degrees and the + // minutes/seconds are unsigned magnitudes, so they must extend the value + // away from zero. Adding them toward zero instead put :Sd-05*30:00# back + // on the wire as -04*30:00 (see Declination::fromCelestialDegrees). + EXPECT_EQ(-(5L * 3600L + 30L * 60L), DayTime::joinSeconds(-5, 30, 0)); + EXPECT_EQ(-(24L * 3600L + 23L * 60L), DayTime::joinSeconds(-24, 23, 0)); + EXPECT_EQ(-(69L * 3600L + 6L * 60L), DayTime::joinSeconds(-69, 6, 0)); + EXPECT_EQ(-(89L * 3600L + 59L * 60L + 59L), DayTime::joinSeconds(-89, 59, 59)); +} diff --git a/unit_tests/test_core/types/test_declination.cpp b/unit_tests/test_core/types/test_declination.cpp index 9788c28d..0e72121e 100644 --- a/unit_tests/test_core/types/test_declination.cpp +++ b/unit_tests/test_core/types/test_declination.cpp @@ -153,3 +153,35 @@ TEST(DeclinationTest, FromTotalSecondsClamps) EXPECT_EQ(-180L * 3600L, Declination::fromTotalSeconds(-200L * 3600L).getTotalSeconds()); EXPECT_EQ(12345L, Declination::fromTotalSeconds(12345L).getTotalSeconds()); } + +TEST(DeclinationTest, CelestialWireRoundTrip) +{ + // Reproduces the composition that Declination::fromCelestialDegrees and + // Declination::getCelestialDegrees perform for the Meade :Sd/:Gd/:CM + // commands. src/Declination.cpp itself needs Arduino String and the board + // configuration, so it cannot be linked here: this pins that the sequence + // is correct, not that src/Declination.cpp still uses it. + struct WireDec { + int deg; + int min; + int sec; + }; + const WireDec cases[] = {{-5, 30, 0}, {-24, 23, 0}, {-69, 6, 0}, {-89, 59, 59}, {5, 30, 0}, {0, 30, 0}, {45, 0, 0}, {89, 59, 59}}; + const bool hemispheres[] = {true, false}; + + for (bool north : hemispheres) + { + for (const WireDec &wire : cases) + { + const long celestial = core::DayTime::joinSeconds(wire.deg, wire.min, wire.sec); + const Declination onAxis(Declination::fromTotalSeconds(Declination::celestialToAxisSeconds(celestial, north))); + + int deg, min, sec; + core::DayTime::splitSeconds(Declination::axisToCelestialSeconds(onAxis.getTotalSeconds(), north), deg, min, sec); + + EXPECT_EQ(wire.deg, deg) << "north=" << north << " deg=" << wire.deg; + EXPECT_EQ(wire.min, min) << "north=" << north << " deg=" << wire.deg; + EXPECT_EQ(wire.sec, sec) << "north=" << north << " deg=" << wire.deg; + } + } +}