bird-sanctuary / bluejay

:bird: Digital ESC firmware for controlling brushless motors in multirotors
GNU General Public License v3.0
301 stars 32 forks source link

Remove delay in wait_for_start_nonzero #190

Open ahalekelly opened 4 months ago

ahalekelly commented 4 months ago

There's a 100ms delay in wait_for_start_nonzero because with PWM input you sometimes have glitch pulses from electrical noise, and you don't want to start the motor on a glitch. But bluejay no longer supports PWM input and is dshot-only, so glitch inputs are no longer possible, particularly because there's a checksum in the dshot protocol.

This 100ms delay is a major issue for applications which need to start a stopped motor, such as nerf blasters and robotics. Currently blheli_s and bluejay are unusable in these applications. I haven't tested to confirm, but Mathias told me that this one-line change should fix that

github-actions[bot] commented 4 months ago

Build artifacts:

stylesuxx commented 4 months ago

Hey, thank you!

I need to check chat logs/comments, I think this delay had some other reason too according to Mathias, if I remember correctly.

CC @damosvil - do you see any reason why we should not remove this delay?

damosvil commented 4 months ago

That delay exists as a safety measure in case the fc may send frames before arming the drone. So on order to start the motor the esc shall reveive non zero and no command frames for more than 100ms.

El mié, 3 abr 2024, 11:12, Chris L. @.***> escribió:

Hey, thank you!

I need to check chat logs/comments, I think this delay had some other reason too according to Mathias, if I remember correctly.

CC @damosvil https://github.com/damosvil - do you see any reason why we should not remove this delay?

— Reply to this email directly, view it on GitHub https://github.com/bird-sanctuary/bluejay/pull/190#issuecomment-2034001888, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWBQ62BD6XBNZRPWQLSDPDY3PBZLAVCNFSM6AAAAABFUMLCR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZUGAYDCOBYHA . You are receiving this because you were mentioned.Message ID: @.***>

PippoAe commented 4 months ago

I appreciate every change that makes things faster!

"This 100ms delay is a major issue for applications which need to start a stopped motor, such as nerf blasters and robotics" This applies only to when the ESC is receiving the first throttle inputs after startup (aka. not armed yet) right? Or are these 100ms added to every throttle input when the ESC is sitting ready and armed at 0 speed?!

ahalekelly commented 4 months ago

This 100ms delay is added every time the ESC is armed and starting from 0 speed. Keeping a 100ms delay when arming would be fine for nerf and robotics applications, I think there's a separate 100ms delay on arming at line 595?

PippoAe commented 4 months ago

This 100ms delay is added every time the ESC is armed and starting from 0 speed. Keeping a 100ms delay when arming would be fine for nerf and robotics applications, I think there's a separate 100ms delay on arming at line 595?

Holy shit.... I have timed the motor startup and watched it in super slowmotion. Came to the conclusion that (my) motor needs about 180ms to go from 0 to spinning. Thought this is all credited to the motor first having to figure out the correct rotation before speeding up. Now you're telling me that this is 100ms pure waiting and only 80ms actually starting up? Holy cow!

This pullrequest has my dearest desperate upvote then!

stylesuxx commented 4 months ago

... I think there's a separate 100ms delay on arming at line 595?

This is for BB1 MCUs only.

ahalekelly commented 4 months ago

True, but there's also ones at lines 611 and 625 with different conditions, I think it covers all possible conditions?

Also fyi, none of blheli_32, AM32, or simonk seem to have any delay once the ESC is armed, even though they all support PWM input, it's only blheli_s and derivatives.

stylesuxx commented 4 months ago

Yep, those are for DSHOT rate detection.

The point is, for Bluejays intended purpose - controlling brushless motors in multirotors - those 100ms do not play a role whatsoever. I am having a hard time removing this delay without knowing what the possible side effects might be and super thorough testing. I do understand that it might matter for blasters and other applications (that are out of scope for this project). Still I am not willing to risk quads blowing up in peoples faces because we wanted to "save" 100ms.

howels commented 4 months ago

In fact for applications needing rapid motor start stop it is better to use reversible mode and a deadband instead of starting from a dead stop each time. Go live, put the motor in neutral then go full positive or full negative when firing.

PippoAe commented 4 months ago

(Im not - yet - in any way fluent in assembler!) Looking at the code i currently interpret this:

  1. gets non 0 throttle
  2. waits 100ms
  3. checks if throttle still non 0
  4. Initiate startup

As dshot only, we shouldn't get non 0 throttle by accident no more. But what if we'd get such a signal... If the value is 0 when checking exactly 100ms later, we'd abort. If the value is still above 0 when checking exactly 100 ms later, we'd start.

A noisy signal that has a jumping throttle (with a correct checksum) would get through that check either way, no? What this currently catches is a random spike above 0 that is not present 100ms later. Without the check, the motor would initate the starting and then stop on the next throttle checks down the line...

PippoAe commented 4 months ago

In fact for applications needing rapid motor start stop it is better to use reversible mode and a deadband instead of starting from a dead stop each time. Go live, put the motor in neutral then go full positive or full negative when firing.

Thanks for pointing that out! From neutral... can the motor just go full ham without having to 'probe' the correct motor rotation first? The only downside would be loosing a little(deadband) over 1000 steps of throttle resolution (in a usecase where only one direction is needed)?

PippoAe commented 4 months ago

That delay exists as a safety measure in case the fc may send frames before arming the drone. So on order to start the motor the esc shall reveive non zero and no command frames for more than 100ms. El mié, 3 abr 2024, 11:12, Chris L. @.> escribió: Hey, thank you! I need to check chat logs/comments, I think this delay had some other reason too according to Mathias, if I remember correctly. CC @damosvil https://github.com/damosvil - do you see any reason why we should not remove this delay? — Reply to this email directly, view it on GitHub <#190 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWBQ62BD6XBNZRPWQLSDPDY3PBZLAVCNFSM6AAAAABFUMLCR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZUGAYDCOBYHA . You are receiving this because you were mentioned.Message ID: @.>

The 100ms wait on line 727 is after after arming, no? There's several other 100ms waits for the arming procedure, but that wouldn't be affected by line 727 🤔

howels commented 4 months ago

Sounds like the client code needs to appreciate that the design is to power-on, establish communication with the ESC then set motor levels multiple times. As opposed to powering on the ESC for every motor change.

PippoAe commented 4 months ago

Sounds like the client code needs to appreciate that the design is to power-on, establish communication with the ESC then set motor levels multiple times. As opposed to powering on the ESC for every motor change.

Hmm... I'd guess every somewhat thoughtful client to appreciate this design. But that doesn't make those additional 100ms disappear when starting from 0 throttle, does it? (Except - as you pointed out 🙏 - in 3D mode)

In my case, client MCU and ESC's start together (same power) MCU establishes connection once and sends valid DSHOT frames until the whole system shuts down again. While the motors are not needed the ESC is sent 0 throttle.

howels commented 4 months ago

Sounds like the client code needs to appreciate that the design is to power-on, establish communication with the ESC then set motor levels multiple times. As opposed to powering on the ESC for every motor change.

Hmm... I'd guess every somewhat thoughtful client to appreciate this design. But that doesn't make those additional 100ms disappear when starting from 0 throttle, does it? (Except - as you pointed out 🙏 - in 3D mode)

In my case, client MCU and ESC's start together (same power) MCU establishes connection once and sends valid DSHOT frames until the whole system shuts down again. While the motors are not needed the ESC is sent 0 throttle.

If you are armed and are at a very low non-zero throttle value in general, then it is possible to avoid the motor spinning while passing a non-zero value to the ESC. As long as input is zero at startup the ESC will go live, then you can select a non-zero deadband value even without reversible mode, and avoid the 100ms delay.

PippoAe commented 4 months ago

Sounds like the client code needs to appreciate that the design is to power-on, establish communication with the ESC then set motor levels multiple times. As opposed to powering on the ESC for every motor change.

Hmm... I'd guess every somewhat thoughtful client to appreciate this design. But that doesn't make those additional 100ms disappear when starting from 0 throttle, does it? (Except - as you pointed out 🙏 - in 3D mode) In my case, client MCU and ESC's start together (same power) MCU establishes connection once and sends valid DSHOT frames until the whole system shuts down again. While the motors are not needed the ESC is sent 0 throttle.

If you are armed and are at a very low non-zero throttle value in general, then it is possible to avoid the motor spinning while passing a non-zero value to the ESC. As long as input is zero at startup the ESC will go live, then you can select a non-zero deadband value even without reversible mode, and avoid the 100ms delay.

Another possible workaround to note down, thanks! Lets say I'd send 1/2000 throttle instead of 0.. wouldn't that make the ESC always be in a kind of limbo trying to start the motors? Sounds kinda sketch :)

howels commented 4 months ago

Sounds like the client code needs to appreciate that the design is to power-on, establish communication with the ESC then set motor levels multiple times. As opposed to powering on the ESC for every motor change.

Hmm... I'd guess every somewhat thoughtful client to appreciate this design. But that doesn't make those additional 100ms disappear when starting from 0 throttle, does it? (Except - as you pointed out 🙏 - in 3D mode) In my case, client MCU and ESC's start together (same power) MCU establishes connection once and sends valid DSHOT frames until the whole system shuts down again. While the motors are not needed the ESC is sent 0 throttle.

If you are armed and are at a very low non-zero throttle value in general, then it is possible to avoid the motor spinning while passing a non-zero value to the ESC. As long as input is zero at startup the ESC will go live, then you can select a non-zero deadband value even without reversible mode, and avoid the 100ms delay.

Another possible workaround to note down, thanks! Lets say I'd send 1/2000 throttle instead of 0.. wouldn't that make the ESC always be in a kind of limbo trying to start the motors? Sounds kinda sketch :)

Not really, the heating effect should be very minimal. This is what will happen on multirotors after arming before take-off, there is some noise on the control channels and gimbals don't always read 0 at full deflection so multi-rotors will sit there with some low value non-zero throttle for some time.

howels commented 4 months ago

As others pointed out above - makes more sense to alter the client code for your use case instead of changing the behaviours of a core ESC safety code that may affect multirotor users.

PippoAe commented 4 months ago

As others pointed out above - makes more sense to alter the client code for your use case instead of changing the behaviours of a core ESC safety code that may affect multirotor users.

I like how that leftover of PWM days suddenly turned into 'core esc safety code'. damosvil called it a safety measure before arming, but the line we're talking about is AFTER arming and doesn't have anything to do with the arming procedure.

I'll try to set up the environment to build this baby and test it.

PippoAe commented 4 months ago

Sounds like the client code needs to appreciate that the design is to power-on, establish communication with the ESC then set motor levels multiple times. As opposed to powering on the ESC for every motor change.

Hmm... I'd guess every somewhat thoughtful client to appreciate this design. But that doesn't make those additional 100ms disappear when starting from 0 throttle, does it? (Except - as you pointed out 🙏 - in 3D mode) In my case, client MCU and ESC's start together (same power) MCU establishes connection once and sends valid DSHOT frames until the whole system shuts down again. While the motors are not needed the ESC is sent 0 throttle.

If you are armed and are at a very low non-zero throttle value in general, then it is possible to avoid the motor spinning while passing a non-zero value to the ESC. As long as input is zero at startup the ESC will go live, then you can select a non-zero deadband value even without reversible mode, and avoid the 100ms delay.

Another possible workaround to note down, thanks! Lets say I'd send 1/2000 throttle instead of 0.. wouldn't that make the ESC always be in a kind of limbo trying to start the motors? Sounds kinda sketch :)

Not really, the heating effect should be very minimal. This is what will happen on multirotors after arming before take-off, there is some noise on the control channels and gimbals don't always read 0 at full deflection so multi-rotors will sit there with some low value non-zero throttle for some time.

You normally have an idling speed somewhere around 3-5% where multirotors sit when armed and no throttle is applied by the pilot. Setting idling speeds below the minimum motor speed leads to a twitching mess.

howels commented 4 months ago

As others pointed out above - makes more sense to alter the client code for your use case instead of changing the behaviours of a core ESC safety code that may affect multirotor users.

I like how that leftover of PWM days suddenly turned into 'core esc safety code'. damosvil called it a safety measure before arming, but the line we're talking about is AFTER arming and doesn't have anything to do with the arming procedure.

I'll try to set up the environment to build this baby and test it.

BlueJay is designed for and working fine with multirotors, so introducing non-multirotor features that will change the behaviour for multirotor pilots seems like a quick way to cause unexpected issues for the 99% of people who are using BlueJay in multirotors. The safety aspects of not unexpectedly spinning a powerful 2207 or 2809 25V motor and propeller near people's fingers cannot be overstated here.

PippoAe commented 4 months ago

Okay, let's wrap this up.

Current:

  1. Throttle command above 0 received.
  2. Wait 100ms
  3. Throttle input still above 0.
  4. Spin the motors.

After proposed change:

  1. Throttle command above 0 received.
  2. Spin the motors.

The 'unexpected issue' seems to be quite predictable: On one (possibly not so healthy) hand we'd have the motor chopping up people's fingers 100ms earlier if theres a sustained throttle input.
On the other hand, we'd have a bunch of happy campers that can finally chop off 100ms of valuable reaction time.

stylesuxx commented 3 months ago

@PippoAe yeah, that wrap up covers the obvious. Still we need according testing to make sure it has no other side effects.

We will consider this PR for our next bigger clean-up release, but please don't expect this anytime soon. If you need it "now", I would suggest running your own fork. The team is unfortunately currently a bit swamped private/work wise so our time is super limited right now.

PippoAe commented 3 months ago

@PippoAe yeah, that wrap up covers the obvious. Still we need according testing to make sure it has no other side effects.

We will consider this PR for our next bigger clean-up release, but please don't expect this anytime soon. If you need it "now", I would suggest running your own fork. The team is unfortunately currently a bit swamped private/work wise so our time is super limited right now.

Thanks for the reply 🙏

Absolutely no stress on my side. In the meantime, I'll try finally setting up to build and tinker myself.

Thanks for the great work!