Cleanup and improve sim battery#32124
Conversation
|
|
||
| Aircraft::Aircraft(const char *frame_str) : | ||
| frame(frame_str) | ||
| Aircraft::Aircraft(const char *unused_frame_str) |
There was a problem hiding this comment.
I looked at removing this, but it looks like ~20 children also have it because this does. 🙁
I'm happy to do that in a separate cleanup PR, but it would distract too much from the focus of this one.
| // OPTIONAL battery model | ||
| // ("OPTIONAL" because a child can ignore it and directly set/get battery_* members.) | ||
| Battery battery; |
There was a problem hiding this comment.
I don't love how every Aircraft has a SITL::Battery but many ignore it. That's a slightly smelly design. But its consistent with the current state of the code. Hence me merely adding a comment here to alert folks about that.
I welcome a spin-off discussion about the future of simulated batteries. I'd prefer that every aircraft (every instantiated child of SITL::Aircraft) either have a SITL::Battery and use it, or do something custom and not have a SITL::Battery inside at all.
As an example, an alternate design would be for "the sim" to own "the simulated battery" as a stand-alone object. While that's nonphysical, its consistent with SITL::Sim containing stuff like a SITL::Sprayer, SITL::Parachute, SITL::Buzzer, etc. which I presume would be physically carried by the aircraft.
|
TIL that (In case anyone is interested, there are targets which are only valid for Update: Fixed. |
003f6ac to
9b5d9d7
Compare
|
This is a surprisingly big change, the commits also need a squash. Rather than a larger rework it would be easyer to review if you could make the minimum change to get the new functionality. |
|
I'm not yet understanding the feedback, can you provide more detail?
Sure! I am happy to squash them together after the content is approved. (I assumed it is helpful to parse the changes step-by-step, but of course if a reviewer prefers not, just review the final outcome.)
What "new functionality" are you referring to?
Which step(s) should not be done? |
|
Rebasing on master to see if the tests now pass. (At least the most significant-looking one did for us locally yesterday...) |
9b5d9d7 to
f1f12b1
Compare
|
I have repro'ed the failure locally. Converting to draft while I fix it. |
Specifically, the children of Aircraft which have a Frame, naturally.
It uses the batt owned by Aircraft-children which also have a Frame.
f1f12b1 to
adba694
Compare
Description
This PR prepares for every simulated vehicle to implement battery consumption (#10050).
It causes minimal (no?) changes in observed behavior. A follow-up PR will switch vehicles to actually using these improvements. (If you're interested, preview that PR here.)
Summary of significant changes:
SIM_BATT_VOLTAGEandSIM_BATT_CAP_AHparameter descriptions for clarity & completeness. These define the user-facing behavior.Testing (your PR may be summarily closed without these)
Side-by-side of EvaluateBatteryModel shows no changes (figures below).
Simple MultiCopter (
--model x) in SITL: arm, takeoff, hover, land.Recommendations
Reviewers: Please start with the descriptions of
SIM_BATT_VOLTAGEandSIM_BATT_CAP_AHparameters. In case I interpreted these incorrectly, we must sort that first! (They're in the last commit.)After that, I recommend skimming commit-by-commit to understand the individual steps being taken.
Testing images from example
Observe that before-and-after are the same.
After this PR:

Reference image (from here in repo):
