Completely Detach Servo

[obrain17]

In current implementation twitches may occur when calling Servo.detach().

This is because after stopwaveform is performed immediately and then followed by digitalWrite(_pin, LOW) causing a different duty-factor on the PWM.

Completely cutting off the pin withpinMode(_pin, INPUT)solves this issue.

A welcome side-effect is that Servo.detach() would really detach the pin (high impedance) instead of setting it to LOW.

[dok-net]

@obrain17 Your PR is kind of running against my major rework of the waveform generator, PR #7022. But you are certainly on to something here.
If that gets merged, to stop the PWM without causing a final period that's unpredictably off, if things work as designed, this should be the right thing to do in Servo.detach() (may or may not actually work with esp8266 master branch, too):

void Servo::detach()
{
  if (_attached) {
    startWaveform(_pin, 0, REFRESH_INTERVAL, 1);
    delay(REFRESH_INTERVAL / 1000); // long enough to complete active period under all circumstances.
    stopWaveform(_pin);
    _attached = false;
    _valueUs = DEFAULT_NEUTRAL_PULSE_WIDTH;
  }
}

This way, first, without affecting period, the PWM duty cycle is cut to 0 for the next upcoming period. The waveform gets stopped only after the final real duty cycle has completed.

[dok-net]

@obrain17 I am extremely keen on getting your feedback on my code snippet above - if it works well for you, I would like to adopt it in my PR #7022 for completeness.

[obrain17]

@dok-net I just inserted your code snippet into my project and the servo rotated smoothly without "jumps" with detach().
But I must say it also could have been a good day today, because also a cross-check with the original code from master did not show the bad effects as well. Probably because I might be doing the detach() at different positions than last week?
I did not include the last line _valueUs = DEFAULT_NEUTRAL_PULSE_WIDTH; because I did not know a neutral pulse and thought a re-attach() should start at the latest position written before with write(_valueUs).

Anyway your code with controlled ending the waveform within a period makes much more sense than my easy workaround. So I will withdraw/close my PR.

Thank you for the good work!

[dok-net]

@obrain17 When you close this PR, please open an issue so we can keep track of the work to fix it - mention this PR, too, so the discussion isn't lost.
I'm unsure about my addition of
_valueUs = DEFAULT_NEUTRAL_PULSE_WIDTH;
Initializing to a clean defined state makes sense in my mind; I think that in general the fault lies rather with attach(), which specifies min/max, but always "jerks" to center. I will simply add another overload so one can select the pulse with that the servo attaches to in the first place. You'd have to store your current angle between detach and attach, but that applies to min/max anyway.
Now that you've verified the merit of the change for git master, I'm making the changes not to #7022, but in #7023 instead. A controversial one, but more focused, for what it's worth.

[Tech-TX]

Leaving the pin in input after detach() has bad effects with my servo: the servo sees noise from an adjacent pin and slams the 0 stop, groaning loudly. With a low it wouldn't be picking up noise from adjacent signals. I'll take a scope capture later this evening. I've noticed this with your PR, Dirk.

[dok-net]

@Tech-TX

I've noticed this with your PR, Dirk.

What do you mean? My PR #7022? There the GPIO remains in OUTPUT, like you say is better for signal stability. And PR #7023 is my suggested fix (best on top of #7022) for this issue behind, and superseding, this #7084. Am I making any sense :-) ?

[earlephilhower]

@Tech-TX, you might want to replace INPUT with INPUT_PULLUP manually and give your servo a try.

[earlephilhower]

INPUT leaves the pin in a high impedance state and subject to noise unless an external pull-up/down resistor is added to the circuit.

INPUT_PULLUP, however, will have a weak pull-to-VCC resistor placed on the pad. Two potential issues are:

1. Is "always 1" a valid, safe state for servo control inputs?
2. What is the "typical" input impedance of a servo, and how does it compare to the (weak, can't seem to find the exact value) internal pullup?

[Tech-TX]

That's weird, it's not doing it now, and looking at servo.cpp in your latest PR it's actively driven low. I remember seeing the pin at ~1V with noise on it when my servo hit the stops. Maybe that was when I had mixed versions of files due to a git hiccough. I'd been disconnecting the servo between reboots so I wouldn't burn it for the last several days. I snagged your latest updates yesterday.

Earle, either high or low should be safe, legal states. It's noise that can fry a servo as it tries to hit whatever it interprets as a valid pulse. I'll test and let you know the results with a high. The weak internal pullup is roughly 47K from my earlier measurements.

edit: Here's what I'd seen (last weekend or earlier this week):

With that, the servo hits the stop and hums / heats up. With either HIGH or LOW on the pin there's no hum, and no measurable temperature rise.

With pinMode(INPUT_PULLUP) and noise on an adjacent pin, it looks dangerous but I don't get any effects with this one servo:

That's not an exhaustive test of ALL servos, as I only have an old Hitec micro servo from one of my gliders to play with. The Signetics NE544 that they used long ago for servo amps isn't in production now, so I don't know what the current servos use to decode the pulse train and drive the servo.

My servo doesn't twitch when you detach() it. It's likely an antique, so all bets are off with current technology servos. I don't really want to run out and spend $1000USD to get a random sampling of servos to test with. 😛

[obrain17]

There are two issues I wanted to fix with my PR:

1. Do not jump to either LOW or HIGH Signal within a PWM Duty Cycle. Because this caused twitches in some (not all !) situations, obviously depending on the position you are in when interrupting the duty cycle.
   But I think this issue is fixed with the code of @dok-net above. I already commented about it.

2. After detach() set the signal of the servo's PWM input to a level it can deal with when "idle". I thought it would do best using its existing internal logic (e.g. using an internal pull-up or pull-down resistor). So I set my output to high impedance (=INPUT), and left the option to add two additional lines after detach: pinMode(pin, OUTPUT); digitalWrite (pin, HIGH or LOW) depending on the servo's behavior.

Anyway 1) is fixed with #7023 (containing the above mentioned code)
and 2) can easily be added after detach() with this fix: "Output LOW, HIGH or High-Impedance"

I will close my PR but create an issue instead so our discussion here does not get lost.

[obrain17]

Issue is fixed with #7023
Code that prevents twitches is

startWaveform(_pin, 0, REFRESH_INTERVAL, 1);
delay(REFRESH_INTERVAL / 1000);

to be called before detach() as workaround until #7023 is merged
Further discussion in #7138