-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Default motor PWM configuration #21513
Conversation
aee9cda
to
7cd745b
Compare
StandardVTOL now works as expected. Other VTOL cases 👀 Line 100 in 0afd93a
|
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 for this push, I think it makes sense to align all this.
I didn't yet have time to in depth go through it in detail, but maybe answering my couple of questions already is enough for me to get a better understanding.
src/modules/control_allocator/ActuatorEffectiveness/ActuatorEffectivenessFixedWing.cpp
Outdated
Show resolved
Hide resolved
const matrix::Vector<float, NUM_ACTUATORS> &actuator_max) | ||
{ | ||
if (matrix_index == 0) { | ||
// Stop front facing motors when they are commanded 0 thrust |
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.
Don't we just want to keep them at PWM_MIN? Stop for me is to, well, stop completely, which is what most VTOL pusher/pullers should be confiugred to do, but if one wants to have them always spinning that should be possible by increasing PWM_MIN to higher than 1000 right?
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.
which is what most VTOL pusher/pullers should be confiugred to do
That's what I'm trying to solve that you have to configure PWM according to what the motor is supposed to do which is context the allocator has but you currently need to set manually.
if one wants to have them always spinning that should be possible by increasing PWM_MIN to higher than 1000 right?
It's possible and currently, every user has to manually set the PWM_MIN of any motor that should not stop spinning e.g. any multicopter motor to a higher value. Also consider this only works for PWM outputs and the special cases all have to be manually configured for any other interface. E.g. DSHOT pusher is not even possible because the functionality is captured manually in PWM configuration. Instead of re-adding some magic to guess which motor should not stop spinning without having the context I suggest configuring every motor to be the same way and the allocator to have the interface available to command a motor to stop depending on what it does. So in summary it's extending the VTOL motor start/stop interface to all cases a motor should start/stop.
break; | ||
|
||
case FlightPhase::HOVER_FLIGHT: | ||
case FlightPhase::TRANSITION_FF_TO_HF: | ||
case FlightPhase::TRANSITION_HF_TO_FF: | ||
_stopped_motors = 0; | ||
_stopped_motors &= ~_mc_motors_mask; |
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.
Would this stop the pusher in hover? It's needed for pusher support.
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.
Would this stop the pusher in hover? It's needed for pusher support.
No, exactly, good question. That's what I took care of. It only enables and disables mc motors but the pusher is only off when it's commanded zero thrust. This can be extended to where the controller decides to always run it even if just idling but currently for 1.14, it just works again like it was.
7cd745b
to
9ed24eb
Compare
Thanks for carrying this through the finish line before the release @MaEtUgR |
9ed24eb
to
d831832
Compare
1a56c49
to
75a80f1
Compare
ok, I went through all vehicle types including boat, uuv, tiltrotor in simulation and made sure it does what I expect: As discussed with @bkueng the next step would be to allow the controller to decide which motors are turning off or not but we need to discuss the interface there and I didn't want to introduce that bigger change for 1.14 now. |
75a80f1
to
f7b662f
Compare
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 I see makes sense to me. As discussed, let's though give this some testflying on all VTOL types early next week. That's something I anyway had planned to do to sign off the release.
const uint32_t motor_mask = (1u << actuator_idx); | ||
|
||
if (stoppable_motors_mask & motor_mask) { | ||
if (fabsf(actuator_sp(actuator_idx)) < .02f) { |
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.
Can you drop a comment for the magic number 0.02? And/Or make it a constant?
I'm not sure, maybe we should skip this one until we fix PWM calibration. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/px4-maintainers-call-may-09-2023/32047/1 |
f7b662f
to
3ca90ff
Compare
Just rebased on main again since @sfuhrer and I go out to flight test the latest state of main including this. |
Flight tested together with @MaEtUgR on a MC, FW, VTOL tailsitter, VTOL standard, VTOL tiltrotor. No issues around the new PWM logic as proposed in this Pr was found. All VTOLs switched off the motors not in use correctly. On one vehicle (the tiltrotor), the ESCs required a minimum PWM of 1150 to turn, and the ESC was not configurable. On all other vehicles the default range (1100-1900 for motors) was kept. I didn't check if the ESC response would have gone above 1900. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/px4-maintainers-call-may-16-2023/32137/1 |
For this one I'm looking into making sure all the PWM calibrations are always done with 2000us and then 1000us as the reference values which right now they are the configured |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/px4-maintainers-call-may-23-2023/32245/1 |
3ca90ff
to
bee2f4b
Compare
Now that #21711 is merged PWM outputs can consistently be calibrated on 1.14. So I:
I plan to merge this, inform users to recalibrate PWM/Oneshot ESCs also in the 1.14 release notes and if necessary update the documentation. |
Let's make sure the release notes are calling out any user actions for upgrading |
For the 1.14 release because we already changed the actuator configuration and users will need to go through their setups I also corrected the PWM calibration and the default PWM values. See PX4/PX4-Autopilot#21711 and PX4/PX4-Autopilot#21513 This is the documentation change with it. And needs to be referenced by the release notes.
Here's the documentation change for this mainly with the fixed PWM calibration (#21711): PX4/PX4-user_guide#2599 That's what we need to reference in the release notes. Something along the lines of:
|
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/px4-maintainers-call-july-04-2023/32943/1 |
For the 1.14 release because we already changed the actuator configuration and users will need to go through their setups I also corrected the PWM calibration and the default PWM values. See PX4/PX4-Autopilot#21711 and PX4/PX4-Autopilot#21513 This is the documentation change with it. And needs to be referenced by the release notes.
I think PX4/PX4-user_guide#2599 should be ready now. So once the doc is available via URL we can merge this and notify + reference what should be done. |
For the 1.14 release because we already changed the actuator configuration and users will need to go through their setups I also corrected the PWM calibration and the default PWM values. See PX4/PX4-Autopilot#21711 and PX4/PX4-Autopilot#21513 This is the documentation change with it. And needs to be referenced by the release notes.
PWM calibration was fixed with #21711 |
@MaEtUgR Does this apply to all Airframes? |
@vincentpoont2 This applies to all vehicle types and airframes that use one or more PWM or OneShot ESC to drive motors and are updated to 1.14 release candidate (not out yet, will suggest that today) or later (latest main with this pr). Fixed-wing and VTOL motors should be calibrated and configured the same way. The autopilot can now stop the motor by using the disarmed PWM automatically when required for these vehicle types. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/1-14-stable-holybro-x500-v2-motors-ramp-down/35326/5 |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/the-latest-firmware-for-cube-orange-issue/35341/15 |
Solved Problem
PWM configuration parameters for ESCs have ambiguous meanings depending on the use case and have to manually be adjusted according to the context that the allocator already has.
Fixes #21506
Solution
PWM 900us is out of spec of any ESC I know and they all stop the motor with 1000us
This is the most compatible range I can think of with any ESC I worked with. Some have a lower limit of 1050us or 1075us but all turn the motor with 1100us. Some stop the range at 1925us or 1940us but all of them have the range not yet ended with 1900us. The idea is that the conservative defaults work reliably out of the box. If a user carefully follows the steps and e.g. experimentally finds out the exact PWM limits for all ESCs in use they can still configure that. From looking at logs I see that most users don't do that.
Changelog Entry
For release notes:
Documentation improvement pending: PX4/PX4-user_guide#2599
Alternatives
I also thought about keeping the ambiguity of PWM configuration and adding some magic again to change it automatically but just for multicopters in one way and for fixed wings the other way. But I think that's not the right solution because it will cause more confusion and corner cases and explicit configuration and use.
Test coverage
What's still missing are the VTOL cases. I think I can figure out standard VTOL but might need some help with Tiltrotor, let's see.EDIT: See above what I covered.Context
1.14 release has only the new actuator configuration now and I want to make sure we get these PWM problems out of the way from the beginning since many people will switch and reconfigure the actuators once.