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

Fix esc init #4322

Merged
merged 1 commit into from Oct 11, 2017
Merged

Fix esc init #4322

merged 1 commit into from Oct 11, 2017

Conversation

jirif
Copy link
Contributor

@jirif jirif commented Oct 10, 2017

This is workarround for issue #4257. It cannot be fixed by other way because during power up spurious pulse starting esc initialization (before any code is run, tested at OMNIBUS). Second initialization happen during motorDevInit call so the best way is initialize motors soon as posible what was done before db8698d whish showed/introduced this problem.

@Arakon Arakon mentioned this pull request Oct 10, 2017
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Good finding and good fix!

}
LED0_OFF;
LED1_OFF;

#ifdef USE_SERVOS
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the changes done in #4218, I'd move the LED blinking to after the servo init, just to revert the timing back to how it was before #4218 (or as close as possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikeller Yes,this spurious pulse will affect servos too, i moved it after ..

@adrianmiriuta
Copy link
Contributor

@jirif not working for FF_FORTINIF4

@mikeller
Copy link
Member

@adrianmiriuta: 'not working' as in still 2 sets of startup tones? If so, can you please confirm that this isn't already the case in RC5 or earlier?

@adrianmiriuta
Copy link
Contributor

@mikeller
i merged the PR into actual 3.3.0
and I still got 2x ESC init sound.

@mikeller
Copy link
Member

@adrianmiriuta: That's awkward. I just tested with BLHeli_S 16.63, on a BEEROTORF4, and the double init was fixed with this patch. (I also tested with KISS and BLHeli32, but none of them showed the problem with 3.2.0, so there is nothing to fix.)

But then since the problem seems to be a timing issue, and the fix is shifting the motor initialisation to an earlier point in the initialisation, different boards with different peripherals that need to be initialised, and different timings required by different ESCs might mean that this does not fix it for all possible combinations. The initialisation process is a complicated dance already, with different parts being dependent on each other, and some of them having timing requirements. This is just a 'cosmetic' issue, so users might have to live with it, in return for getting all the other components on their board initialised properly.

@adrianmiriuta
Copy link
Contributor

@mikeller
I have a FF_FORTINIF4 with blheli_s 16.67

I have done 2 Traces to get a clue

  1. FF_FORTINIF4 3.3.0
  2. NAZE3.1.7

they are quite different in the time the motor output is high after poweron.
but I do not know how this gets interpreted by blheli.

ff_fortinif4_3 3 0
naze_3 1 7

@mikeller
Copy link
Member

I guess the difference there is that the FF_FORTINIF4 has got a lot more on board peripherals that need to be initialised first, pushing the motor init farther back. There should be a difference in the output high time between without and with this patch, but possibly not enough to stop your ESCs from booting up before they get a valid signal.

@martinbudden
Copy link
Contributor

martinbudden commented Oct 11, 2017

@jirif , well found.

In terms of initialisation, the motor initialisation depends on little else and it seems it should be done as early as possible. So I think the whole block:

    mixerInit(mixerConfig()->mixerMode);

    uint16_t idlePulse = motorConfig()->mincommand;
    if (feature(FEATURE_3D)) {
        idlePulse = flight3DConfig()->neutral3d;
    }
    if (motorConfig()->dev.motorPwmProtocol == PWM_TYPE_BRUSHED) {
        featureClear(FEATURE_3D);
        idlePulse = 0; // brushed motors
    }
    mixerConfigureOutput();
    motorDevInit(&motorConfig()->dev, idlePulse, getMotorCount());
    systemState |= SYSTEM_STATE_MOTORS_READY;

should be moved to just after the call to updateHardwareRevision. And for neatness I'd move the call to mixerConfigureOutput to just after mixerInit, so the block becomes:

    mixerInit(mixerConfig()->mixerMode);
    mixerConfigureOutput();

    uint16_t idlePulse = motorConfig()->mincommand;
    if (feature(FEATURE_3D)) {
        idlePulse = flight3DConfig()->neutral3d;
    }
    if (motorConfig()->dev.motorPwmProtocol == PWM_TYPE_BRUSHED) {
        featureClear(FEATURE_3D);
        idlePulse = 0; // brushed motors
    }
    motorDevInit(&motorConfig()->dev, idlePulse, getMotorCount());
    systemState |= SYSTEM_STATE_MOTORS_READY;

@adrianmiriuta
Copy link
Contributor

@martinbudden
your ideea works

ff_fortinif4_3 3 0_fixed

@martinbudden martinbudden added this to the Betaflight v3.2.1 milestone Oct 11, 2017
@martinbudden
Copy link
Contributor

@jirif , just checked your latest version, and it seems you have accidentally deleted that block entirely.

@jirif
Copy link
Contributor Author

jirif commented Oct 11, 2017

@martinbudden here is one dependency left, ppmRxInit() need motors have initialized in ppmAvoidPWMTimerClash() so we should move it to its original location or whole

    if (0) {}
#if defined(USE_PPM)
    else if (feature(FEATURE_RX_PPM)) {
        ppmRxInit(ppmConfig());
    }
#endif
#if defined(USE_PWM)
    else if (feature(FEATURE_RX_PARALLEL_PWM)) {
        pwmRxInit(pwmConfig());
    }
#endif

after

latest revision moved led/beep block after servo init as @mikeller suggested

@martinbudden
Copy link
Contributor

@jirif , well spotted. Yes, move the block back to its original location. I'd also add a comment stating that the motor initialisation needs to occur at that point, so that someone doesn't move it again.

@mikeller mikeller merged commit cdb5bc1 into betaflight:master Oct 11, 2017
mikeller added a commit that referenced this pull request Oct 11, 2017
@jirif jirif deleted the fix_esc_init branch October 11, 2017 12:45
@0crap
Copy link

0crap commented Oct 11, 2017

Tested build 249 and this fix works on my Naze32.
Thx all involved!

@TimothyGold
Copy link

I just upgraded from a 3.2 RC to the full latest release of 3.2.1 on my Matek 405 AIO and am finding the double ESC init is happening. I didn't have this on the 3.2 RC.

@mikeller
Copy link
Member

mikeller commented Nov 1, 2017

@TimothyGold: The problem causing the double ESC init is the delay between power being connected and the flight controller initialising the ESC outputs. A lot of peripherials (like OSD) are initialised before the ESC outputs, and in some cases this causes enough delay for the ESC to initialise on power up, and then again when they get a valid signal. This isn't a problem in itself, and there isn't really a fix for it (except to disable some of the peripherals), since a lot of the initialisation has to be done within a certain time frame after powering up.

@TimothyGold
Copy link

@mikeller Sounds good. Not a problem. Just thought I would mention this in case it indicated anything that was missed through the initial issues reported. Thanks for reply.

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

Successfully merging this pull request may close these issues.

None yet

6 participants