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

Remove delay in wait_for_start_nonzero #190

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ahalekelly
Copy link

@ahalekelly ahalekelly commented Apr 3, 2024

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

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 glitches 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, and this one-line change would fix that
Copy link

github-actions bot commented Apr 3, 2024

Build artifacts:

@stylesuxx
Copy link
Contributor

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?

@stylesuxx stylesuxx changed the base branch from main to develop April 3, 2024 09:12
@damosvil
Copy link
Contributor

damosvil commented Apr 4, 2024 via email

@PippoAe
Copy link

PippoAe commented Apr 4, 2024

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
Copy link
Author

ahalekelly commented Apr 4, 2024

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
Copy link

PippoAe commented Apr 4, 2024

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
Copy link
Contributor

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

This is for BB1 MCUs only.

@ahalekelly
Copy link
Author

ahalekelly commented Apr 4, 2024

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
Copy link
Contributor

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
Copy link

howels commented Apr 5, 2024

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
Copy link

PippoAe commented Apr 5, 2024

(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
Copy link

PippoAe commented Apr 5, 2024

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
Copy link

PippoAe commented Apr 5, 2024

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
Copy link

howels commented Apr 5, 2024

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
Copy link

PippoAe commented Apr 5, 2024

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
Copy link

howels commented Apr 5, 2024

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
Copy link

PippoAe commented Apr 5, 2024

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
Copy link

howels commented Apr 6, 2024

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
Copy link

howels commented Apr 6, 2024

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
Copy link

PippoAe commented Apr 6, 2024

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
Copy link

PippoAe commented Apr 6, 2024

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
Copy link

howels commented Apr 6, 2024

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
Copy link

PippoAe commented Apr 6, 2024

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
Copy link
Contributor

@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
Copy link

PippoAe commented Apr 8, 2024

@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!

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.

5 participants