BUG: Fix the missing x y forces from TVC - #28
Merged
Merged
Conversation
Member
Author
|
@thc1006, thanks for raising the issue of this bug. Could you please help review this PR. Feel free to add pytests on this branch directly |
Member
學長 Seem like still have CI jobs failed maybe can fix them first. and 同時 I am reveiwing the other part. |
Member
Author
Yes, my bad, another commit pushed |
zuorenchen
force-pushed
the
bug/fix-TVC
branch
from
August 25, 2026 00:07
3d28249 to
461ce43
Compare
Merged
zuorenchen
added a commit
that referenced
this pull request
Aug 27, 2026
* Coverage that cannot report a green it did not earn Two parts of the coverage path report success while doing nothing. Both are in the logs of any recent run. The six matrix legs all uploaded an artifact named `coverage` with `overwrite: true`, which deletes the previous artifact rather than merging into it. Run 30461091931 shows `Artifact 'coverage' (ID: 8727950028) deleted` and then finalized again by a later leg, so five of the six reports were thrown away and the surviving one was whichever leg happened to finish last. Each leg now writes and uploads a report named after itself, and the upload job downloads all of them. The headline number was not far off, since all six legs land on 80%, but which platform's report reached Codecov was decided by a race, and code that only runs on one platform was counted or not depending on it. `files:` was a YAML block literal, so the name arrived as "coverage.xml\n" and was not found. The upload only worked because the uploader falls back to searching. That fallback is now unnecessary: the reports are in one directory, and the directory is what is passed. `fail_ci_if_error` is tied to whether a token was there to use. A pull request from a fork gets no secrets, and a contributor should not see red for that. What this does not fix: `CODECOV_TOKEN` resolves to empty on this repository, so the upload is still rejected with "Token required - not valid tokenless upload", and with no token the run stays green. Setting that secret is the last step and it needs repository admin. Once it is set, this workflow fails when an upload fails, without another change. Upstream RocketPy carries both of these unchanged. Happy to send the same patch there so it comes back through the usual sync. Verified: actionlint clean, `--cov-report=xml:<name>` writes the named file, and `pattern` / `merge-multiple` / `directory` / `fail_ci_if_error` all exist as inputs on the pinned actions. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * Docs that can build The docs job has failed on every run since it was added: five runs, back to 2026-07-26, all red. `sphinx-build -W` turns four warnings into errors, and all four come from this fork's own additions rather than from upstream. Three are one stray line, copied three times. `add_thrust_vector_control`, `add_roll_control` and `add_throttle_control` describe the controller arguments as a numbered list, and item 7 ends with a loose ``interactive_objects`` at one space less than the continuation lines above it. docutils reads that as a definition list ending without a blank line. The same item in upstream's `add_air_brakes` does not have it, so removing it puts these three back in step with the method they were copied from. Measured per docstring, running napoleon and docutils the way Sphinx does. Before: those three warn at line 28, `add_air_brakes` does not. After: none of the four warn. The fourth is `halcyon_flight_sim_active_control.ipynb`, which is in `docs/examples/` and in no toctree, so Sphinx builds it and then reports that nothing links to it. It is this fork's own example of the feature the fork exists for, so it goes in the list next to the flight it varies rather than into an exclude. Not verified by a full local build: an unrelated notebook fetches a live weather forecast and the file it reaches no longer covers the date it asks for, which stops the build here for a reason CI does not have. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * Run the docs check before the release merge, not at it `docs.yml` triggered on a base of master only, so the only pull request that ever started it was develop into master. A docstring that does not build therefore lands on develop unremarked and first fails the merge that was meant to ship it, which is what #19 and #23 hit and why the four warnings in the commit before this one went unnoticed for three days. develop as well. The same paths filter, so it still only runs when something it reads has changed. This also makes the commit before it self checking: without this, a pull request into develop cannot start the job that would prove the warnings are gone, and the only evidence would be a measurement in the description. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: make every step_simulation call advance the flight A call that landed on a phase boundary advanced the phase index, left the new phase uninitialised and returned, so it moved neither t nor y_sol. A caller counting one step per call, which is what the Balloon Popping Challenge environment does, then ran ahead of the flight's own clock. The phase transition now carries on into the new phase in the same call, so the only path that returns without advancing is the one that has just set finished, and by then there is nothing left to advance. Measured on flight_calisto: 5 calls with 2 that did not advance, against 3 calls with none. Final t is 48.4363 either way, so the same flight is being walked in fewer calls rather than a different one. Ignoring whitespace the change is a while loop and a continue; the rest of the diff is the indentation that loop adds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: Fix the missing x y forces from TVC (#28) * Fix the missing x y forces from TVC * Fix the cases where thrust1/2 are not calculated * BUG: Fix gravity term in accelerometer (#29) * Fix gravity term in accelerometer * Fix accelerometer gravity model in tests --------- Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> Co-authored-by: thc1006 <84045975+thc1006@users.noreply.github.com>
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
2 participants
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.
thrust1andthrust2forcesthrust_eccentricity_{x/y}andthrust{1/2}