-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## enh/sensors-impl #583 +/- ##
===================================================
Coverage ? 73.78%
===================================================
Files ? 64
Lines ? 10049
Branches ? 0
===================================================
Hits ? 7415
Misses ? 2634
Partials ? 0 ☔ View full report in Codecov by Sentry. |
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.
This is a great start for RocketPy Sensors
feature. Well done @MateusStano and thank you for providing references in the PR description.
Before approval, I have made some comments, but overall I liked the implementation a lot.
plane : str, optional | ||
Plane in which the rocket will be drawn. Default is 'xz'. Other | ||
options is 'yz'. Used only for sensors representation. |
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.
I forgot to comment in the review:
|
Co-authored-by: Pedro Henrique Marinho Bressan <[email protected]>
@MateusStano , tomorrow I will invest some time to test the jupyter notebook and search for any bug. This is a already too large PR (70 commits), feel free to write down some TODOs in the code if you think something is not crucial, just let us know please. Btw the head branch seems to be outdated with respect to the develop branch. |
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.
Good work addressing most of the review comments. I know sometimes it can be hard, but please go through all the open conversations. You can let some TODO in the code if needed.
Let me know when you are done.
I have used the sensors notebook and found some inconsistent information. Can you take a look?
Seems like we have an inverted direction somewhere:
Co-authored-by: Gui-FernandesBR <[email protected]>
… into enh/sensors
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.
Approving this because:
- I know you made a good job
- This is not going to develop yet
- We can make some small adjustments before merging to develop later.
I think 1 or 2 comments/suggestions were left behind, but hopefully we will double click on them in future PRs, no problem.
Also I did not care so much about the new jupyter notebook, I think it is there as a fast testing option only, right?
Yes, its just for testing right now I'll go ahead and merge this then |
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Description
This PR tackles issue #273 and builds on top of #580
Added:
Sensors
abstract classAccelerometer
classGyroscope
classSensors
are included in theFlight
simulation loop. Having any amount of sensors in the simulated rocket will add only one additional call tou_dot
per simulation step. Sensors only worktime_overshoot=False
add_sensor
method in rocket class that receives the sensor and its position and saves them in aComponent
object__and__
operator for elementwise multiplication betweenVector
objectseuler_to_quaternions
function intools.py
for conversion from euler angles to quaternionstransformation_euler_angles
static method in theMatrix
class for construction of a rotation matrix from euler anglesComponents
class for speed purposestriggerfunc
can now receive a list of sensors as its last argumentcontroller_function
can now receive a list of sensors as its last argumentSensors Architecture
Sensors
have theirorientation
and noise information saved in itself. When added to the rocket, a position must be givenmeasure
method that characterizes the sensor. Themeasure
function receives(t, state, state_derivative, *args)
.*args
refers to the specific input required by the sensor. Different types of sensors will receive different*args
.measure
method, the sensor's reading is saved in themeasured_data
attribute. This means that the state of the sensor changes throughout the simulation.Notes
sensors_testing.ipynb
for testing the current sensor implementation. It does not need to be reviewedtest_ideal_sensors
. The tolerance in this test is limited by numerical errors in the calculation of the rotation matrix from the quaternions. This is probably causing inaccuracies in the simulation. We should fix this soonz
axis of theCalisto
flight. Seems to be related to errors in the derivation of the motor'smass_dot
. This does not happen when the old udot is used. We should fix this soonBreaking change