Conversation
ClutchplateDude
left a comment
ClutchplateDude
left a comment
There was a problem hiding this comment.
The Meade protocol docs are contradictory with longitude, especially getter vs. setter.
The setter is defined as :SgDDD*MM and so does NOT accept a sign (it also give no indication what numbers are returned).
The getter is defined as :Gg# and is documented as returning sDDD*MM#, where East is negative.
I think we should have :Sg accept a (optional) sign.
Regression from OpenAstroTech#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-OpenAstroTech#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 OpenAstroTech#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 OpenAstroTech#291 routed around them, so the idea is applied here where the code now lives. Co-authored-by: Claude <noreply@anthropic.com>
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz
MeadeProtocol.hpp documents :Sg and :Gg as east-negative -- zero at Greenwich, negative coordinates going east. OpenAstroTech#291 dropped the negation on both sides at once, so the wire convention silently inverted while every readback still round-tripped perfectly. Restore it on both sides in one commit. readLongitude negates into the east-positive struct; writeLongitude negates back out. Moving only one side would be worse than either convention: a client would set its site, read back the mirror, and push the mirror in on the next connect, where it persists to EEPROM. Under east-negative the signed and the unsigned forms are the same mapping -- east = wrap(-value) either way -- so the two branches collapse into one reader with an optional sign, and the legacy 0..360 westward count INDI sends is just the sign == '+' case. That also retires the sub-degree-west limitation: the sign now travels in MeadeLongitude's `negative` field rather than in `degrees`, so "000*30" (30' west) and "359*30" (30' east) are no longer the same struct. Greenwich goes out as "+000*00#": it is on neither side, and "-000*00" reads as a negative zero. The struct comment now records which convention the value is in. Nothing in the type could show it before, which is how a flip on both sides at once went unnoticed. NOTE: this changes released behaviour. Firmware through v1.13.20 replies to :Gg east-positive, so a client that adapted to that will mirror its site once. A mount whose site was set under that firmware also holds the mirrored value in EEPROM, which this does not correct -- the site has to be pushed again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz
3648272 to
6bd9591
Compare
Agreed, and it makes the change smaller rather than bigger. Under east-negative the signed and unsigned forms turn out to be the same mapping, so the two branches collapse into one reader with an optional sign, and the old 0..360 westward count is just the case where the sign is That also gets rid of the sub-degree limitation I flagged in the description, since the sign now lives in I had to flip the getter in the same commit.
Two things that probably want a release note, since east-positive is in released firmware (v1.13.20) and not just develop:
One other thing: #296 shouldn't merge alongside this. It flips the sign in Rebased onto #305 and marked ready for review. |
2 participants
Regression from #291. Draft, because it also raises a protocol-convention question I'd like a maintainer to rule on — see the last section, and note that #296 asserts the opposite convention for the signed form.
The defect
readLongitude()usesCursor::signed3(), which makes the sign mandatory. INDI sends:Sg121*53#unsigned for a site 121°53′ west, so the command is answered0and the mount silently keeps whatever longitude it had.Captured off the wire on an INDI connect to an OpenAstroExplorer:
An unsigned Meade site longitude is the legacy count running westward from Greenwich, so it folds into the east-positive range the mount stores by negating modulo a full circle.
Not by
180 - value. That is the other reflection — the one that puts Greenwich at 180 — and it is what the (now dead)Longitude::ParseFromMeadecomputes and whatMeadeProtocol.hppdescribed. It turns a 121°53′ west site into 58°07′ east: exactly 180°, i.e. 12 hours of local sidereal time, from where it belongs, which mispoints every subsequent GOTO.None of this is visible in readback —
:Gg#applies the inverse of whatever the setter did, so a wrong convention round-trips perfectly. It was verified instead by comparing:XGL#against an independently computed LST.Also fixed
Range validation.
digits(3, …)accepted 0..999 and the wrap was a singleif, not a loop, so:Sg600*00#yielded −240°. Nothing downstream caught it:core::Longitude(int, int, int)never callscheckHours(), andEEPROMStore::storeLongitudeclampsdegrees * 100into anint16(±327.67°) — that clamp destroys the mod-360 equivalence and persists a genuinely wrong site across reboots. Values outside one full circle, and minutes above 59, are now refused.Signed overflow on AVR. The arcminute arithmetic is widened to
long.intis 16-bit on ATmega2560, sodeg * 60 + mmexceededINT16_MAXfrom:Sg545*69#onward — undefined behaviour in an-O2build, and it did diverge between 16- and 32-bit targets. The range check also prevents reaching it; both are in.Deliberately unchanged
The signed form. Which hemisphere its sign denotes is a separate question —
MeadeProtocol.hppsays negative goes east, the deadLongitude::ParseFromMeadeimplemented that, #291 dropped it, and #296 proposes restoring it. This change passes signed values through exactly asdevelopdoes and adds a test pinning that, so the discussion starts from a documented baseline. I have no evidence either way for the signed form and am not trying to settle it here.Known limitation
A west longitude under one degree (
000*01..000*59) still loses its sign, becauseMeadeLongitudecarries the sign only indegreesand-0is unrepresentable. There is aKNOWN LIMITATIONcomment at the exact spot and a test named for it. The structural fix isfix/meade-sign-of-zero, which gives all three coordinate structs a real sign channel; I kept it out of this PR to keep the convention argument separable from the struct change.The question for maintainers
MeadeProtocol.hppcurrently documents the unsigned form as "0 to 360 going WEST with 180 at Greenwich. So 369 is 179W and 1 is 179E." No live code implements that, and it disagrees with what INDI puts on the wire. This PR updates that block to match observed client behaviour. If the document is right and INDI is wrong, this PR is wrong too — please say so and I'll close it.Verification
pio test -e native: 281 pass (269 ondevelop+ 12 added, none removed or weakened). An exhaustive sweep of all 21,600 unsigned wire values decodes exactly, except the 59 in the documented sub-degree band. Builds clean under-Werrorforoaeboardv1andramps;avr-g++ -mmcu=atmega2560 -Werror -Wconversionclean on the parser.File overlap
Same two files as #303 (
:SG), in different blocks, andfix/meade-sign-of-zerorewrites the three readers this touches. All are independently mergeable; whichever lands second gets a trivial rebase from me.