Move parameters and update load_params - #119
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.
52ee07d to
634a264
Compare
…rate standalone duplicates per model.
amacati
left a comment
amacati
left a comment
There was a problem hiding this comment.
Are we sure we want the gravity vector like this? Imo it overcomplicates things way too much
|
|
||
| Args: | ||
| fn: The controller function for which to load parameters. | ||
| controller: Name of the controller package, e.g. ``"mellinger"``. |
There was a problem hiding this comment.
For future reference: This should be a StrEnum at some point.
| with open(Path(__file__).parent / "params.toml", "rb") as f: | ||
| global_params = tomllib.load(f) |
There was a problem hiding this comment.
This is a single parameter from a file we don't expect to grow. I find this a bit strange
There was a problem hiding this comment.
In the future, maybe there will be constants on air viscosity etc. Things that sometimes live in the xml files for MuJoCo.
| # Global parameters shared by every drone and dynamics model. Drone specific parameters live in | ||
| # the params.toml of each model. | ||
|
|
||
| gravity_vec = [0.0, 0.0, -9.81] # m/s^2 |
There was a problem hiding this comment.
I find this very odd, especially since we need dedicated machinery just to load and merge this one file. If we don't even share the mass across drones, then why bother with gravity_vec?
There was a problem hiding this comment.
I did this because gravity is the only param that is not distinct to each drone - it's global/environmental. Other simulators just hard code this.
There was a problem hiding this comment.
Should we? Do we gain anything from having this in a config file?
There was a problem hiding this comment.
Flexibility is what we gain. See comment above - we might want to add things in the future. Also, let's say someone want to build quadrotors for Mars. This is something that belongs into a config in a proper system an not be hard coded imo
There was a problem hiding this comment.
Updated docstring
Co-authored-by: Martin Schuck <57562633+amacati@users.noreply.github.com>
12e8594 to
f6602f7
Compare
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.