Move winch controllers out - #52
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the migration of V3Kite’s winch controllers/settings out of V3Kite and into WinchControllers.jl, making V3Kite a torque-only plant API. Tests and example scripts are updated to own the winch length loop externally via a new adapter.
Changes:
- Removed in-package winch controller structs/settings (
WC_Settings,WinchPosController,WinchForceController) and updatedstep!/warmup!to be torque-only. - Added
examples/winch_adapter.jlto bridge V3Kite plant scalars (drum_params,winch_acc_limit, etc.) to WinchControllers’ scalar APIs. - Updated tests/examples/data/docs and added WinchControllers dependency to the example/test environments.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-turbulence-injection.jl | Switch tests from set_length to external torque loop via adapter. |
| test/test-interface.jl | Updates interface expectations and adds adapter-based winch loop tests (including feed-forward). |
| test/test_parking_ripple.jl | Updates simulation loop to command winch torque via adapter. |
| test/Project.toml | Adds WinchControllers as a test dependency. |
| src/wc_settings.jl | Removes now-externalized winch settings struct/loader. |
| src/V3Kite.jl | Stops including/exporting winch controllers/settings; exports new plant helpers. |
| src/sim_helpers.jl | Minor doc formatting change. |
| src/ripple_metrics.jl | Removes stale mention of WC_Settings convention. |
| src/precompile.jl | Updates workload to no longer use set_length (but now diverges from stated “mirrors” comment). |
| src/interface.jl | Refactors to torque-only winch input; adds drum_params/winch_acc_limit; updates warmup! API. |
| Manifest-v1.12.toml.default | Bumps Julia and dependency versions; adds WinchControllers/WinchModels. |
| examples/winch_adapter.jl | New adapter providing load_wc_settings, winch_torque!, and force-mode helper. |
| examples/steering_test_v3.jl | Uses adapter-driven torque loop instead of set_length. |
| examples/simple_sinus.jl | Uses adapter-driven torque loop instead of set_length. |
| examples/simple_parking.jl | Uses adapter-driven torque loop instead of set_length. |
| examples/simple_auto_parking.jl | Uses adapter-driven torque loop instead of set_length. |
| examples/rest_server.jl | Keeps REST API set_length but converts it to torque via adapter; stores controller in session. |
| examples/Project.toml | Adds WinchControllers as an examples dependency. |
| data/wc_settings.yaml | Updates header comment to reference WCSettings (needs follow-up for other stale mentions). |
| CHANGELOG.md | Adds unreleased notes (currently inconsistent with the new torque-only step! API). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ufechner7
marked this pull request as ready for review
August 16, 2026 15:18
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The controllers moved to WinchControllers.jl, but three pieces came back downstream instead of being used from there. - Drop V3Kite's winch_acc_limit: WinchControllers exports the same function, so both packages exporting it made the name ambiguous for any script that loaded both. The adapter calls winch_acc_limit(s.set.max_acc). - Drop load_wc_settings for WinchControllers' WCSettings(true; dt), which already resolves the file through the wc_settings: field of the system YAML rather than hardcoding the filename at every call site. Unknown keys are no longer an error; that check belongs upstream if it is wanted. - Drop winch_force_hold!, which had no callers. - Collapse the six repeats of the controller construction into winch_pos_controller(s). Also trims the docstrings and the changelog to the API delta, and puts the BREAKING prefixes on the user-facing changes rather than on the move itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/winch_hold_stub.jl:44
length_hold_controller(s::V3KITE)claims to be “fors” / “one per model”, but it currently ignoressand always creates the same default state (v_sp_prev = 0.0). Initializingv_sp_prevfrom the model’s current reel-out speed makes the rate limiter consistent from the first step and aligns better with the docstring.
"""
length_hold_controller(s::V3KITE) -> LengthHoldController
The length-holding torque controller for `s`. One per model.
"""
length_hold_controller(s::V3KITE) = LengthHoldController()
Manifest-v1.12.toml.default:10
- The PR description mentions bumping WinchControllers, but this manifest update also bumps
julia_version(1.12.6 → 1.12.7) and many unrelated dependencies (e.g. ADTypes 1.22.4 → 1.23.0). If the intent was only the WinchControllers bump, consider regenerating the manifest with a narrower update to reduce churn/merge conflicts; otherwise, documenting the broader environment bump in the PR description would help reviewers/users.
# This file is machine-generated - editing it directly is not advised
julia_version = "1.12.7"
manifest_format = "2.0"
project_hash = "116eb6e9b6fc09104a63693604b0ee347c201417"
[[deps.ADTypes]]
git-tree-sha1 = "5970c86505ae9c07bf5bc521ef2bbbb3849e8b7b"
uuid = "47edcb42-4c32-4615-8424-f2b9edc5f35b"
version = "1.23.0"
1-Bart-1
approved these changes
Aug 17, 2026
Member
There was a problem hiding this comment.
Shared code should not be in a separate example
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
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.
WinchControllers is still a dependency for examples and tests, but not for the package itself. This avoids the long precompilation times of this package if changes to WinchControllers are applied and makes it easier to test and improve the winch controllers separately.