feat: Add runtime validation for option types by robertlipe · Pull Request #1466 · GPSBabel/gpsbabel

robertlipe · 2025-10-01T07:44:24Z

This PR introduces a runtime validation mechanism to catch mismatches between declared and actual option types in arglist_t tables.

Key changes:

A get_type() virtual method was added to the Option class hierarchy to enable runtime type identification.
Validation logic was added to Vecs::validate_args to compare the declared argtype with the actual Option type, calling gbFatal on a mismatch.
This addresses an issue where an incorrect argtype in the kalman filter's options was not caught at startup.
The Option class hierarchy was refactored to fix various build errors, including breaking a circular dependency between defs.h and option.h.

codacy-production · 2025-10-01T08:51:49Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01%	✅ 77.50%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`cd991b4`)	27512	17189	62.48%
Head commit (`29c3155`)	55103 (+27591)	34435 (+17246)	62.49% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1466)	40	31	77.50%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

Add a `get_type()` virtual method to the `Option` class hierarchy to enable runtime type identification. Use this new method in `Vecs::validate_args` to validate that the `argtype` specified in `arglist_t` tables matches the actual type of the `Option` object. This will catch configuration errors at startup, such as the one that was present in the kalman filter. This change also includes several fixes to the `Option` class hierarchy to resolve build errors that surfaced during development, including breaking a circular dependency between `defs.h` and `option.h`.

This commit introduces several refinements to the Kalman filter and updates its associated test suite. Key changes include: - **Pre-filtering Logic:** The pre-filtering logic in `kalman.cc` has been adjusted to use the configurable `gap_factor_` instead of a hardcoded value, and the condition for gap detection has been changed from `>` to `>=` to ensure more aggressive filtering of gaps. - **Profile Differentiation:** The `MAX_REASONABLE_SPEED` for the `running` profile in `kalman.cc` has been set to `10.0` to differentiate its behavior from the `cycling` profile, allowing for more distinct testing. - **Test Suite Enhancement:** - `testo.d/kalman.test` has been updated to include a new test case for `kalman_seen_track.gpx` with `gap_factor=10.0`, ensuring comprehensive testing of different `gap_factor` values. - Duplicate test cases in `testo.d/kalman.test` have been removed. - The `kalman_seen_track_out.gpx` and `kalman_out_running.gpx` reference files have been regenerated to reflect the updated filter behavior and `GPSBABEL_FREEZE_TIME=y` consistency. - **Documentation Updates:** - `kalman.h` has been updated to correctly declare `gap_factor_`. - `reference/filter0.txt`, `reference/filter1.txt`, and `reference/help.txt` have been updated to reflect the new Kalman filter options and their `ARGTYPE`s. - New XML documentation files for Kalman filter options have been added (`xmldoc/filters/options/kalman-*.xml`). - `xmldoc/formats/options/igc-VAT.xml` has been created. - `CMakeLists.txt` has been updated to include `src/core/matrix.cc` and `src/core/matrix.h`.

This reverts commit afcdbfb.

This reverts commit 4f44090.

robertlipe · 2025-10-05T17:47:51Z

Oof. Yes. Thers

robertlipe · 2025-10-05T17:48:59Z

Oof. Yes. Theres way more stuff in this or than I meant to expose. I'll integrate that and fix the fallout.

Thanx for a actually reading my scribbling.

tsteven4

This review addresses extraneous changes in this PR, but doesn't get into the intent of the PR. Please address these issues, and merge with master, and I will do an additional review of the core of this PR, the addition of a kalman filter.

Once we get all the extraneous things flagged out I don't think you need to break this up into multiple PRs.

tsteven4 · 2025-10-08T15:39:37Z

 #include "inifile.h"                 // for inifile_t
-#include "option.h"
 #include "session.h"                 // for session_t
+


As the mismatched options are already detectable at test please revert all changes to this file.

tsteven4 · 2025-10-08T15:52:01Z

@@ -0,0 +1 @@
+<para>Write timestamps with offset x to UTC time.</para>


this is outside of scope of the kalman filter.

tsteven4 · 2025-10-08T15:52:02Z

@@ -0,0 +1 @@
+<para>Include Total Energy Variometer in IGC output.</para>


this is outside of scope of the kalman filter.

tsteven4 · 2025-10-08T15:52:05Z

@@ -0,0 +1 @@
+<para>Include True Track in IGC output.</para>


this is outside of scope of the kalman filter.

tsteven4 · 2025-10-08T15:52:07Z

@@ -0,0 +1 @@
+<para>Include True Airspeed in IGC output.</para>


this is outside of scope of the kalman filter.this is outside of scope of the kalman filter.

tsteven4 · 2025-10-08T15:53:58Z

 {
  return allow_trailing_data_;
 }
+


As the mismatched options are already detectable at test please revert all changes to this file.

tsteven4 · 2025-10-08T15:54:44Z

@@ -0,0 +1,140 @@
+This is GPSBabel, known publicly as https://gpsbabel.org and


Do we really want to add this file? If so, it needs work. For example we may or may not use Makefiles, and if we do they need to be generated, so the Quick build instructions are wrong.

tsteven4 · 2025-10-08T15:56:05Z

@@ -0,0 +1,110 @@
+/*


the whole gemini-tmp directory shouldn't be included.

tsteven4 · 2025-10-08T15:56:39Z

the whole gemini-tmp directory shouldn't be included.

tsteven4 · 2025-10-08T15:57:03Z

This file shouldn't be changed, please restore it.

tsteven4 · 2025-10-08T17:08:15Z

needs content

tsteven4 · 2025-10-08T17:08:33Z

needs content

tsteven4 · 2025-10-08T17:08:37Z

needs content

tsteven4 · 2025-10-08T17:08:40Z

needs content

tsteven4 · 2025-10-08T17:08:44Z

needs content

tsteven4 · 2025-10-08T17:08:48Z

needs content

tsteven4 · 2025-10-08T17:08:51Z

needs contentneeds content

tsteven4 · 2025-10-08T17:08:55Z

needs content

tsteven4 · 2025-10-08T17:08:59Z

needs content

tsteven4 · 2025-10-08T18:38:59Z

@@ -0,0 +1,40 @@
+rm -f ${TMPDIR}/kalman_out.gpx
+./gpsbabel -i gpx -f ${REFERENCE}/track/kalman_in.gpx -x kalman -o gpx -F ${TMPDIR}/kalman_in_filtered.gpx -o gpx -F ${TMPDIR}/kalman_out_filtered.gpx


These should all be just gpsbabel, not ./gpsbabel. In test gpsbabel is a function.

tsteven4 · 2025-10-08T18:45:36Z

+//	global_opts.debug_level = debugLevelInfo; // TODO: get from options
+    global_opts.debug_level = std::max(global_opts.debug_level, debugLevelInfo); // TODO: get from options


I think you are ready to just let main set global_options.debug_level, eliminating the body of this ctor.

robertlipe · 2025-10-08T20:24:47Z

It's hopefully clear that I never intended a majority of this PR to BE IN this PR and certainly not to be pushed publicly. Sorry that you've invested so much in it. I should have marked it as a draft, but I knew that only one person ever reads this stuff and thought he wasn't going to see it.

Then I got distracted and you invested in this. Sorry.

Whether I pull 80% of this into other PRs or pull the 20% of this into a single PRthat's actually worth human attention seemed like 6's (six of one, half a dozen...) at the time. I was choosing the latter and starting to break it into smaller pieces that can go in one at a time, though ordering might be tricky.

Things like gbDebug (or whatever we choose to call it) are arguably projects of their own. Maybe each module should have a Logging object that persists that could store its debugging level) inside it so that input could have one level and output could have its own level and we wouldn't keep creating static instances of these empty objects to pretend they're not globals. Then all the member calls become logging_.Warning(foo) and logging_.Debug(6, ...) and such. Maybe it.s logging_.(Warning, foo) vs. logging_.(Debug, 6, foo) or such. I've really not explored that. It's an example of a half-baked idea that just fell out during this process.

I had two big downfalls:

Getting tired of being asked every time "can I run git diff? Can I run git reset" and answering 'y'. Chosing "don't ask me again" also gives it permission to do anything possible with git, like make more branches, push the tree, bring in all the scribbly files that yuou might exist in my ~/src/gpsbabel space, etc.
Trying overlapping projects in the same workspace. This human can (and has for years) easily keeps separate "those files that have needed doc for years" and such and while it's convenient to have overlapping agents working on things, I've now learned that it makes a disastrous mess of things, owing somewhat to # 1, that then becomes Real Work to separate back apart.

I'm finally happy with the results of kalman.cc - I just have to do the work to get it product-worthy. It was probably the most difficult project I've tackled with AI (and clearly one I couldn't have done without out it) but it was still a really painful slog.

I'll be more careful to mark non-trivial things as WIP to be more respectful of your review input.

tsteven4 · 2025-10-10T00:52:14Z

I tired this with some real world cycling data from a garmin edge device that was recording without issues. There wasn't a need to filter this data at all. However, without parameters, the filter introduced some artifacts. The recorded track and points are blue, the filtered track and points are red. I hadn't read your doc yet and didn't select a profile.
./bld/gpsbabel -i gpx -f ht.gpx -o kml -F ht.kml -x kalman -o kml -F htf.kml
Subsequently I tried
./bld/gpsbabel -i gpx -f ht.gpx -o kml -F ht.kml -x kalman,profile=cycling -o kml -F htfcyc.kml
but that showed the same artifacts.
The full data set is 24931 points.

I can get some problematic recordings from a phone that might benefit from filtering, but I haven't got them yet.

tsteven4 · 2025-10-10T01:15:35Z

Perhaps the speeds are relevant, at the start of the artifacts they were 33.2 mph, 32.9 mph, 33.4 mph (14.8 mps, 14.7mps, 14.9mps). Faster speeds occur regularly when descending steeper terrain. The average speed of the entire data set was 15.4mph (6.9mps), so maybe the cycling profile is automatically selected (is auto selection described in document?). However, max_speed_option_.has_value() is true even when not set because of the default provided, so auto select isn't in play. I tried max_speed=30 but that didn't fix it.

GPSBabelDeveloper · 2025-10-10T04:09:30Z

They are indeed relevant. It looks like it's over-aggressively smoothing/filtering. I've looked at this enough that this MIGHT be the same issue three times, It looks like a low Q as you can see in the super cool https://github.com/GPSBabel/gpsbabel/pull/1477/files#diff-66e756d9695282bf21acb4caf360a34cfc38d451f07129ffcf1d74d9491794c1 Counterintuitively, the low q is slow to react and aims for where the moving object used to be. It's the family sedan that's slow to react. A high Q is the blue line that's fast to track, but has high 'inertia' and overshoots on a change (like oversteer) so when there's a change it's disoriented until a new direction is clear, then it quickly snaps back. It'll depend on direction and such, but maybe a higher Q is appropriate for cycling. We're likely to have data-specific issues until we get these profiles dialed in. My test case is a cycling track (from a flat-lander that's on a brightly colored bike that weighs a ton, wink) so I don't think we're TOO far off from making these awesome, but we may need to dial in r_scale_ (measurement noise) and q_scale_pos_ (process noise for position), but we're going to need the direction and time of the GPX segments in question. If you can snip them, that'd be great. If you want to just flip me the file as a test, we can do that. If you want to provide them to me privately, I can anonymize them, capture the important parts, and shove them into the ocean as part of the test suite. We have 42 kalman*out files at this point, each providing one or more specific cases, and I'm sure we'll be adding more. The presence of data-specific issues was one of the clouds that hung over my head for not wanting to do this for so long. It's nightmarish. I'm going to focus now on getting all the pieces landed. I'll get everything into the tree overnight for you to do at least a coarse-level review of them, and then we can do a more thorough one. I'm almost tempted to just commit them and then do a review as they've landed, as this keeps growing. Back to it now. RJL

…

On Thu, Oct 9, 2025 at 8:16 PM tsteven4 ***@***.***> wrote: *tsteven4* left a comment (GPSBabel/gpsbabel#1466) <#1466 (comment)> Perhaps the speeds are relevant, at the start of the artifacts they were 33.2 mph, 32.9 mph, 33.4 mph (14.8 mps, 14.7mps, 14.9mps). Faster speeds occur regularly when descending steeper terrain. The average speed of the entire data set was 15.4mph (6.9mps), so maybe the cycling profile is automatically selected (is auto selection described in document?). However, max_speed_option_.has_value() is true even when not set because of the default provided, so auto select isn't in play. I tried max_speed=30 but that didn't fix it. — Reply to this email directly, view it on GitHub <#1466 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC3VAD7Z5F6WQQT6DSDQCIL3W4CE7AVCNFSM6AAAAACH7HYTMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGOBXHE3DEMBYGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

robertlipe closed this Oct 1, 2025

robertlipe reopened this Oct 1, 2025

robertlipe force-pushed the kalman-2 branch 8 times, most recently from aeed048 to fb863d5 Compare October 1, 2025 08:50

robertlipe force-pushed the kalman-2 branch 12 times, most recently from fd9e565 to f39325f Compare October 1, 2025 09:45

robertlipe force-pushed the kalman-2 branch from f39325f to 6b3661c Compare October 1, 2025 09:46

robertlipe added 5 commits October 1, 2025 07:52

fix(gui): Remove deleted core_strings.h

10cd607

make_gpsbabel_doc - ensure gpsbabel executable exists

0d48e18

doc(xcsv): Add documentation for utc option

afcdbfb

Revert "doc(xcsv): Add documentation for utc option"

4f44090

This reverts commit afcdbfb.

robertlipe added 2 commits October 1, 2025 08:09

Reapply "doc(xcsv): Add documentation for utc option"

271c849

This reverts commit 4f44090.

docs(gemini): Add lessons learned from Gemini interactions

b0f76b6

tsteven4 reviewed Oct 5, 2025

View reviewed changes

Comment thread src/core/nvector.cc

tsteven4 requested changes Oct 8, 2025

View reviewed changes

tsteven4 reviewed Oct 8, 2025

View reviewed changes

Comment thread kalman.cc

		@@ -0,0 +1 @@
		<para>Write timestamps with offset x to UTC time.</para>

		@@ -0,0 +1 @@
		<para>Include Total Energy Variometer in IGC output.</para>

		@@ -0,0 +1 @@
		<para>Include True Airspeed in IGC output.</para>

		@@ -0,0 +1,140 @@
		This is GPSBabel, known publicly as https://gpsbabel.org and

		@@ -0,0 +1,40 @@
		rm -f ${TMPDIR}/kalman_out.gpx
		./gpsbabel -i gpx -f ${REFERENCE}/track/kalman_in.gpx -x kalman -o gpx -F ${TMPDIR}/kalman_in_filtered.gpx -o gpx -F ${TMPDIR}/kalman_out_filtered.gpx

		// global_opts.debug_level = debugLevelInfo; // TODO: get from options
		global_opts.debug_level = std::max(global_opts.debug_level, debugLevelInfo); // TODO: get from options

Conversation

robertlipe commented Oct 1, 2025

Uh oh!

codacy-production Bot commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage summary from Codacy

Uh oh!

Uh oh!

robertlipe commented Oct 5, 2025

Uh oh!

robertlipe commented Oct 5, 2025

Uh oh!

tsteven4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertlipe commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 10, 2025

Uh oh!

tsteven4 commented Oct 10, 2025

Uh oh!

GPSBabelDeveloper commented Oct 10, 2025 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codacy-production Bot commented Oct 1, 2025 •

edited

Loading