-
Notifications
You must be signed in to change notification settings - Fork 466
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
BLDC engine added #560
BLDC engine added #560
Conversation
FRBLDC_first_release
I'd recommend only committing the core BLDC engine code in the initial pull request. Will make it easier for others to see/find the relevant code when looking at the commits without having to wade through tons of additional files related to aircraft specific examples etc. Rather submit the aircraft example making use of the BLDC engine as a separate commit/pull request. Also take a look at your CR-LF settings, I see a lot of the diffs are showing control-M characters. Lastly also match the coding standards in terms of the number of spaces for indentation. |
I wouldn't want to push the F450 into the JSBSim repo. I still haven't validated it against flight data. And there are a bunch of commented out blocks, which I think is just more confusing. I also have Aero.xml, Gear.xml, ... as separate files because of how I auto generate config files and select amongst options. A clean version of a monolithic F450.xml would be appropriate in the future. Also, this has a U of MN UAV Lab image, and I'm pretty sure the .ac file for FlightGear is actually an UltraStick25e... |
Thanks for the PR @pbecchi 👍 There is a bit of cleanup involved but you should start with a PR containing only the C++ code with the corresponding update to the build files ( |
@seanmcleod |
Sorry if I did not make myself clear but I meant that the example files should be merged in a separate PR for the only purpose of making this PR smaller and easier to review. I agree with you that including examples and documentation will be needed ...but in a separate PR. |
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.
Thanks again for the PR @pbecchi
There is quite a lot of changes that are requested:
- Cosmetic changes
- Do not use cryptic acronyms such as
bldc
for naming. That's OK for a proof of concept but not for production code. So please rename the filesFGBlbc.*
toFGBrushlessDCMotor.*
and the classFGBldc
toFGBrushlessDCMotor
. - Please match JSBSim code formatting conventions:
- No tabs.
- 2 spaces indentations.
- Camel case naming for class members.
- No trailing spaces.
- Opening brackets of methods shall be on a new line and on the leftmost column.
- Remove outdated/useless comments.
- Legal changes
- The C++ code shall be released under the LGPL2.0+ license. You will need Matt Vacanti's agreement for that.
- Implementation
- I am concerned by the consistency of the units used in your code. The units of
VelocityConstant
are unclear and I am quite skeptical thatRPM / (MaxVolts* VelocityConstant)
is dimensionless (but I'd be glad to be proven wrong 😃).- Please check the units consistency.
- Please specify the units using
FindElementValueAsNumberConvertTo
(note that you might need to update the containerconvert
insrc/input_output/FGXMLElement.cpp
with new units).
- Do real life brushless DC motor include an RPM controller ? I am surprised to find that the motor torque is computed from a target RPM
commandedRPM
...
Actually, the units can be consistent provided that:
|
@bcoconni Regarding Units: All those units are somehow "uniques", since there is no alternatives .... Regarding name of variables most are heritage from Vacanti code (commanded_rpm in reality is delta_rpm ) . another question : there is a standard copyright note I can copy from other files?? |
No problem. There is no rush.
You'll most likely need to add new lines to jsbsim/src/input_output/FGXMLElement.cpp Lines 119 to 121 in 2bd60cf
New lines will be for instance: convert["VOLTS"]["VOLTS"] = 1.0
convert["OHMS"]["OHMS"] = 1.0 You don't need to add unit conversion for a start, just add the units you need just as the example above.
Yes please.
No problem.
Yes, see the example below: Lines 2 to 29 in 2bd60cf
|
@bcoconni |
@pbecchi The build fails because you've forgotten to update the file Please take this opportunity to clean the tabs that you have inserted in Thanks. |
Done...wait for your comments! |
I made all corrections possible..... |
TargetTorque = min(InertiaTorque, TorqueAvailable) + TorqueRequired; | ||
} else { | ||
// Deceleration is due to braking force given by the ESC and set by parameter deceleration_time | ||
TargetTorque = TorqueRequired - min(abs(InertiaTorque)/(max(DecelerationFactor,0.01)*30),RPM*TorqueConstant/VelocityConstant/VelocityConstant/CoilResistance); |
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 wait the factor is approximately the time delay in seconds
Rather make this a constant with an appropriate name and comment so that anyone else looking at the code knows what it is.
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.
RPM*TorqueConstant/VelocityConstant/VelocityConstant/CoilResistance)
describes the energy dissipated by the Joule effect, doesn't it ?
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.
We're almost getting there @pbecchi. A few additional comments (sorry !) mostly to polish the details.
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
Co-authored-by: Bertrand Coconnier <[email protected]>
I have added units to motor speed constant Kv, i think it is necessary to avoid confusion. |
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.
OK now there has been a major improvement regarding units. The code looks much slicker and, with the exception of DecelerationFactor
, it is no longer using magic factors.
And just for clarification: the formula RPM*TorqueConstant/VelocityConstant/VelocityConstant/CoilResistance)
models the energy dissipated by the Joule effect, no ? But in that case, why isn't the Joule effect modeled during acceleration as well ? After all, the dissipation takes place as long as the current and the voltage are non-zero.
I'd suggest to keep these topics open for a later improvement.
V = MaxVolts * in.ThrottlePos[EngineNumber]; | ||
|
||
// Delta RPM = (input voltage - currentRequired * coil resistance) * velocity costant | ||
DeltaRPM = round((V - CurrentRequired * CoilResistance) * VelocityConstant); |
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.
What's the purpose of this call to round()
? Why should DeltaRPM
be rounded to the nearest integer ? I'd expect that to be destabilizing numerically the computations.
TargetTorque = TorqueRequired - min(abs(InertiaTorque)/(max(DecelerationFactor,0.01)*30),RPM*TorqueConstant/VelocityConstant/VelocityConstant/CoilResistance); | ||
} | ||
|
||
EnginePower = ((2 * M_PI) * max(RPM, 0.0001) * TargetTorque) / 60; //units [#*ft/s] |
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.
Why are you using max(RPM, 0.0001)
? What's wrong with getting EnginePower
null ? There again I'd expect this to destabilize the motionless state of the propeller.
TargetTorque = min(InertiaTorque, TorqueAvailable) + TorqueRequired; | ||
} else { | ||
// Deceleration is due to braking force given by the ESC and set by parameter deceleration_time | ||
TargetTorque = TorqueRequired - min(abs(InertiaTorque)/(max(DecelerationFactor,0.01)*30),RPM*TorqueConstant/VelocityConstant/VelocityConstant/CoilResistance); |
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.
RPM*TorqueConstant/VelocityConstant/VelocityConstant/CoilResistance)
describes the energy dissipated by the Joule effect, doesn't it ?
This pull request is relative to #333 and propose a new BrushLess DC motor to be incorporated as alternative to basic electric motor. BLDC motor code is based on basic "3 constant motor equations" and allow a good phisical model of motor performances.
It require 3 basic motor properties that are normally available from manufactures or easy to measure:
In addition a braking time constant has been added to allow change of braking torque:
Torque constant is just a unit conversion factor and maxvolts is battery voltage available. This voltage is nominal battery voltage that can be factored to consider internal and wires resistance. Next step could be to add a module that represent batteries (and wires) and that will output voltage as function of nominal battery data and SOC (state of charge).
This pull request include the source code files that have been modified as well as several examples.
Few examples have been tested and run OK. Some other are just an heritage from repositories from where have been taken @mvacanti and @rega0051 forks. May be they are unnecessary and it will be cleaner to remove it. I am not expert of PR procedures, so let me know what shoud be included.