Repository navigation
refactor: Enhance Meade command parsing and response handling - #291
Merged
Merged
Conversation
…y layer responsibilities
- Introduced MeadeParser.hpp to define enums and structures for parsing Meade commands. - Implemented parsing functions for various command types including Get, Set, Movement, and more. - Created unit tests in test_MeadeParser.cpp to validate the parsing logic and ensure correct command classification. - Added assertions for valid and invalid command inputs, covering all command families and their respective payloads.
- Moved MeadeParser related code into a new 'meade' namespace for better organization. - Added comprehensive unit tests for MeadeParser functionality, covering various command parsing scenarios. - Implemented assertions for valid and invalid command parsing, ensuring robustness of the parser. - Included tests for all command families, including Get, Set, Sync, Movement, Home, Quit, Slew Rate, Focus, and Extra commands.
- Implemented MeadeResponse class for type-safe Meade replies with fixed capacity. - Defined response shapes using zero-size tag types in the response namespace. - Created makeResponse overloads for various response shapes. - Added traits for mapping command kinds to response tags. - Implemented entry points for responding to different command families. - Developed unit tests to validate response shapes and command bindings, ensuring correct wire formatting.
…er and MeadeResponse for AVR compatibility
- Consolidated response trait mappings into a single primary template `Response<K>`, replacing multiple family-specific templates. - Updated entry point function from `respondGet<K>` and `respondSet<K>` to a unified `respond<K>`. - Adjusted macro definitions for binding command kinds to response tags to accommodate the new structure. - Modified unit tests to reflect changes in response function calls and ensure consistent behavior across all command kinds.
- Removed MeadeGet command parsing and related tests from MeadeParser unit tests. - Introduced new unit tests for MeadeGet command handling in test_MeadeGetDispatcher.cpp. - Added comprehensive wire-byte tests for Meade Get-family commands in test_MeadeGet.cpp. - Updated MeadeResponse tests to remove redundant Get command assertions. - Ensured all tests maintain coverage for command routing and response formatting.
- Removed the MeadeSetCommandKind enum and associated MeadeSetParseResult structure from MeadeParser.hpp. - Introduced IMeadeSetHandlers interface for handling Meade Set commands, with methods for each command type. - Implemented handleMeadeSet function to parse and dispatch Meade Set commands directly, returning a MeadeResponse. - Updated MeadeResponse to remove bindings related to MeadeSetCommandKind. - Added unit tests for the new handleMeadeSet functionality, covering various command scenarios and edge cases. - Removed obsolete unit tests related to the old Meade Set command parsing.
- Implement tests for Meade Focus dispatcher covering commands such as continuous motion, MoveBy, speed settings, GetPosition, SetPosition, GetState, and Stop. - Add tests for Meade GPS dispatcher to validate GPS acquisition commands and payload handling. - Create tests for Meade Home dispatcher to verify park, slew-to-home, unpark, and setting Az/Alt home commands. - Introduce tests for Meade Init dispatcher to ensure proper handling of serial control commands. - Develop tests for Meade Movement dispatcher to cover slew, tracking, guide pulse, and stepper movement commands. - Add tests for Meade SetSlewRate dispatcher to validate mapping of slew rates to commands. - Implement tests for Meade SyncControl dispatcher to check synchronization commands and responses. - Remove outdated MeadeResponse tests as they are no longer needed.
- Removed #include "core/MeadeResponse.hpp" from multiple unit test files. - Updated includes to only reference "core/MeadeParser.hpp" where applicable.
… switch-case structure
…on and consolidating handler interfaces
…mplify coordinate reading functions
…age in handleMeadeFocus
andre-stefanov
requested review from
ClutchplateDude and
julianneswinoga
and removed request for
julianneswinoga
May 19, 2026 12:49
…overage reporting
andre-stefanov
force-pushed
the
feature/meade-parser
branch
from
May 25, 2026 19:39
0d3b5e1 to
eb154fa
Compare
ClutchplateDude
requested changes
May 26, 2026
ClutchplateDude
approved these changes
May 26, 2026
ClutchplateDude
left a comment
ClutchplateDude
left a comment
Member
There was a problem hiding this comment.
ok, good start to refactoring :-)
andre-stefanov
pushed a commit
that referenced
this pull request
Sep 1, 2026
The Meade parser refactor (#291) dropped the conversion between the mount's internal DEC axis coordinate (0 = pole above the mount) and celestial declination. As a result :GD#/:Gd# reported the raw axis coordinate and :Sd#/:CM stored wire DEC as an axis coordinate, so clients saw wrong values (e.g. +10*00'00 for celestial +80) and slews landed at the complement of the intended DEC. Move the transform into pure, unit-testable core helpers (Declination::axisToCelestialSeconds / celestialToAxisSeconds, fromTotalSeconds, DayTime::splitSeconds), expose wire-format accessors on the app-level Declination overlay (getCelestialDegrees / fromCelestialDegrees), and route all three Meade boundary handlers through them: decFrom() for :GD/:Gd, decFromWire() for :Sd and :CM. Behavior matches the pre-refactor code exactly; LCD/OLED/status paths were unaffected and are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Sep 7, 2026
rbhbokka
added a commit
to rbhbokka/OpenAstroTracker-Firmware
that referenced
this pull request
Sep 19, 2026
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>
rbhbokka
added a commit
to rbhbokka/OpenAstroTracker-Firmware
that referenced
this pull request
Sep 19, 2026
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
rbhbokka
added a commit
to rbhbokka/OpenAstroTracker-Firmware
that referenced
this pull request
Sep 19, 2026
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
rbhbokka
added a commit
to rbhbokka/OpenAstroTracker-Firmware
that referenced
this pull request
Sep 19, 2026
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
ClutchplateDude
pushed a commit
to rbhbokka/OpenAstroTracker-Firmware
that referenced
this pull request
Oct 5, 2026
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
ClutchplateDude
pushed a commit
to rbhbokka/OpenAstroTracker-Firmware
that referenced
this pull request
Oct 5, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor the Meade command parser and response system for improved organization and maintainability. Introduce new command handlers, enhance unit test coverage, and update documentation to clarify layer responsibilities. This update aims to streamline command processing and ensure robust handling of various Meade commands.