Skip to content

fix(meade): build declination from wire components without losing the sign - #302

Merged
ClutchplateDude merged 1 commit into
OpenAstroTech:developfrom
rbhbokka:fix/meade-dec-wire-sign
Sep 9, 2026
Merged

ClutchplateDude merged 1 commit into
OpenAstroTech:developfrom
rbhbokka:fix/meade-dec-wire-sign

Conversation

@rbhbokka

@rbhbokka rbhbokka commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #300, which restored the DEC hemisphere conversion. The conversion itself is correct; the component assembly that feeds it is not.

The defect

Declination::fromCelestialDegrees() (src/Declination.cpp) builds the wire-seconds total as

const long wireSecs = ((60L * deg) + min) * 60L + sec;

meade::DecCoordinate is int16_t degrees; uint8_t minutes; uint8_t seconds; — minutes and seconds are unsigned magnitudes, and only degrees carries the sign. For a negative declination they are therefore added toward zero rather than away from it.

Error = 2 × (minutes × 60 + seconds) arcseconds, i.e. 0 to 1.9994°, on every celestial declination in (−90°, 0°) with non-zero arcminutes, in both hemispheres.

The setter and the getter disagree as a result, since getCelestialDegrees() splits correctly via core::DayTime::splitSeconds() while the join was hand-rolled — :Sd-05*30:00# followed by :Gd# returns -04*30:00.

wire axis seconds correct error
-05*30:00 340200 343800 −1.000°
-24*23:00 (M8) 409020 411780 −0.767°
-69*06:00 (LMC) 572040 572760 −0.200°
-89*59:59 640801 647999 −1.999°

:CM sync goes through the same decFromWire() 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.

These figures are derived by exercising the shipped fromCelestialDegrees() text against the real core:: conversions, not from a hardware session — I have not reproduced it on a mount.

Why the tests did not catch it

platformio.ini sets build_src_filter = +<./core> +<./ports> +<./adapters>, so src/Declination.cpp is never compiled into the native test binary. #300's tests exercise core::Declination::axisToCelestialSeconds, celestialToAxisSeconds and core::DayTime::splitSeconds — all three are correct and are unchanged here. The one piece of new arithmetic that landed outside src/core is the one that was wrong.

The fix

The asymmetry is the bug: getCelestialDegrees() delegates the split to the tested primitive core::DayTime::splitSeconds(), while fromCelestialDegrees() hand-rolled the join, because that inverse was never written.

This adds core::DayTime::joinSeconds() next to splitSeconds() as its exact inverse, refactors the existing DayTime(int, int, int) constructor onto it — that constructor already applied the sign correctly, so this lifts existing behaviour rather than introducing new arithmetic — and reduces fromCelestialDegrees() to pure composition, leaving no bare arithmetic outside core.

Refactoring the constructor onto the new primitive is deliberate: two pre-existing constructor tests now also guard the join, so the two can no longer drift apart.

Verification

  • pio test -e native: 275 pass (269 on develop + 6 added).
  • An exhaustive sweep of the domain readDecCoordinate accepts, both hemispheres, is now exact everywhere; before this change 1,771,746 of 3,543,852 inputs were wrong, worst case 14,400 arcsec.
  • Builds clean under -Werror for oaeboardv1 (ESP32) and ramps (AVR, 16-bit int).

Not fixed here

DecCoordinate carries the sign only in degrees, so -00*30:00 is unrepresentable and still parses as +00*30:00 regardless of this change. That is a separate, wider issue across all three coordinate structs; it is noted in a comment here and addressed on fix/meade-sign-of-zero.

File overlap

None with the other Meade fixes I have open — this touches src/Declination.cpp, src/core/types/DayTime.* and the test_core/types tests only.

… sign

Follow-up to OpenAstroTech#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. OpenAstroTech#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 <noreply@anthropic.com>
@ClutchplateDude
ClutchplateDude enabled auto-merge (squash) September 9, 2026 07:13
@ClutchplateDude
ClutchplateDude merged commit f0e89ff into OpenAstroTech:develop Sep 9, 2026
8 checks passed
@LeoXX

LeoXX commented Sep 12, 2026

Copy link
Copy Markdown

After this commit, oat hangs immediately after displaying the version; the connection to the PC is also lost.

@rbhbokka

rbhbokka commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor Author

After this commit, oat hangs immediately after displaying the version; the connection to the PC is also lost.

Sorry you hit that. I don't think this commit can be the cause, though, and I'd rather narrow it down with you than guess.

The function it changed, Declination::fromCelestialDegrees, is only called from decFromWire, which is a Meade command handler. Nothing in setup reaches it, so it can't run before a command arrives. The other change, the DayTime(int,int,int) constructor, is on the boot path, but every caller there passes non-negative values, and for those the old and new code give identical results. I swept the arithmetic to be sure: the only inputs where they differ at all are at INT_MIN, and neither version loops.

My guess is "after this commit" means "after I pulled", and that same pull also brought #300, #297 and #291.

Things that would narrow it down a lot:

  1. Which board, RAMPS or ESP32? That changes what "lost the PC connection" means. On RAMPS the USB serial is a separate chip that survives the MCU hanging, so a port disappearing points at a reset or brownout instead.
  2. Does the commit just before this one (1c85a37) also hang? That's one flash and it settles it either way.
  3. With DEBUG_LEVEL turned up, what's the last log line you see? "After the version" still covers a lot of setup.
  4. Your NORTHERN_HEMISPHERE, TRACK_ON_BOOT and UART_CONNECTION_TEST_TXRX settings.
  5. Does erasing EEPROM (hold DOWN at boot) change anything?

Unrelated, but I spotted it while looking: Mount::waitUntilStopped is an unbounded loop calling Mount::loop(), which doesn't service serial. That would produce both halves of your symptom at once. I couldn't find a path that actually enters it during setup, so I'm not claiming it's your bug, but a timeout and a log line there would turn a silent hang into something you can diagnose. Happy to open that separately if it's wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants