alignment: add SPK physics-based math plugin#2376
Conversation
17d2542 to
ea5bb05
Compare
|
I am not sure about the proper way to introduce a new dependency. It looks like CI does not have ERFA. The homebrew package for ERFA is under review here: Homebrew/homebrew-core#279160 |
There was a problem hiding this comment.
Pull request overview
Adds a new physics-based SPK alignment math plugin (Wallace pointing kernel) to INDI’s alignment subsystem, including a vendored SPK C implementation built against ERFA via a SOFA-API shim, plus simulator/test enhancements to validate conventions and regression cases.
Changes:
- Introduces
SPKMathPluginwith cold-path (Pmfit) and hot-path (incremental normal-equation) fitting, and integrates it into build/test targets with an ERFA dependency. - Vendors Wallace SPK kernel sources and documentation, with small INDI-specific modifications (ERFA shim, TF/VD handling, basis-term ordering).
- Expands alignment tests and simulator behaviors to validate coordinate/time conventions, meridian flip behavior, and noise/time-shift regressions.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/alignment/test_alignment_plugins.cpp |
Adds extensive SPK-focused regression/validation tests and supporting helpers (noise, time shift, convention checks). |
test/alignment/CMakeLists.txt |
Links plugin tests against ERFA and compiles SPK sources into the test binary. |
libs/alignment/spk/vtel.c |
Adds vendored SPK VT solver with an INDI modification to TF/VD application. |
libs/alignment/spk/spk.h |
Introduces SPK public API/types and adds Pmfit-related prototypes for INDI use. |
libs/alignment/spk/sofa.h |
Adds SOFA→ERFA compatibility shim header used by vendored SPK sources. |
libs/alignment/spk/s2e.c |
Adds vendored “one-call” SPK wrapper (reference/demo support). |
libs/alignment/spk/point.c |
Adds vendored demo program (not compiled into plugin). |
libs/alignment/spk/pmfit_equat.dat |
Adds sample equatorial Pmfit dataset (reference). |
libs/alignment/spk/pmfit_altaz.dat |
Adds sample altaz Pmfit dataset (reference). |
libs/alignment/spk/pmfit.c |
Adds vendored Pmfit + basis functions and Simeqn solver, with INDI term-order changes. |
libs/alignment/spk/equat.c |
Adds vendored equatorial demo program (not compiled into plugin). |
libs/alignment/spk/ctar.c |
Adds vendored catalog-target transform helper. |
libs/alignment/spk/astr.c |
Adds vendored astrometry setup helper (spkAstr). |
libs/alignment/spk/altaz.c |
Adds vendored altaz demo program (not compiled into plugin). |
libs/alignment/spk/README.md |
Documents provenance, licensing, file roles, and INDI-specific modifications. |
libs/alignment/SPKMathPlugin.h |
Declares the new SPK alignment plugin and its incremental-fit state. |
libs/alignment/SPKMathPlugin.cpp |
Implements SPK plugin transforms, fitting logic, and ERFA/libnova time synchronization. |
libs/alignment/CMakeLists.txt |
Builds and installs indi_SPK_MathPlugin, adding ERFA as a link dependency. |
libs/alignment/ARCHITECTURE.md |
Adds detailed subsystem architecture notes, especially SPK conventions/time sync. |
drivers/telescope/telescope_simulator.cpp |
Adds EQ goto logging and fallback warning when alignment transform fails. |
drivers/ccd/guide_simulator.cpp |
Improves PE snoop activation gating (only enable on Ok state) + one-time warning. |
drivers/ccd/ccd_simulator.cpp |
Same PE snoop gating + one-time warning as guide simulator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| find_package(PkgConfig REQUIRED) | ||
| pkg_check_modules(ERFA REQUIRED IMPORTED_TARGET erfa) | ||
|
|
||
| ADD_EXECUTABLE(test_alignment_plugins | ||
| test_alignment_plugins.cpp | ||
| ../../libs/alignment/BuiltInMathPlugin.cpp | ||
| ../../libs/alignment/SVDMathPlugin.cpp | ||
| ../../libs/alignment/NearestMathPlugin.cpp | ||
| ../../libs/alignment/BasicMathPlugin.cpp | ||
| ../../libs/alignment/SPKMathPlugin.cpp | ||
| ../../libs/alignment/spk/vtel.c | ||
| ../../libs/alignment/spk/pmfit.c | ||
| ../../libs/alignment/spk/astr.c |
There was a problem hiding this comment.
The alignment plugin test target now hard-requires ERFA at configure time via pkg_check_modules(ERFA REQUIRED ...), which will prevent building/running the test suite on environments that don't have ERFA packaged yet. If ERFA is intended to be optional for the overall project, consider making this conditional on ERFA availability (or on an ENABLE_SPK option) and skipping only the SPK-related tests when missing.
There was a problem hiding this comment.
see my comment below about making ERFA a requirement.
| spkTAR tar; | ||
| tar.sys = APPT; | ||
|
|
||
| double tara, tare, tarr, tarp, soln[5]; | ||
| int status = spkVtel(TARG, &m_Obs, &m_Opt, &m_PM, &m_Ast, &ax3, &tar, &m_PO, | ||
| &tara, &tare, &tarr, &tarp, soln); |
There was a problem hiding this comment.
spkTAR tar is only setting tar.sys before calling spkVtel(TARG, ...). Inside spkVtel the code unconditionally reads tar->a/tar->b to compute the observed target place, so leaving them uninitialized is undefined behavior and can yield NaNs/non-deterministic results. Initialize tar.a and tar.b to a valid value (even a dummy 0/0) or restructure the call so spkVtel never sees uninitialized target coordinates.
|
|
||
| // Use official spkAstr | ||
| spkAstr(1, utc, 0.0, &eop, &m_Obs, &air, &m_Opt, &m_Ast); | ||
|
|
||
| // Synchronize SOFA internal "clock" (ERA) with libnova LST | ||
| // This ensures consistency with the simulator's view of HA/Az. | ||
| double gmst_hrs = ln_get_apparent_sidereal_time(JD); | ||
| double lst_rad = HOURS_TO_RAD(range24(gmst_hrs + RAD_TO_HOURS(m_Obs.slon))); | ||
| m_LST_rad = lst_rad; |
There was a problem hiding this comment.
spkAstr(...) returns a status code (including negative errors) but the return value is ignored. If it fails, m_Ast may be left in an invalid state and subsequent spkVtel calls will operate on garbage. Check the return code and either fail the transform/initialise path or fall back to a safe behavior when spkAstr returns < 0.
| // Use official spkAstr | |
| spkAstr(1, utc, 0.0, &eop, &m_Obs, &air, &m_Opt, &m_Ast); | |
| // Synchronize SOFA internal "clock" (ERA) with libnova LST | |
| // This ensures consistency with the simulator's view of HA/Az. | |
| double gmst_hrs = ln_get_apparent_sidereal_time(JD); | |
| double lst_rad = HOURS_TO_RAD(range24(gmst_hrs + RAD_TO_HOURS(m_Obs.slon))); | |
| m_LST_rad = lst_rad; | |
| // Synchronize SOFA internal "clock" (ERA) with libnova LST | |
| // This ensures consistency with the simulator's view of HA/Az. | |
| double gmst_hrs = ln_get_apparent_sidereal_time(JD); | |
| double lst_rad = HOURS_TO_RAD(range24(gmst_hrs + RAD_TO_HOURS(m_Obs.slon))); | |
| m_LST_rad = lst_rad; | |
| // Initialize astrometry storage before calling spkAstr so failures cannot | |
| // leave stale or uninitialized data behind. | |
| std::memset(&m_Ast, 0, sizeof(m_Ast)); | |
| // Use official spkAstr and honor its status code. | |
| int spkStatus = spkAstr(1, utc, 0.0, &eop, &m_Obs, &air, &m_Opt, &m_Ast); | |
| if (spkStatus < 0) | |
| { | |
| // Fall back to a safe, deterministic state instead of using invalid | |
| // astrometry output from a failed initialization. | |
| std::memset(&m_Ast, 0, sizeof(m_Ast)); | |
| m_Ast.astrom.eral = iauAnp(lst_rad); | |
| return; | |
| } |
|
|
||
| int Pmfit ( double, char, int, double*, int, | ||
| double*, double*, double* ); | ||
|
|
||
| /* Internal helper Bfun prototype */ | ||
| int Bfun ( int, double, char, double, double, double* ); | ||
| int Simeqn (int, double*, double* ); | ||
|
|
||
| /*--------------------------------------------------------------------*/ | ||
|
|
||
| #include <sofa.h> | ||
|
|
||
| int Bfun ( int, double, char, double, double, double* ); | ||
| int Simeqn (int, double*, double* ); | ||
|
|
There was a problem hiding this comment.
pmfit.c currently includes both the local ERFA shim ("sofa.h") and then later #include <sofa.h>, and also repeats multiple forward declarations (Bfun, Simeqn, Pmfit). This is redundant and makes it easier to accidentally pick up a system SOFA header (or otherwise change include resolution) rather than the intended ERFA shim. Remove the second <sofa.h> include and the duplicate prototypes, keeping a single include path through the shim.
| int Pmfit ( double, char, int, double*, int, | |
| double*, double*, double* ); | |
| /* Internal helper Bfun prototype */ | |
| int Bfun ( int, double, char, double, double, double* ); | |
| int Simeqn (int, double*, double* ); | |
| /*--------------------------------------------------------------------*/ | |
| #include <sofa.h> | |
| int Bfun ( int, double, char, double, double, double* ); | |
| int Simeqn (int, double*, double* ); | |
| int Pmfit ( double, char, int, double*, int, | |
| double*, double*, double* ); | |
| /*--------------------------------------------------------------------*/ |
There was a problem hiding this comment.
I am trying to leave this as close as possible to the original source of the vendored library, but I had to make some changes to improve the fit for a smaller number of alignment points. I reverted some of the changes I made, so that I now only include sofa.h once.
| find_package(PkgConfig REQUIRED) | ||
| pkg_check_modules(ERFA REQUIRED IMPORTED_TARGET erfa) | ||
|
|
||
| add_library(indi_SPK_MathPlugin SHARED ${SPKMathPlugin_SRCS}) | ||
| target_include_directories(indi_SPK_MathPlugin | ||
| PRIVATE | ||
| ${CMAKE_SOURCE_DIR}/libs/indicore | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/spk | ||
| ) | ||
| target_link_libraries(indi_SPK_MathPlugin | ||
| indidriver | ||
| PkgConfig::ERFA | ||
| ${NOVA_LIBRARIES} | ||
| $<$<PLATFORM_ID:CYGWIN>:AlignmentDriver> | ||
| ) |
There was a problem hiding this comment.
pkg_check_modules(ERFA REQUIRED ...) makes configuring the project (and tests) fail outright on systems without ERFA installed, even if a user doesn't need the SPK plugin. Consider making ERFA detection/build optional (e.g., a CMake option to enable SPK, or using QUIET + conditional add_library), and emit a clear message when the plugin is disabled due to missing ERFA.
|
Finally a pointing model based on mount modelling. We should do whatever it takes to merge this. This is of particular value for the drivers which depend on pointing model like celestron AUX - where the pointing model does the whole heavy lifting part. Thank you. |
|
I have generated a copilot review for the PR. Some comments seem to be valid. Please review them and, if indeed valid, correct the code. |
You are welcome. Using this for Celestron AUX is my personal goal :-). This math plugin should behave nicely for tracking. Take a look at the tracking implementation I added to the telescope simulator last week. It implements a parabolic fit over the pastm current and future position to determine the current servo rates. According to the tests it tracks with very low error rates. I am thinking to try that for Celestron AUX as well. |
|
Compiling as we speak ;) |
5e55b12 to
1e15ae7
Compare
|
I tested it tonight with my C8 Evo - it was very windy today and the clouds rolled in quickly but the results were fairly promissing - as far as I can tell from very few sync points. I will be very busy next two weeks so no more tests but it seems to be an improvement. I intent to run more tests mid - may. I will report on the results. |
1e15ae7 to
dca21b6
Compare
2c4592b to
161dab6
Compare
|
@ckemper67 I am compiling the newest version from your PR, but before I finishes I have a note: it seems to start my mount in eq mode not altaz (pointed at polaris). I need to switch to some other plugin to force a northern horizon park position and after switching back to spk it stays on the horizon. I do not know what is the current state - maybe you already fixed it. |
I noticed that in the simulator as well but I thought that it was related to me fixing some bugs in EQ mode. I will take a look. |
|
See if this fixes it
I pushed a fix that makes the initialization more robust following what the other plugins are doing. But I am not 100% sure how the lifecycle of the park position works. The driver could have saved a previous park position at polaris that you still have to override now. |
Add SPKMathPlugin, a new alignment math plugin based on Patrick Wallace's SPK pointing kernel. Vendor the SPK C library (spk/ subdirectory) under its original license.
…r axis mapping Implement the full 6-term Pmfit least-squares fitting loop with hot-path incremental normal-equation accumulation. Apply the altaz roll convention (pi - Az, matching vtel Note 7). Add call to MathPlugin::Initialise() so the base class sanitizes sync points before Pmfit runs. Activate EQUATORIAL_PE output in the CCD/guide simulators only when tracking state is Ok, so plate solves see a stable star field. Map ME (polar elevation error, pmv[2]) to pan/AN (along-meridian tilt) and MA (polar azimuth error, pmv[3]) to paw/AW (across-meridian tilt) in ParsePmfitCoefficients, following Wallace PM struct conventions. Use RA-based (ANTI_CLOCKWISE) TDV encoding throughout: telescope_simulator Sync/Goto, MathPlugin polar sanitizer, and SPKMathPlugin boundary functions. Store LST in UpdateAstrometry so DirectionVectorToRollPitch and RollPitchToDirectionVector can convert between RA-based TDVs and the HA-based roll/pitch that the Wallace kernel expects internally. Add altaz/EQ polar degeneracy and regression tests. Update ARCHITECTURE.md to document the Wallace error injection pipeline.
- Add JD-Shift tests to verify sidereal time handling over several hours. - Add physical encoder mapping tests for Alt-Az and Equatorial mounts. - Add Southern Hemisphere regression to verify geometric symmetry. - Refactor buildEntry to support arbitrary Julian Dates in simulations.
Fix undefined behavior in TransformTelescopeToCelestial: spkVtel reads tar->a and tar->b unconditionally before its internal mode switch, so leaving them uninitialized in TARG mode was UB. Initialize both to 0.0. Check the spkAstr return code in UpdateAstrometry. Zero m_Ast before the call so a failure cannot leave stale data behind; on error, seed eral from the libnova LST so subsequent spkVtel calls see a coherent ERA rather than garbage, then return early. Restore pmfit.c closer to the original vendored source.
Guard the SPK math plugin build behind if(ERFA_FOUND) so the rest of the build succeeds on systems where liberfa-dev is not yet installed. The plugin is simply skipped if erfa is absent.
UpdateObsConfig() is now called at the start of every transform rather than only in Initialise(). This means the mount type and observer location are always current, matching how the matrix plugins (BasicMath, Nearest) read ApproximateMountAlignment on each call. The specific bug: celestronaux calls Initialise() on physical connection with the SetApproximateMountAlignmentFromMountType call commented out, so a stale EQUAT mode could persist in m_Obs.mount causing an ALTAZ mount to appear pointed at the NCP. UpdateObsConfig() now returns bool and guards against a null pInMemoryDatabase and a missing reference position. Both transform functions propagate the failure, matching the defensive pattern in BasicMathPlugin and NearestMathPlugin. Initialise() treats a missing location as "model not yet ready" (returns true) rather than a hard error, consistent with how it already handles zero sync points. Four new lifecycle tests cover: transforms failing without a location, Initialise succeeding without a location, late location arrival, and mount-type changes taking effect without a second Initialise.
12264f2 to
0ceb6e8
Compare
|
Let's keep it optional for now as not to not break all package builds. We can later revise this. |
|
@knro @ckemper67 I am glad it got merged. My feeling is very positive about it. I do not think it is perfect but the behavior of the mount is substantially improved. In my opinion workining on polishing this into the default plugin is a worthwile goal. @ckemper67 : I have red that this library is able to provide estimates of the tracking rates. Is this true? This will be very usefull for CAUX driver- right now I am doing numerical differentiation to get the rates. It works but is potentially problematic when the rates are high and changing quickly (AIt /Az close to zenith). The analytical formula will be much better. My additional question is: how the build works now when the erfa is present and what happens when it is absent? |
What is the best place for discussions like this? I was expecting the Indi forum and I posted a message about my work four weeks ago, but never got any replies.
Tracking rates will get determined by determining the pointing the future, so that is no different than what some of the mount drivers including celestronaux are doing. But I implemented a parabolic fit instead of the linear fit in the telescope simulator's tracking implementation which results in smoother movement. That idea came out of reading one of Wallace's essays on the topic: https://www.tpointsw.uk/gbt2016.pdf
I just tested on my machine and I had a bug in the code that made the change optional. The tests still made ERFA a required dependency. I sent out #2380 to fix that. Once that is merged cmake will no longer configure SPK as a target when ERFA is absent on the machine and the SPK plugin will not get built. |

Build prerequisite: ERFA
The SPK plugin depends on ERFA (
liberfa-devon Debian/Ubuntu,erfa-develon Fedora/RHEL). Install it before configuring:On Mac, a Homebrew formula is under review. Until it lands, ERFA can be installed as a transitive dependency of another package (e.g.
brew install wcslib) or built from source.Summary
Adds a new
indi_SPK_MathPluginto the INDI alignment subsystem, implementing a global physics-based pointing model using Patrick Wallace's SPK pointing kernel. Unlike the existing SVD/BuiltIn/Nearest plugins (which fit a local mesh of sky patches), the SPK plugin fits a single unified six-term mechanical model across the full sky:The plugin uses
Pmfit(Wallace's least-squares fitter) to determine these terms from alignment sync points, andspkVtel(the SPK Virtual Telescope solver) to apply the model at transform time. Term count is gated on sync point count (2->4->5->6 terms) to maintain numerical stability.Fitting uses a two-path design. On the cold path (first fit, or after a mount-type change)
Pmfitis called directly over all sync points. Once a full 6-term model is established, the plugin switches to a hot path: each new sync point incrementally updates a set of accumulated normal equations without re-runningPmfitover the full history, keepingInitialiseO(1) per added point. The cold path is also used as a fallback if the incremental update fails.Coordinate system contract: The INDI subsystem passes coordinates in JNow (EOD) form -- what INDI calls "observed" and SOFA calls "apparent": precession, nutation, and aberration applied, but no atmospheric refraction. The plugin sets
tar.sys = APPTto match this (telling spkVtel not to re-apply those transforms) andpressure=0inspkAIRto disable spkVtel's internal refraction model. Refraction is not modeled in this pipeline.Architecture and coordinate conventions are documented in
libs/alignment/ARCHITECTURE.md.Changes also include:
EQUATORIAL_PE), allowing end-to-end validation without real hardwareVendored library: Wallace SPK kernel
The SPK pointing kernel (
libs/alignment/spk/) is vendored from Patrick Wallace's reference implementation (2024). It consists of ~700 lines of portable C covering the Virtual Telescope solver (vtel.c), astrometry (astr.c), coordinate transforms (ctar.c,s2e.c), tube flexure (vtel.c), and the Pmfit least-squares fitter (pmfit.c).License: ISC (permissive, no copyleft):
The
altaz.c,equat.c, andpoint.cfiles are standalone demo programs (each has amain()); they are not compiled into the plugin but are retained as reference for the full VT pipeline.spk.pdfandspk.texare Wallace's own documentation and serve as provenance for the implementation choices.New dependency: ERFA
The SPK kernel was written against the SOFA (
iauXxx) API. Since SOFA carries a non-standard restrictive license incompatible with LGPL distribution, the plugin uses ERFA (the ESO/Astropy open-source SOFA fork, mathematically identical). A thin header shim (libs/alignment/spk/sofa.h) maps the API at compile time without modifying Wallace's sources.On Mac, a Homebrew formula is under review. Once merged,
brew install erfawill satisfy the dependency alongside the existing Linux packages.