Conversation
|
I would contend that this is a INDI defect. The Meade Telescope Serial Command Protocol, Revision 2010.10 from 7 October 2010, is quite explicit that the command is |
ClutchplateDude
left a comment
There was a problem hiding this comment.
See comment on main page.
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
91991da to
f976a99
Compare
Cursor::signed2 requires a sign and exactly two digits, so the ":SG+7.0#" that INDI puts on the wire was refused and the mount kept whatever offset it already had -- local sidereal time then out by the whole offset, with no indication anything had failed. readUtcOffset takes the sign, one or two digits, and leaves the remainder unconsumed, so "+7.0", "+7" and "+07" all set +7. The sign stays required, as MeadeProtocol.hpp specifies. Pre-OpenAstroTech#291 the value went through String::toInt(), which accepts an unsigned number, so this is a narrowing rather than a restoration -- but a missing sign is much more likely a client bug than a deliberate "+", and guessing wrong puts sidereal time out by twice the offset. (That same path also returned "1" for ":SGx#" having stored 0; this rejects it.) Half-hour zones are still unrepresentable, and deliberately so here: the offset is a single marker-gated signed byte in a packed EEPROM map with no unit tag, and DayTime::addHours takes a float, so re-unitising the stored value would compile clean and silently double the error on any mount flashed from an older build. That is a storage change and belongs in its own commit. Also fixes the :SG header line, which still read ":SGsHH#". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz
f976a99 to
1a1e615
Compare
Fair on all three, and I agree it's an INDI deviation rather than a mount defect. I've put that in the The sign is mandatory again. One test flipped; everything else already sent one. I also fixed the header line, which still read One thing I'd rather mention than quietly skip: pre-291 the value went through On 0.5, I'd like to do that as a follow-up PR, because the parser part is the easy half:
Three questions, since they're your calls:
Rebased on top of #304. |
Regression from #291. INDI cannot set the site's UTC offset, so local time is wrong for every INDI/KStars session.
The defect
The
:SGarm ofhandleMeadeSet()usesCursor::signed2(), which requires a leading+/-and exactly two digits. INDI sends one digit and a fractional part.Captured off the wire on an INDI connect to an OpenAstroExplorer:
:SG+7.0#is answered0, the handler never fires, and the offset is silently left unset. The pre-#291 handler rantoInt()over the remainder (inCmd.substring(1, 4).toInt()), which accepted all of these forms.The fix
A
readUtcOffset()helper alongside the otherread*helpers, accepting an optional sign and one or two digits, and leaving trailing characters unconsumed so+7.0parses as+7— matching the pre-#291 behaviour rather than inventing a new grammar.Every form that already parsed behaves identically, sign included;
+05→ 5 and-08→ −8 on both sides of the change.Tests
Added under the existing
---- UTC Offset (G) ----block: the exact INDI form; single-digit positive and negative (the suite never covered that intersection); unsigned; and the+5.5half-hour case (India, Newfoundland), which truncates becausesetLocalUtcOffset()takes anint— pinned so the limitation is documented rather than discovered in the field.One existing test asserted that
G+5was rejected. That was pinning the regression, since the pre-#291 handler accepted it, so it is repointed at genuinely malformed input rather than removed; the rejection path is now covered by two tests instead of one.pio test -e native: 277 pass (269 ondevelop+ 9 added − 1 repointed). Builds clean foroaeboardv1andramps.Deliberately not included
No range validation.
:SG+13/:SG-15are accepted ondeveloptoday and the tests pin that as-is — adding a-14..+14check would change behaviour beyond the reported fault and could reject something a real client sends. Happy to fold it in if you'd rather have it here.Scope is
:SGonly.:Sg(site longitude) has a related but separate #291 regression with a contested sign convention, so it is not touched here.File overlap
Touches the same two files as my
:Sgchange (MeadeParserSet.cpp,MeadeProtocol.hpp), in different blocks. Each is mergeable on its own; whichever lands second will need a trivial rebase and I'm happy to do it.