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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Sensors #583
ENH: Sensors #583
Changes from 70 commits
e403872
6cd3598
3b81777
4c8aa13
09e0d75
38866f4
b386e38
6ff2dde
5ae01b9
1dd5781
943542a
28ebc46
378bb54
d8440f2
99f445f
4db26f0
4356af7
5c37f06
8d64998
28da8f3
cef72a0
5f8f1f4
9a6b052
7953cb0
5a37553
09ea252
9851392
0f81bc3
a740fc2
cf6c26d
2a14f1d
216523c
09288d4
aa6fcdf
bf6b083
5f86223
4dcc26b
32898c5
f7332d8
2131ee9
b2da0c3
123d033
7d0e6f3
00f0f3a
de2d8bd
fec6725
ce2a63d
f2656c5
41bf9e9
11873cd
b795031
968f55a
d827b8e
0b0f201
bad3b07
0891c6c
fa1b6a8
b7439f4
8c71432
5885992
02cad05
69f17cd
6894f58
8d96e58
ec4e25e
adb2dbb
e0812a2
6ec4c0b
78e67f2
d49b5c4
57b41ab
7a2e50a
a57308d
11a8ab6
2984065
82f79e7
5016283
6806912
b99b301
75f8d9e
c1864b2
a20d31d
ce1d179
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Check warning on line 123 in rocketpy/control/controller.py
Codecov / codecov/patch
rocketpy/control/controller.py#L122-L123
Check warning on line 125 in rocketpy/control/controller.py
Codecov / codecov/patch
rocketpy/control/controller.py#L125
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, as far as I remember, the controller function is still private.
Can't we simply change the expected behavior of controller function to always receive 7 arguments?
Of course current applications would breake, but the class was still private...
I'm just not 100% sure about the air brakes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would change it for the airbrakes too, so its technically a breaking change. I do think having the sensors argument be optional can be good since not all rockets will have sensors.
One thing I just thought of, maybe we should have all this arguments be passed as key word arguments in the user's
controller_function
. The user's controller functions would then have to receive**kwargs
and just unpack it as needed:This would allow us to never need to care about how many params we are passing and in what order.
What do you think of this? It would be a breaking change, but only for the air brakes usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your proposal is really good.
Why would it be a breaking change? Couldn't you individually handle the case where the key "sensors" is not present in the kwargs dict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a breaking change because any controller_function defined by a user would need to define its parameters differently
Check warning on line 160 in rocketpy/mathutils/vector_matrix.py
Codecov / codecov/patch
rocketpy/mathutils/vector_matrix.py#L159-L160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would detail a bit more where these axes are defined (e.g. the x axis is the one defined as reference in the sensor definition). Perhaps even link the docs page.
A future enhancement would be the correct angular position of the rail buttons, even though we don't have that information right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MateusStano
Check warning on line 566 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L564-L566
Check warning on line 569 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L568-L569
Check warning on line 573 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L571-L573
Check warning on line 576 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L575-L576
Check warning on line 579 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L578-L579
Check warning on line 581 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L581
Check warning on line 584 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L584
Check warning on line 586 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L586
Check warning on line 594 in rocketpy/plots/rocket_plots.py
Codecov / codecov/patch
rocketpy/plots/rocket_plots.py#L594