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

added support for motor test on brushless motors #820

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

ntamas
Copy link
Contributor

@ntamas ntamas commented Aug 1, 2021

The "Propeller test" button on the Console tab of the Crazyflie client does not work with brushless motors at the moment because the 50 msec period during which the motor is operated at full throttle is not long enough for a brushless motor to spin up. (At least definitely not with the motors that we have here in the lab). This PR adds support for testing brushless motors by making it possible to use different spinup durations and PWM ratios, depending on whether the motor is brushed or brushless.

Notable changes in the brushless test (the brushed test remains the same):

  • The motor spins for 2 seconds instead of 50 msec.
  • The delay between consecutive motor tests is 1 second instead of 950 msec.
  • The PWM ratio when the motor spins is approximately 15% instead of 100%.
  • XY (and Z) acceleration is measured for 100 msecs; the measurement starts 1 second after spinning up the motor.

These settings are identical to what we use on our outdoor drones running ArduCopter; feel free to change the parameters if you feel fit.

The "beep-beep-beep" warning for motors that have a large XY vibration is not implemented for brushless motors yet; this could be the subject of another PR, I am soliciting feedback on this one before proceeding further.

I was a bit unsure whether the parameters of the health test should be in motors.c or health.c; right now they are in motors.c because this way the motor driver can keep it private whether the motor is brushed or brushless. The alternative would be to add a motorsGetType(uint32_t id) function that returns the type of the motor; the health module could then check whether it's brushed or brushless and we can then move the parameters there. Let me know which solution you feel more comfortable with in the long run.

@ataffanel
Copy link
Member

Hi, thanks a lot for the PR, this looks great!

Our Brushless expert, @tobbeanton, is currently on vacation and will be back in 2 weeks, I think it is best if he has a look at it when he is back.

@ataffanel ataffanel requested a review from tobbeanton August 5, 2021 08:51
@tobbeanton
Copy link
Member

This all looks good and a great addition!
One thing that worries me is the safety aspect when it comes to brushless and more powerful drones. How does ArdoCopter tackle this? Can perhaps 15% throttle be considered safe? Also we have the arming mode where the brushless motors idle (spins slowly) when it is armed. Can this effect the test and test result? Then we need to make sure all motors has stopped spinning before the test begins.

@ntamas
Copy link
Contributor Author

ntamas commented Aug 12, 2021

ArduCopter sidesteps this problem by requiring the user to supply the PWM ratio in a MAVLink message (see here ), and it is up to the ground station software that you use to provide the appropriate safety measures. In other words, you can easily spin up a motor even to 100% at will by sending the appropriate MAVLink message, assuming that you do not have a hardware safety switch (which our drones do have, but it's not common for smaller drones).

The GUI of Mission Planner (the ground station that we use for testing when we are not using Skybrush) has a widget where the PWM ratio can be set. It has to be set to 15% for our drones, and we have also tested many other third party "show drones" that also require 10-15%, but they are all of roughly the same size and power so it does not mean anything. My assumption is that if you are designing a drone from scratch, chances are that you would aim for a hover thrust of ~30% or so, so using 15% thrust for a motor test probably won't cause any harm. We can lower it to 10% of course, or we can make it a configurable parameter (but then this parameter won't do anything for brushed motors).

Re the arming mode: we haven't tried that, but on our ArduCopter-based drones you can perform a motor test only when the drone is not armed (otherwise the firmware prevents the motor test).

@ntamas
Copy link
Contributor Author

ntamas commented Aug 12, 2021

Maybe the easiest safety measure that could be implemented in the Crazyflie desktop client is to 1) detect if the motor is brushless and 2) ask the user to remove the props before testing. (Maybe a checkbox labeled "I have removed the propellers from all the motors" -- ticking the checkbox would enable the motor test button). But this might defeat the whole purpose of measuring vibrations while the motor is spinning.

@tobbeanton
Copy link
Member

Thanks for the info!

First thing I agree that the test should only be possible to run if the CF is disarmed.

We could have a parameter that is setting the PWM for the brushless test and this could be set to 0% by default so it has to be changed by either the user or a script/program for the test to work.

What do you think about the changes above?

@ntamas
Copy link
Contributor Author

ntamas commented Aug 21, 2021

Sounds good to me, but this means that the "Propeller test" button in the Crazyflie client would not work with the current version out of the box because it would not set the PWM ratio for the brushless test before attempting the test. This is OK for me (we will compile a custom firmware for our drones anyway and we can hardcode the default PWM ratio for brushless tests to be larger), but this needs to be handled on the cfclient GUI for the next release or so.

I'll (mostly) be on holiday for the next week but I'll try to get back to this PR when I'm back (at the latest).

@ntamas
Copy link
Contributor Author

ntamas commented Aug 26, 2021

Added a health.propTestPWMRatio parameter that overrides the default PWM ratio dictated by the new MotorHealthTestDef struct. At the same time, I also changed the default PWM ratio for brushless motors to be zero, meaning that the user would have to set health.propTestPWMRatio explicitly for brushless motors. The default is kept for brushed motors to make things compatible with the existing CF firmware.

@tobbeanton Is this what you were thinking about?

@tobbeanton
Copy link
Member

Yes exactly! I will test this as soon I get some time left over.

@tobbeanton tobbeanton merged commit cb21487 into bitcraze:master Aug 30, 2021
@tobbeanton
Copy link
Member

I did some small changes and added a zero pwm setting in beginning of propeller test. This way it works if there is a idleThrust set. I also moved the rate supervisor inside the estimator stabilizer loop to get rid of the warning message in the console during the propeller test.

Great work!

@ntamas ntamas deleted the feature/brushless-prop-test branch August 30, 2021 19:05
@knmcguire knmcguire assigned knmcguire and unassigned knmcguire Oct 27, 2021
@knmcguire knmcguire added this to the next release milestone Oct 27, 2021
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.

4 participants