LX200 C++20 rewrite #39

andre-stefanov · 2025-10-04T21:09:15Z

No description provided.

…ce, UX principles) - Add Test-First Development (TDD) as non-negotiable principle - Establish hardware abstraction and modularity requirements - Define real-time performance standards (<100ms response, sub-arcsecond tracking) - Specify C++20 application and C driver code quality standards - Mandate LX200 protocol compliance and software integration consistency - Add performance standards, quality gates, and governance procedures - Update plan-template.md with detailed constitution check section

- Parser contract tests (TC-001 to TC-010) - Coordinate parsing tests (RA/DEC/LAT/LON/TIME/DATE) - Command family identification tests (18 families) - Integration tests (slew, query, config sequences) - Performance tests (<10ms parsing, <5μs coordinates) All tests currently FAIL as implementation does not exist yet. This is the expected TDD "red" phase.

- Public API header (lx200.hpp) with all types and functions - Parser implementation (parser.cpp) with ParserState class - Coordinate parsing (coordinates.cpp) for RA/DEC/LAT/LON/TIME/DATE - Zero dynamic allocation, RAII, constexpr validation - Incremental parsing for UART interrupt compatibility Implementation follows data-model.md and contracts. Tests should now compile and pass (TDD green phase).

- Enable full C++ stdlib in test prj.conf (CONFIG_REQUIRES_FULL_LIBCPP) - Fix ParseResult enum: Success = 0 for zassert_ok compatibility - Add operator! for ParseResult to work with Zephyr test macros - Fix test suite registration (auto-registered via ZTEST macro) Test results: 44/56 PASS (78.57%) Remaining failures (12): - test_buffer_overflow: Buffer overflow detection needs fix - test_command_with_parameters: Parameter parsing edge case - test_datetime_commands: Command family mapping for GC - test_empty_command: Empty command validation - test_error_recovery: Error recovery flow - test_feed_valid_command: Command complete detection - test_getinfo_commands: lowercase g command family - test_missing_prefix: Error handling edge case - test_parser_reset: Reset state management - test_partial_command_buffering: Incremental parsing - test_ra_format_validation: Format error detection - test_rapid_command_sequence: Sequential command handling These failures indicate edge cases and refinements needed in: 1. Parser state machine (command completion, error recovery) 2. Command family identification (DateTime, GPS) 3. Parameter extraction logic 4. Input validation (buffer overflow, format errors)

…nate Major API improvements to the LX200 C++20 implementation: **API Modernization:** - Replace C-style const char* with std::string_view in all parsing functions - parse_ra_coordinate() - parse_dec_coordinate() - parse_latitude_coordinate() - parse_longitude_coordinate() - parse_time_value() - parse_date_value() - Remove operator!(ParseResult) - use explicit comparisons instead - Update all tests to use zassert_equal() with ParseResult::Success **Structure Optimization:** - Remove redundant 'tenths' field from RACoordinate (4 bytes → 3 bytes) - Unify low/high precision: both now use hours/minutes/seconds - Convert low precision tenths to seconds: tenths * 6 = seconds (0.1 arcminute = 6 arcseconds) - Add conversion table documentation **Implementation Improvements:** - Replace C pointer arithmetic with string_view methods - Use string_view::find() instead of custom find_char() helper - Use string_view::substr() for parsing instead of pointer manipulation - Better error handling: separate format vs range validation - Remove obsolete find_char() helper function **Benefits:** - More idiomatic modern C++ (C++17+ style) - Better type safety with string_view - Reduced memory footprint (3 bytes vs 4 bytes per RA coordinate) - Cleaner, more maintainable code - No performance regression All 56 tests passing (100% pass rate). Closes #T031 test verification phase.

Copilot

Pull Request Overview

This PR rewrites the LX200 protocol implementation from the prior C version to a modern C++20 version, adding a new parser, coordinate parsing utilities, and a comprehensive test suite aligned with specification/contracts.
Key changes include:

Replacement of C headers/sources with C++20 implementations (parser, coordinates, command model) and updated build system (CMake, test configuration).
Addition of extensive unit, integration, performance, and contract-style tests; removal of legacy unimplemented C tests.
Introduction of specification, planning, contracts, and task template documents to formalize development.

Reviewed Changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
tests/lib/lx200/testcase.yaml	Replaces single test entry with multiple categorized test scenarios and tags.
tests/lib/lx200/src/test_parser.cpp	Adds parser contract test coverage (init/reset, buffering, errors, overflow, parameters, precision, sequencing).
tests/lib/lx200/src/test_integration.cpp	Adds end-to-end scenario tests (slew, position query, site config, movement, recovery, performance).
tests/lib/lx200/src/test_coordinates.cpp	Adds coordinate/time/date parsing and performance tests.
tests/lib/lx200/src/test_commands.cpp	Adds command family identification & lookup performance tests.
tests/lib/lx200/src/main.cpp	Defines consolidated test suite entry (ZTEST_SUITE).
tests/lib/lx200/prj.conf	Enables C++20, logging, and LX200 config for tests.
tests/lib/lx200/CMakeLists.txt	Switches test build to C++ sources; adds compile flags.
lib/lx200/parser.cpp	Implements incremental parser, command extraction, family classification heuristics.
lib/lx200/coordinates.cpp	Implements RA/Dec/Lat/Lon/Time/Date parsing logic.
lib/lx200/lx200.cpp	Provides namespace/unit anchor for LX200 module.
lib/lx200/CMakeLists.txt	Replaces C build with C++20 sources and flags; sets include paths.
include/lx200/lx200.hpp	New C++20 public API (types, parser class, parse functions).
Removed C sources/headers (lx200.c, lx200.h, C test files)	Cleans out legacy C implementation and tests.
specs/* (spec.md, plan.md, data-model.md, contracts, tasks, quickstart, research)	Adds formal architecture/design/contract documentation.
OpenAstroFirmware.code-workspace	Updates editor settings for C++20 support.
.specify/templates/*	Adds templates supporting structured plan/spec/task generation.

lib/lx200/src/coordinates.cpp

lib/lx200/src/lx200.cpp

lib/lx200/parser.cpp

lib/lx200/CMakeLists.txt

tests/lib/lx200/src/test_parser.cpp

This commit fixes all critical issues identified in PR review: 1. DEC 90° Boundary Validation (coordinates.cpp:169-207) - Added validation to reject 90° with non-zero arcminutes/arcseconds - Implemented for both high precision (±90*00:00) and low precision (±90*00) - Handles both positive and negative poles correctly 2. LAT 90° Boundary Validation (coordinates.cpp:275-279) - Added validation to reject 90° with non-zero arcminutes - Poles must have exactly zero arcminutes (±90*00) 3. Date Validation (coordinates.cpp:458-481) - Implemented per-month day count validation - Added leap year logic (divisible by 4, century rule) - Rejects impossible dates: 02/30, 04/31, 06/31, etc. - Correctly handles February 29th in leap years 4. string_view Lifetime Safety (lx200.hpp:221-250, parser.cpp:96-100) - Added comprehensive documentation warnings - Documented safe vs unsafe usage patterns - Clarified that Command views become invalid after next feed_character() Test Coverage: - Added test_dec_90_degree_boundary (10 assertions) - Added test_latitude_90_degree_boundary (6 assertions) - Added test_date_month_day_validation (22 assertions) - Added test_date_leap_year_validation (9 assertions) - All 60 tests passing (100%) These fixes ensure production-ready code quality by: - Preventing invalid astronomical coordinates at boundaries - Rejecting impossible calendar dates - Documenting memory safety requirements clearly Addresses review comments from GitHub Copilot in PR #39.

Copilot

Pull Request Overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 12 comments.

lib/lx200/src/coordinates.cpp

lib/lx200/src/lx200.cpp

lib/lx200/CMakeLists.txt

Phase 2 improvements: - Fix CMakeLists.txt hardcoded path: use ${PROJECT_SOURCE_DIR} instead of hardcoded relative path - Refactor parameter parsing: replace if-else chain with lookup table for better maintainability Phase 3 improvement: - Expose MAX_COMMAND_LENGTH via constexpr accessor for testability All 240 tests passing (100%) Addresses PR review feedback on code quality and portability

Copilot

Pull Request Overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 13 comments.

Comments suppressed due to low confidence (1)

lib/lx200/coordinates.cpp:1

RA parsing does not validate that the entire token is consumed: inputs like '12:34:56XYZ' or '12:34.5extra' will be accepted because only fixed digit counts are parsed. After numeric extraction, verify there are no trailing unexpected characters (e.g., check second_part.size()==2 and tenth_part.size()==1) to reject malformed inputs per contract intent.

/*

tests/lib/lx200/src/test_parser.cpp

lib/lx200/src/lx200.cpp

lib/lx200/src/coordinates.cpp

include/lx200/lx200.hpp

- Create src/ directory for implementation files - Consolidate parser.cpp into lx200.cpp (no need for separation) - Move coordinates.cpp to src/ - Update CMakeLists.txt to reference src/ directory All 240 tests still passing (100%)

Addresses Copilot review comments - parsing functions now reject trailing garbage characters: - RA: '12:34:56XYZ' now rejected (only '12:34:56' accepted) - DEC: '+45*30:15XYZ' now rejected - Latitude: '+45*30X' now rejected - Longitude: '123*45abc' now rejected - Date: '03/15/23XYZ' now rejected - Time: '12:34:56xyz' now rejected All coordinate/date/time substrings are now validated to be exactly the expected length before numeric parsing. Fixes discussion r2404181093 and related validation issues. All 240 tests passing.

Updated command reference to include comprehensive OpenAstroTech firmware extensions alongside the standard Meade LX200 protocol specification. Changes: - Added OAT column to command support matrix - Documented 50+ OAT-specific X-family commands - Added Appendix B: OpenAstroTech Extensions with detailed specs - Added Appendix C: OAT vs Meade behavioral differences - Documented OAT enhancements to standard commands (GPS, Movement, Focus) - Included Hall sensor homing, digital level, backlash compensation - Added mount status queries, stepper control, and configuration commands Based on OpenAstroTech wiki documentation (Firmware V1.13.12). This provides a comprehensive reference for both standard Meade LX200 compatibility and OAT-specific advanced features.

Fixed critical bug where :gT# was incorrectly hardcoded as GetInfo family instead of GPS family. This protocol-level bug was discovered through comprehensive documentation comparison with LX200CommandSet.md. The LX200 protocol uses case-sensitive command families: - :gT# (lowercase g) = GPS family: 'Set Mount Time from GPS' Blocking call up to 2 minutes, returns 1 (success) or 0 (timeout) - :GT# (uppercase G) = GetInfo family: 'Get tracking rate' Instant query returning tracking frequency Bug Impact: - Misrouting could cause 2-minute hangs when expecting instant query - Firmware command dispatch would route to wrong handler - Integration with ASCOM/INDI/Stellarium could fail unexpectedly Changes to lib/lx200/src/lx200.cpp: - Removed incorrect special case forcing :gT# to GetInfo family - Added clarifying comments about lowercase vs uppercase distinction - Now correctly routes based on first character ('g' vs 'G') Changes to tests/lib/lx200/src/test_commands.cpp: - Split test_getinfo_commands to test only uppercase G commands - Created test_gps_commands for lowercase g commands * :gT# - Set mount time from GPS * :gTnnn# - Set mount time with timeout parameter [OAT] * :g+# - Turn on GPS power * :g-# - Turn off GPS power - Created test_library_commands for L family * :LI# - Get object information * :LMNNNN# - Select Messier object with parameter extraction - Created test_extended_oat_commands for X family (OAT extensions) * :XFR# - Factory reset - Added comprehensive documentation for each test explaining: * Command purpose and behavior * References to LX200CommandSet.md appendices * Critical distinctions (blocking vs instant, case sensitivity) * OAT-specific extensions vs standard Meade commands Test Results: - All 248/248 tests passing (100%) - Added 8 new test cases (+3.3% coverage) - GPS family: 4 commands tested - Library family: 2 commands tested (+ parameter extraction) - Extended family: 1 command tested - GetInfo family: corrected to test :GT# (uppercase) instead of :gT# This fix addresses documentation comparison findings and ensures proper command routing in the C++20 rewrite. Case sensitivity in LX200 protocol is critical for correct telescope control behavior.

- Remove ParseResult enum, add Result<T> based on std::variant - Update all parsing functions to return Result<T> or VoidResult - Implement functional error handling with Ok()/Err() factories - Add match() and value_or() methods for functional composition - Rewrite all 98 tests for Result<T> API (100% passing) Breaking changes: - ParseResult enum removed entirely - All parse functions now return Result<T> instead of out-parameters - feed_character() returns VoidResult instead of ParseResult

- Add comprehensive tests for all 18 command families - Add case-sensitivity test (G/g, H/h distinction) - Enhance Movement, GetInfo, GPS, SetInfo, Tracking tests - Fix command family classification for H and S commands - All 101 tests passing (100% success)

CommandFamily::Backup should be CommandFamily::Reticle. The 'B' commands control reticle brightness and accessories, not backup operations. Matches LX200CommandSet.md Section B.

Updated CommandFamily enum to accurately reflect all command families documented in LX200CommandSet.md: Added command families: - Fan ('f') - Fan/power control (LX200GPS/LX16") - HourFormat ('H') - Time format toggle - Derotator ('r') - Field de-rotator (LX16") - Waypoint ('W') - Site/waypoint commands (LX200GPS) - Help ('?') - Help commands (LX200GPS/LX16") Renamed command families: - DateTime → Sync ('C') - Per spec "Sync Control" - Removed special case handling for date/time commands Updated tests: - Sync commands now use CommandFamily::Sync - Hour format uses CommandFamily::HourFormat - Date/time GET commands (GC, GL) correctly use GetInfo family - Date/time SET commands (SC, SL) correctly use SetInfo family - All 101 tests passing

Add CommandFamily::Backlash and CommandFamily::SmartDrive to support LX200GPS special commands that use two-character designators starting with '$'. Protocol additions: - $B family: Active backlash compensation (:$BAdd#, :$BZdd#) - $Q family: Smart Drive PEC control (:$Q#, :$QA+#, :$QZ-#, etc.) Implementation changes: - Updated CommandFamily enum with Backlash('$') and SmartDrive('@') - Modified identify_family() to accept string_view instead of char - Added special handling for $-prefixed commands (two-char lookup) - Updated parse_command_parts() to handle $XY command pattern - Commands like :$BA15# now parse as name="$BA", params="15" Tests: - Added test_backlash_commands with 2 test cases - Added test_smartdrive_commands with 3 test cases - All 103 tests passing (25 command tests, 51 coordinate, 17 integration, 10 parser) This completes full LX200 protocol support including all standard and special command families from the Meade specification.

Remove character literal assignments from CommandFamily enum values. These were never used functionally - the enum serves purely as a type-safe categorization, while identify_family() does the actual character-to-family mapping. Benefits: - Simpler and more idiomatic C++ (compiler auto-assigns 0,1,2...) - More accurate: Backlash/SmartDrive aren't single characters - Clearer intent: enum is for type safety, not character storage - Smaller and more cache-friendly with sequential values The character mappings are documented in comments and implemented in identify_family() where they actually matter. All 103 tests still passing (25+51+17+10).

@Par

Add detailed documentation to the Result<T> functional interface: Class-level documentation: - Explains Rust-style error handling philosophy - Lists key benefits (type-safe, composable, zero-cost, explicit) - Provides 5 comprehensive usage examples: * Basic usage with is_ok()/value()/error() * Using value_or() for safe defaults * Pattern matching with match() * Chaining operations functionally * Creating Results with Ok()/Err() Method-level documentation: - is_ok(), is_error(): Boolean checks - value(): Extract success value (with safety notes) - error(): Extract error value - value_or(): Safe extraction with fallback - match(): Pattern matching with detailed examples Factory function documentation: - Ok(): Creating success Results (with/without values) - Err(): Creating error Results (with type specification notes) - VoidResult: Explained use case for void operations Examples show real-world usage patterns including: - Parse validation with error handling - Logging both success and error cases - Transforming Results with callbacks - Safe extraction without exceptions - Chaining operations functionally All documentation uses @Par example blocks for clarity. Tests continue passing (103/103).

Synchronized documentation with the actual firmware implementation from: https://github.com/OpenAstroTech/OpenAstroTracker-Firmware/blob/main/src/MeadeCommandProcessor.cpp Added missing commands: - :Qa# - Halt all direction slews (OAT extension) - :XGDP# - Get DEC parking position (obsolete/disabled) - :XSDP# - Set DEC parking position offset (obsolete/disabled) - :XGO# - Get log buffer (debugging command) Enhanced documentation: - Added source reference link to firmware repository - Added note about firmware being authoritative source - Enhanced :XGDLx# with parameter documentation - Enhanced :XSDLl# with 'if not configured' clarification All changes verified against MeadeCommandProcessor.cpp implementation.

Copilot

Pull Request Overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 6 comments.

lib/lx200/src/coordinates.cpp

tests/lib/lx200/src/test_coordinates.cpp

Copilot · 2025-10-06T11:27:25Z

lib/lx200/src/lx200.cpp

+VoidResult ParserState::feed_character(char c) noexcept
+{
+    // Check for buffer overflow
+    if (buffer_length_ >= MAX_COMMAND_LENGTH) {


[nitpick] On buffer overflow the parser returns BufferFull but leaves existing partial content intact; this can trap the parser in an unrecoverable state unless the caller manually resets. Consider invoking reset() (preserving precision_) before returning the error so the next well-formed command can start immediately.

Suggested change

if (buffer_length_ >= MAX_COMMAND_LENGTH) {

if (buffer_length_ >= MAX_COMMAND_LENGTH) {

reset(); // Clear buffer and state, but preserve precision_

Copilot · 2025-10-06T11:27:25Z

lib/lx200/src/lx200.cpp

+    // Extract command string (skip ':' prefix and '#' terminator)
+    std::string_view full_command(buffer_.data() + 1, buffer_length_ - 2);
+
+    // Split into name and parameters
+    std::string_view name, params;
+    parse_command_parts(name, params);


[nitpick] The local full_command variable is computed but not used; parse_command_parts recomputes the same slice from buffer_. Pass full_command into parse_command_parts (or incorporate its logic here) to eliminate duplication and reduce risk of divergence if slicing rules change.

- Implement month-specific day limits (30/31 days per month) - Add leap year calculation for Gregorian calendar (2000-2099) - Validate February days (28 or 29 based on leap year) - Add 26 new date validation tests covering edge cases - Test invalid combinations (Feb 30, Apr 31, Jun 31, etc.) - Test leap year boundaries (2020, 2024, 2028, 2000 valid) - All 128 tests passing (76 coordinate tests, up from 51)

andre-stefanov added 8 commits October 4, 2025 20:31

refactor: remove C implementation of LX200 parser for C++20 rewrite

fe25e26

build: configure C++20 build system for LX200 rewrite

70891ea

style: reformat code

0bbfec4

Copilot AI review requested due to automatic review settings October 4, 2025 21:09

refactor(main): remove unused lx200 header inclusion

6bdfdff

Copilot AI reviewed Oct 4, 2025

View reviewed changes

andre-stefanov requested a review from Copilot October 4, 2025 21:48

Copilot AI reviewed Oct 4, 2025

View reviewed changes

andre-stefanov requested a review from Copilot October 4, 2025 22:15

Copilot AI reviewed Oct 4, 2025

View reviewed changes

andre-stefanov added 13 commits October 5, 2025 12:04

refactor: reorganize source structure

8b852f1

- Create src/ directory for implementation files - Consolidate parser.cpp into lx200.cpp (no need for separation) - Move coordinates.cpp to src/ - Update CMakeLists.txt to reference src/ directory All 240 tests still passing (100%)

fix: Rename Backup to Reticle command family

b16023d

CommandFamily::Backup should be CommandFamily::Reticle. The 'B' commands control reticle brightness and accessories, not backup operations. Matches LX200CommandSet.md Section B.

style: Format test_integration.cpp code

82b9fd3

andre-stefanov requested a review from Copilot October 6, 2025 11:24

Copilot AI reviewed Oct 6, 2025

View reviewed changes

	if (buffer_length_ >= MAX_COMMAND_LENGTH) {
	if (buffer_length_ >= MAX_COMMAND_LENGTH) {
	reset(); // Clear buffer and state, but preserve precision_

LX200 C++20 rewrite #39

Are you sure you want to change the base?

LX200 C++20 rewrite #39

Uh oh!

Conversation

andre-stefanov commented Oct 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!