Skip to content
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

DJI F450 quadcopter alpha1 release #574

Merged
merged 8 commits into from
Feb 1, 2022

Conversation

pbecchi
Copy link
Contributor

@pbecchi pbecchi commented Jan 21, 2022

This PR contain an example for testing brushless DC motor. The example is a DJI F450 quadcopter FDM.

@seanmcleod
Copy link
Member

seanmcleod commented Jan 21, 2022

Thanks @pbecchi this is much easier to review.

Very initial comment, it looks like roughly half the files have ^M at the end of each line and half don't, looks like some mixture of LF versus LF + CR?

@pbecchi
Copy link
Contributor Author

pbecchi commented Jan 21, 2022

Yes it could be since I work on Windows and Visual Studio , I will try to correct all those at first commit.....
I see there is also some automatic check errors, but I dont understand what they are about.....

@seanmcleod
Copy link
Member

I also work with Visual Studio on Windows. Visual Studio will typically handle files with either CRLF or just LF and keep it that way, and prompt you when you open a file which uses a mixture asking if you want to normalize them. But there is then also your git settings in terms of how it handles line endings, potentially converting on the fly when pushing to the repo and pulling etc.

However I think I possibly saw these ^M in @rega0051's repo originally.

@seanmcleod
Copy link
Member

In terms of the build failures I noticed that TestInitialConditions.py is failing, with a unit lookup failing. I seem to remember that the commit for the BLDC included some new unit conversions for electrical properties, maybe one of the existing unit conversions was accidently deleted?

======================================================================
ERROR: test_initial_conditions_v1 (__main__.TestInitialConditions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:/a/jsbsim/jsbsim/tests/TestInitialConditions.py", line 120, in test_initial_conditions_v1
    self.CheckICValues(vars, 'script %s' % (f,), fdm, IC_root)
  File "D:/a/jsbsim/jsbsim/tests/TestInitialConditions.py", line 168, in CheckICValues
    conv = var['unit'][var_tag.attrib['unit']]
KeyError: 'M/S'

<!-- E305 Motor is a DJI 2312E - 960 -->
<power unit="WATTS"> 320 </power>
<velocityconstant> 960 </velocityconstant>
<torqueconstant> 6.1217 </torqueconstant>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit confused about <torqueconstant>. Is this supposed to be a user supplied value for Kq as per - http://web.mit.edu/drela/Public/web/qprop/motor1_theory.pdf

Although this element isn't read in by FGBrushLessDCMotor::FGBrushLessDCMotor(), instead we have the following in the header file.

constexpr double TorqueConstant = 60 / (2 * M_PI * NMtoftpound);

So what is the relationship between them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to theory Kq (torque constant) can be assumed to be 1/Kv (speed constant ) if we define speed in rad/sec torque in NewtonMeter.
Therefore since torque is in ft
pound a unit conversion factor is necessary (the conversion factor is TorqueConstant)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is confusing, nomenclature from the motor theory document:

image

However your variable, TorqueConstant is a constant conversion factor. Whereas when I was initially reading the source code and comparing it to the motor model document I assumed your TorqueConstant was referring to Kq.

image

Also given this:

image

Shouldn't we allow the user to optionally specify the torque constant Kq if they want to model the efficiency more accurately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i agree....

@pbecchi
Copy link
Contributor Author

pbecchi commented Jan 21, 2022

Let me answer your questions:
I will fix all line ending in next commit
I forgot to delete TorqueCostant from motor.xlm : you are correct it is not needed anymore.
Regarding unit let me ask you a stupid question( since I still have to learn......a lot ):
Since we added unit definitions on C++ code do we need to add the unit on the properties defined in motor.xml? So for example, I need to write:
<maxvolts unit="VOLTS"> 14.6 </maxvolts>??

@seanmcleod
Copy link
Member

I forgot to delete TorqueCostant from motor.xlm : you are correct it is not needed anymore.

Is this because you're working on the assumption that Kq == Kv?

From First-Order DC Electric Motor Model

The torque constant can be simply assumed to be the same as Kv
Alternatively, it can be obtained from motor torque data if this is available.

If so, why not allow the user to specify it optionally, and if it isn't supplied then assume Kq == Kv?

Since we added unit definitions on C++ code do we need to add the unit on the properties defined in motor.xml?

Currently you only have a single set of units for each electrical type, so at the moment it's really optional.

@pbecchi
Copy link
Contributor Author

pbecchi commented Jan 24, 2022 via email

@bcoconni
Copy link
Member

I have fixed the conflicts by merging master to the PR. Now it builds successfully.

Copy link
Member

@bcoconni bcoconni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. This will give a means to test the brushless DC motor model.
A bit of cleanup is needed before merging:

  • Please remove the file aircraft/F450/F450-set.xml as it is not used by JSBSim.
  • Please remove unused parameters from engine/DJI_E305.xml (i.e. the parameters power and deceleration_factor).

@bcoconni bcoconni merged commit 05c0b17 into JSBSim-Team:master Feb 1, 2022
@bcoconni
Copy link
Member

bcoconni commented Feb 1, 2022

PR merged. Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants