Conversation
Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com> Co-authored-by: Giovani Hidalgo Ceotto <ghceotto@gmail.com>
Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com> Co-authored-by: Giovani Hidalgo Ceotto <ghceotto@gmail.com>
Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com> Co-authored-by: Giovani Hidalgo Ceotto <ghceotto@gmail.com>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Gui-FernandesBR
added
the
Enhancement
There was a problem hiding this comment.
This PR looks great. Simple yet powerful implementation of controllers. I was amazed by how such a simple controller could perfectly make the rocket reach the desired apogee.
@phmbressan @brunosorban and I helped in this PR at its infancy, but the example created by @MateusStano is fantastic. Hats off! 🚀
I do have one issue regarding the airbrakes.state_history attribute. I am not sure storing the state history in the airbrake instance is perfectly appropriate. I see a couple of problems:
airbrakes.state_historyis empty before a Flight instance is created.airbrakes.state_historymay contain the history of multiple flights if various Flight instances are created using it, unless it is manually reset.- Plots related to
airbrakes.state_historyare totally empty before a flight.
That is why I wonder if this should be stored in the Flight class somehow, or if it is ok to have the airbrake instance mutate during flight. I would like a third opinion on this. @phmbressan @Gui-FernandesBR, any quick thoughts?
And a quick note regarding sources. I believe that the airbrake drag curve comes from a European Team. If confirmed, we should ask their permission to use the data and cite them.
*If fast approval is needed, we can go ahead and merge this PR and I'll create an issue to handle what is left afterwards. I hereby state that this PR is fully functional as far as I was able to review and test it. What is still necessary is:
- Tests
- Detailed docs
-
airbrakes.state_historydiscussion - Proper citation of data sources
|
Getting back to this one:
Final comment here is that we are definitely going to be missing the #273 Sensors implementation. I believe improving RocketPy's control capabilities should be our priority from here on out |
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Nice changes since the previous reviews. I think we are quite close to conclude this one. I've made some minor comments that I think should contribute to the PR.
Tests could be better (i.e. faster) but I understand it doesn't need to be perfect right now.
The PR already allows users to simulate they airbrakes in RocketPy, which is remarkable!
phmbressan
left a comment
phmbressan
left a comment
There was a problem hiding this comment.
Amazing pull request, these are truly some of the most interesting graphs I have seen in RocketPy.
Great to see more than 100 comments on this PR request with inputs and suggestion.
Superb work @MateusStano in putting this all together.
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
LGTM, @MateusStano whenever you feel confortable, please merge it.
giovaniceotto
left a comment
giovaniceotto
left a comment
There was a problem hiding this comment.
A masterpiece.
It is hard to make it 100% future proof, but this is definitely the best that could have been done for now. I am eager to see how control will evolve in RocketPy. Couldn't ask for a better start.
5 participants
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest --runslow) have passed locallyNew behavior
A simple implementation of an airbrakes simulation has been added. The added code:
Class Airbrakes: Inherits fromAeroSurface. Defines an airbrakes system with a cd curve that is a function of the deployed level and mach number.Class Controller: Defines a controller that has acontroller_function. This function is defined by the user and can alter the state of any number ofobserved_objects. The controller will call thecontroller_functioneverysampling_rate(in simulation time).add_controllersmethod inRocket: Similar toadd_surfacesadd_airbrakesmethod toRocket: Similar to the aero surfacesaddmethods. Simplifies the addition of the air brakes for the userBreaking change