From 7e73aa3b23aacca07812e32b8d027ca3a90c28db Mon Sep 17 00:00:00 2001 From: Kiryl Date: Sun, 6 Sep 2026 20:06:49 -0700 Subject: [PATCH] fix(meade): build declination from wire components without losing the sign Follow-up to #300. The hemisphere conversion it restored is correct; the component assembly feeding it is not. Declination::fromCelestialDegrees() computed const long wireSecs = ((60L * deg) + min) * 60L + sec; but meade::DecCoordinate stores minutes and seconds as uint8_t magnitudes and carries the sign only in degrees, so for a negative declination they were added toward zero instead of away from it. The error is 2 * (min * 60 + sec) arcseconds -- up to 1.9994 degrees -- on every celestial declination in (-90, 0) with non-zero arcminutes, in both hemispheres. It is directly observable, because the getter is right and the setter is wrong: :Sd-05*30:00# followed by :Gd# reads back -04*30:00. getCelestialDegrees() delegates the split to core::DayTime::splitSeconds(), while the join was hand-rolled. :CM sync takes the same path, so a plate-solve sync at a negative declination writes the error into the mount's home reference rather than into a single slew. The tests did not catch it because platformio.ini's native build_src_filter covers only core, ports and adapters, so src/Declination.cpp is never compiled into the test binary. #300's tests exercise axisToCelestialSeconds, celestialToAxisSeconds and splitSeconds, all of which are correct and are unchanged here. Add core::DayTime::joinSeconds() beside splitSeconds as its inverse, refactor DayTime(int, int, int) onto it -- that constructor already applied the sign correctly, so this lifts existing behaviour rather than introducing new arithmetic -- and reduce fromCelestialDegrees() to composition, leaving no bare arithmetic outside core. Co-authored-by: Claude --- src/Declination.cpp | 9 ++- src/core/types/DayTime.cpp | 9 ++- src/core/types/DayTime.hpp | 7 +++ unit_tests/test_core/types/test_daytime.cpp | 60 +++++++++++++++++++ .../test_core/types/test_declination.cpp | 32 ++++++++++ 5 files changed, 113 insertions(+), 4 deletions(-) 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; + } + } +}