Skip to content

feat(igpsport): sync and verify selected cycling rider settings - #63

Open
KirillSugonyakin wants to merge 6 commits into
jorge-huxley:masterfrom
KirillSugonyakin:codex/upstream-rider-settings
Open

KirillSugonyakin wants to merge 6 commits into
jorge-huxley:masterfrom
KirillSugonyakin:codex/upstream-rider-settings

Conversation

@KirillSugonyakin

@KirillSugonyakin KirillSugonyakin commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Add sync-rider-settings to transfer selected cycling profile values from Intervals.icu to iGPSPORT, with dry-run and explicit read-back verification. The revised command reuses the existing sync-zones conversion and HR-mode behavior rather than introducing a second zone policy.

Maintainer-requested revision

  • Reuse existing map_power_zones / map_hr_zones, threshold normalization, ties-to-even rounding, fixed power cap, and maximum-HR mode/table for selected HR-zone synchronization. zone_map.py is unchanged from upstream.
  • Remove strict named-model adapters, alternate cap/coarsening/rounding policies, implicit LTHR-mode selection, and model/adapter status metadata.
  • Configured numeric boundaries work without recognized labels. Preserve fractional source thresholds until legacy rounding is applied by the consumer; the source parser does not pre-round them half-up.
  • Distinguish seven power slots from five HR slots. Five-slot power examples in the research were synthetic tests, not an observed live API table or evidence of a defect in the existing seven-slot conversion.

Retained workflow

  • Ride only; select FTP, power zones, maximum/threshold/resting HR, HR zones, weight, height, birth date, and sex. Zone selections expand threshold dependencies.
  • Preserve missing source values, unselected destination fields, nickname/location, and inactive HR tables. Weight-only or FTP-only selection does not switch HR mode. Malformed input blocks its endpoint group.
  • Apply each changed endpoint group at most once and read it back. An unchanged follow-up run performs no write.
  • --fields, --dry-run, --show-values, and status JSON. Values require explicit --dry-run --show-values; normal/error output does not expose private values or raw API exceptions.
  • Existing sync-zones and GUI parsing/conversion remain unchanged. No estimation, analysis, scoring, scheduling, or new runtime dependency.

Verification

  • uv run --frozen pytest -q: 335 passed, Python 3.13.
  • Combined with revised feat(igpsport): refresh and verify native workouts with secondary targets #62 on upstream 6dfab98: 411 passed, conflict-free merge.
  • Same-input comparisons against legacy apply_intervals_settings, with independent literal expectations: seven power/five HR slots, custom labels/boundaries, fractional thresholds, half-watt rounding, differing fetched caps, inactive-table preservation, and non-HR selection.
  • Existing tests retain missing/invalid handling, dependencies, personal-field preservation, grouped writes, dry-run, read-back failures, repeat no-write runs, and private exception suppression.
  • Bash syntax, diff checks, and CLI help smoke checks passed. Secret scan with network verification disabled found no new candidate lines; all candidates match existing upstream lines.
  • Independent review completed; both sync-error paths now have regression checks for integer counters, failed: 1, and private-error suppression.

Research and evidence limits

Sanitized research and validation summary documents the original downstream strict policy and its limitations, and now explicitly distinguishes it from this revised upstream implementation.

Historical downstream API writes/read-back established persistence for the configuration exercised, including its strict policy. They do not prove that policy is superior or validate HR-mode behavior across apps/devices. The secondary-cadence device test in #62 is a different feature. This revision used offline synthetic fixtures and made no live account writes or packaged GUI builds.

The command needs a release before it appears in intervalssync@latest. The separate #62 contribution shares the small timeout helper so each PR can be merged independently. No release tags or deployment changes are included.

@jorge-huxley

Copy link
Copy Markdown
Owner

Thanks for this contribution! I like the additional profile fields—height, birth date, and sex—and the ability to select what gets synchronized with --fields. Dry-run support and explicit read-back verification also look useful.

Before merging, I’d like to better understand the changes to HR and power-zone synchronization.

The existing sync-zones conversion was designed around iGPSPORT’s limitations, including its fixed zone slots and boundary constraints. Its purpose is to represent the Intervals.icu zones as closely as the destination allows. Those adaptations are intentional, so I’m cautious about introducing a separate implementation that rejects configurations the existing command supports.

Could you clarify:

  1. What specific problems in the existing zone conversion does the new implementation solve? Concrete examples of source zones and the resulting iGPSPORT zones would help.
  2. How do the two commands differ for the same input? Could you show comparisons for:
    • Seven Intervals.icu power zones mapped to five iGPSPORT slots.
    • Seven Intervals.icu HR zones mapped to five iGPSPORT slots.
    • Custom zone names and boundaries.
  3. Why restrict synchronization to recognized named zone models? Are the zone names necessary for correct conversion, or could we use the configured boundaries regardless of their labels?
  4. Why change the HR calculation-mode behavior? What effect does selecting LTHR mode have on the zones shown and used by iGPSPORT and its devices?
  5. Has the new zone behavior been validated against the actual iGPSPORT app/device, or only synthetic API responses?

My preference would be to reuse the existing sync-zones conversion logic for both commands, while adding field selection, the extra profile fields, dry-run, and verification around it—unless there’s a demonstrated correctness issue with the existing conversion.

Would that approach work with your implementation? I’d like to avoid two commands producing different zone configurations from the same Intervals.icu settings without a clear reason.

@KirillSugonyakin

Copy link
Copy Markdown
Contributor Author

Reusing the existing conversion logic would work, and I agree it is the better default for this contribution. Field selection, additional profile fields, dry-run, and read-back verification do not require a separate zone-conversion policy.

I have attached a sanitized research and validation summary, including the original rationale, primary sources, numerical comparisons, and the limits of the live evidence.

To answer your questions:

  1. Problem being addressed. The strict adapters came from a narrower downstream requirement: preserve particular source-zone boundaries, retain the fetched destination power cap, and reject configurations whose intended grouping was uncertain. This is not evidence that the existing general conversion is wrong. It introduces different coarsening, rounding, cap, and recognition policies, which should be reviewed separately from the new command.

  2. Same-input comparisons. These examples are synthetic; arrays show destination upper boundaries:

    Input Existing conversion Proposed strict adapter
    Power: FTP 250, percentages [55,75,90,105,120,150,999], five destination slots, cap 2500 [138,188,225,375,2500] [138,188,225,263,2500]
    HR: [120,145,165,175,185,190,195], max HR 195, LTHR 180, five slots and recognized labels [120,145,165,175,195] [120,145,165,175,195]
    Same numerical inputs, renamed to Custom 1, Custom 2, etc. Still converts using the configured boundaries Rejects the unknown names and blocks the interval-settings group

    In the power example, the two implementations retain different upper boundaries when combining zones. Half-watt rounding is another independent difference: 262.5 becomes 262 with the existing ties-to-even rule and 263 with half-up rounding. The HR example has no numerical boundary improvement.

  3. Why named models. Names were a conservative guardrail before applying a specific grouping policy, not a requirement for copying explicit numerical boundaries. Custom or translated labels make that unnecessarily restrictive for general synchronization. The source material explains the model vocabulary but does not require a relay to reject renamed zones.

  4. HR mode. Selecting LTHR mode was intended to activate iGPSPORT's LTHR table for the recognized source scheme. Legacy sync instead selects maximum-HR mode and its table. That changes the active table/basis even when the boundaries are numerically identical. I have not demonstrated that this change is necessary for the new command or validated its effects across devices, so I agree it should not be introduced implicitly here.

  5. Live validation. The fork's rider-settings synchronization was exercised with real API writes and read-back verification, including zone boundaries, LTHR mode, the destination cap, and preservation of unselected fields. The recorded acceptance also includes an unchanged follow-up run with no writes. This confirms API persistence for the configuration exercised; it is not comprehensive app/device validation or proof that the alternative conversion policy is generally superior. The secondary-cadence device test discussed in feat(igpsport): refresh and verify native workouts with secondary targets #62 is a different feature and should not be counted as evidence for HR-mode behavior.

I propose revising this PR to reuse the existing zone conversion and HR-mode behavior, with field selection, the extra profile fields, dry-run, and verification around it. Any changes to rounding, terminal-cap handling, or model-specific conversion can be discussed separately with focused examples, compatibility tests, and the relevant app/device evidence. The research note documents the rationale; it has not changed the current implementation yet.

@jorge-huxley

Copy link
Copy Markdown
Owner

Thanks for the clarification. One point about the numerical comparison: iGPSPORT uses seven power zones, and the existing implementation already preserves the source’s six finite boundaries in that configuration, replacing the open-ended sentinel with the destination cap. Why is the power comparison based on five destination slots? Was a five-slot power table actually observed in a live API response, or is this only a synthetic scenario? Please distinguish the seven-zone power behavior from the five-zone HR behavior.

To be clear about scope: I’m happy with adding height, sex, and birth date, and with field selection. However, I’m concerned that introducing different zone conversion, rounding, terminal-cap handling, model-name restrictions, or HR calculation-mode selection would regress the existing functionality. I don’t want those behavior changes included in this PR.

As you proposed, please reuse the existing zone conversion and HR-mode behavior rather than introducing a separate policy. Please also include regression tests confirming that, for the same source and destination configuration, the new command produces the same power/HR zone boundaries and HR mode as the existing sync. Any proposed changes to those behaviors should be discussed separately with evidence of an actual problem in the current implementation.

@KirillSugonyakin

Copy link
Copy Markdown
Contributor Author

The five-slot power example was synthetic; the observed configuration had seven power slots and five HR slots. The revised command now reuses existing conversion, rounding, cap, and HR-mode behavior, with same-input regression tests. Research and description updated; all 335 tests pass in CI. Ready for another review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants