Conversation
There was a problem hiding this comment.
Thanks for splitting the PRs, this is really helpful.
There are two issues with this PR: First, it is not clear to me why we place some parameters into the first principles model files. Even our docstrings are inconsistent in this regard.
This inconsistency later appears in the second point, the API around load_params. I suspect the current design is forced by the requirement to get some physical parameters that are not in the function signature of the dynamics function. But getting different results for load_params(first_principles_dynamics) and load_params(Dynamics.first_principles) screams "something is off here".
We should come up with a better, more consistent solution.
| `crazyflow.drones.load_params`. | ||
| - Two `load_params` exist, in `dynamics.core` and `control.core`. Given a function, both filter to | ||
| its signature and silently drop the rest. The dynamics one also takes a `Dynamics` mode and then | ||
| returns every parameter of the drone, which is how to get hardware constants like `pwm_max`. |
There was a problem hiding this comment.
| returns every parameter of the drone, which is how to get hardware constants like `pwm_max`. | |
| returns every parameter of the drone. |
I know we had this in before, but it should not have been there in the first place.
| params = load_drone_params(drone) | ||
| thrust_min, thrust_max = params["thrust_min"], params["thrust_max"] | ||
| params = load_dynamics_params(dynamics, drone) | ||
| thrust_min, thrust_max = float(params["thrust_min"]), float(params["thrust_max"]) |
There was a problem hiding this comment.
Do we need the float conversion? That seems odd.
| rpm2thrust = drone_params["rpm2thrust"] | ||
| hover_thrust_value = np.asarray(drone_params["mass"] * 9.81, dtype=np.float32) | ||
| hover_rotor_vel = motor_force2rotor_vel( | ||
| np.full(4, hover_thrust_value / 4.0, dtype=np.float32), drone_params["rpm2thrust"] | ||
| np.full(4, hover_thrust_value / 4.0, dtype=np.float32), rpm2thrust |
| name = fn.__module__.split(".")[-2] if fn is not None else str(dynamics) | ||
| if name not in tuple(Dynamics): |
There was a problem hiding this comment.
| name = fn.__module__.split(".")[-2] if fn is not None else str(dynamics) | |
| if name not in tuple(Dynamics): | |
| name = fn.__module__.split(".")[-2] if fn is not None else dynamics | |
| if name not in Dynamics: |
| if fn is not None: | ||
| params = filter_to_signature(params, fn) | ||
| return to_xp(params, xp=xp, device=device) |
There was a problem hiding this comment.
This is not at all intuitive. Why do we not filter when we are given the enum over its function, which are supposed to be 1:1 correspondences? I suspect this is because you need some convenient way to get parameters like the pwms, but I am absolutely against introducing this kind of shortcut for it.
| dynamics: A dynamics function, or a dynamics mode. For a function, the result only contains | ||
| the parameters in its signature. For a mode, all parameters of both files are returned. |
| # Parameters of the first principles dynamics, and platform data that no dynamics uses but other tools | ||
| # (sim, estimators, firmware, examples) may need. The core physical parameters shared by all dynamics |
There was a problem hiding this comment.
If no dynamics use the params, they should not be in first principles, should they?
| # | ||
| # [my_drone] | ||
| # gravity_vec = [0.0, 0.0, -9.81] # m/s^2 | ||
| # mass = 0.1 # kg |
There was a problem hiding this comment.
I don't see how mass, something that we can measure, is distinct from, e.g., L, which is also something we can easily measure. The main argument is that mass is used across models, whereas L is only used in one. In turn, that means the location of a parameter is determined by the sum of signatures of our dynamics models, which doesn't make much sense. It should be immediately clear by looking at a parameter where it belongs, and the rule "mass and J are in drones, L is not, but thrust_min is" seems arbitrary.
| are independent of the dynamics formulation used to simulate it (see [crazyflow.dynamics][]). | ||
| files, their referenced meshes (``assets/``), and the core physical parameters shared across all | ||
| dynamics (``params.toml`` with mass, inertia, thrust limits, and the gravity vector). These describe | ||
| the *hardware* and are independent of the dynamics formulation used to simulate it |
There was a problem hiding this comment.
E.g. L, which describes the hardware?
| filtered, unfiltered = load_params(dynamics, drone), load_params(Dynamics(dynamics_name), drone) | ||
| assert filtered.keys() <= unfiltered.keys() | ||
| assert {"thrust_min", "thrust_max"} <= unfiltered.keys() |
There was a problem hiding this comment.
This is not a good API. The test result looks more surprising than something I want my function to guarantee.
2 participants
crazyflow/drones/params.tomlnow only holds the core parameters every drone needs:gravity_vec,mass,J,thrust_min,thrust_max. Everything else moves to theparams.tomlof the dynamics that uses it, i.e. the first principles coefficients and the platform data now live incrazyflow/dynamics/first_principles/params.toml. Values are unchanged. A commented example at the top of the core file documents the required keys.This move broke
crazyflow.drones.load_params, which used to be the only way to get hardware constants likepwm_maxunfiltered. Since those now live in a dynamics file, that loader is removed andcrazyflow.dynamics.load_paramstakes over: given a function it filters to its signature as before, given aDynamicsmode it returns every parameter of the drone for that dynamics.Why
It was unclear which parameters a new drone actually needs. Now a drone that only runs the fitted models needs five keys, and the first principles section carries real content instead of being an empty marker. This prepares the availability methods PR, which derives
available_dronesand the supported drone/dynamics pairs from these files, and the X500 PR, the first drone without first principles parameters.