RocketPy-Team / RocketPy

Next generation High-Power Rocketry 6-DOF Trajectory Simulation
https://docs.rocketpy.org/
MIT License
570 stars 137 forks source link

ENH: Sensors #583

Closed MateusStano closed 1 month ago

MateusStano commented 3 months ago

Pull request type

Checklist

Description

This PR tackles issue #273 and builds on top of #580

Added:

Sensors Architecture

Notes

Breaking change

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 90.80460% with 48 lines in your changes are missing coverage. Please review.

Please upload report for BASE (enh/sensors-impl@fbeff2c). Learn more about missing BASE report.

Files Patch % Lines
rocketpy/plots/rocket_plots.py 30.43% 16 Missing :warning:
rocketpy/sensors/sensors.py 87.35% 11 Missing :warning:
rocketpy/simulation/flight.py 91.79% 11 Missing :warning:
rocketpy/control/controller.py 75.00% 3 Missing :warning:
rocketpy/mathutils/vector_matrix.py 90.90% 2 Missing :warning:
rocketpy/prints/sensors_prints.py 98.14% 1 Missing :warning:
rocketpy/rocket/components.py 85.71% 1 Missing :warning:
rocketpy/rocket/rocket.py 90.00% 1 Missing :warning:
rocketpy/sensors/accelerometer.py 98.48% 1 Missing :warning:
rocketpy/sensors/gyroscope.py 98.55% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## enh/sensors-impl #583 +/- ## =================================================== Coverage ? 73.78% =================================================== Files ? 64 Lines ? 10049 Branches ? 0 =================================================== Hits ? 7415 Misses ? 2634 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

phmbressan commented 2 months ago

I forgot to comment in the review:

MateusStano commented 2 months ago

Hey! So I have changed the way the sensors readings. The initial implementation had the measurements saved in the sensor object being used, this caused the following problems:

  1. If the same sensor instance was added twice to a rocket the measured_data would have intercalated data
  2. Running several Flight simulations with a rocket with the same sensors instances would just keep appending data to measured_data

However, the measured_data attribute was an useful way to get previous sensor readings inside the controller function. My solution was to save the data both in the Flight class and in the Sensors. In the Flight the sensors reading are saved as a dict, while in the Sensors its saved as a list. This grants versatility to the code.

To do this I had to:

So the final structure is of the saved attributes is as follows:

Given an Accelerometer that is added once to the rocket. We have that its:

If there is an Accelerometer that is added twice to the rocket. We have that its:

However, we could also have the Flight.sensors_data be {Accelerometer : [[(t1, ax1, ay1, az1), (t2, ax2, ay2, az3), ...], [(t1, ax1, ay1, az1), (t2, ax2, ay2, az3), ...]] } in this last case. Any opinions on this?

@Gui-FernandesBR @phmbressan This is ready for a new review

Gui-FernandesBR commented 1 month ago

This is awesome!

image

Gui-FernandesBR commented 1 month ago

@MateusStano , tomorrow I will invest some time to test the jupyter notebook and search for any bug. In the mean time, there are still a few conversations opened, would you mind addressing them?

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.

MateusStano commented 1 month ago

Seems like we have an inverted direction somewhere: image

This result is supposed to be right because the noisy gyroscope is upside down. Please check the orientation and see if it makes any sense

MateusStano commented 1 month ago

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