Unacceptable defaults that destroy servos #7023
Conversation
|
You should consider doing this against the main Arduino core if these (imported) defaults are destructive. |
libraries/Servo/src/Servo.h
Outdated
| #define MAX_PULSE_WIDTH 2400 // the longest pulse sent to a servo | ||
| #define DEFAULT_PULSE_WIDTH 1500 // default pulse width when servo is attached | ||
| #define REFRESH_INTERVAL 20000 // minumim time to refresh servos in microseconds | ||
| #define MIN_PULSE_WIDTH 800 // the shortest duty cycle sent to a servo |
There was a problem hiding this comment.
what are the values from the standard Arduino servo library?
There was a problem hiding this comment.
The ones in master are the same as in Arduino's Servo library. This PS restricts the values to a smaller range.
#define MIN_PULSE_WIDTH 544 // the shortest pulse sent to a servo
#define MAX_PULSE_WIDTH 2400 // the longest pulse sent to a servo
#define DEFAULT_PULSE_WIDTH 1500 // default pulse width when servo is attached
#define REFRESH_INTERVAL 20000 // minumim time to refresh servos in microseconds
There was a problem hiding this comment.
@earlephilhower No, no restrictions now in the updated PR, but safer defaults, it is in no one's best interest to destroy quality manufacturer (Tower Pro) servos for people that just unsuspectingly try the example for the first time.
libraries/Servo/src/Servo.cpp
Outdated
| digitalWrite(_pin, LOW); | ||
| } | ||
| } | ||
|
|
||
| void Servo::write(int value) | ||
| { | ||
| // treat values less than 544 as angles in degrees (valid values in microseconds are handled as microseconds) | ||
| // treat values less than _minUs as angles in degrees (values equal or larger are handled as microseconds) | ||
| if (value < _minUs) { |
There was a problem hiding this comment.
I still think there needs to be stronger justification for changing default behavior from Arduino than it just bad for a specific servo.
There was a problem hiding this comment.
@Makuna have you looked at the ostensible default from Arduino and how nothing has been done there in 3.5 years over an active issue arduino-libraries/Servo#3 ?
The "safety limits" in the ESP8266 impl are a local invention, btw, I don't see the AVR impl enforcing any such restriction, but the code is obfuscated, probably to conserve RAM (int8_t instead of int) at the expense of CPU time.
Now, what is that default behavior...
There was a problem hiding this comment.
@dok-net Let me be a little more clear. Provide more details as to why deviating from the AVR/SAM/etc libraries is a good thing. What research have you done? Links to data to back up your statements. Links to documents that describe cautions from manufacture, etc. One servo that doesn't handle the defaults well doesn't seem like a strong enough reason.
I wrote my own for AVR that uses a separate timer for each channel so NO ISRs happen; and as you state, I used pulses in the range of 1ms - 2ms with a 1.5ms center as defaults; but mine doesn't limit the user, it just gives them something to play with and lets them keep it in range.(https://github.com/Makuna/PwmServo/blob/master/PwmServo.h)
There was a problem hiding this comment.
I don't know why the implementation in master deviates from Arduino AVR Servo as it stands ;-)
https://en.wikipedia.org/wiki/Servo_(radio_control)
I repeat, I agree and have reverted the change, that the outer limits of 200µs to 3000µs can remain in effect, mostly because according to your source code comments there are "extreme" servos in the field that we don't want to force users to having to patch the lib source for.
I obviously disagree with you on that stated "extreme" values can be the default for everyone. And yes, a single burned servo is definitely enough to change the defaults, it cost me a lot of time, ire and money.
There was a problem hiding this comment.
As to what happened, here's the story again, this can happen to anyone with any servo, the results may be more or less severe, but the gist of it is, the defaults should be inside safe limits (spec for servo protocol is 1000µs to 2000µs, as you confirm), finding out what the exact 0° and 180° values are for any given servo is a matter of calibration.
I inspected the sweep example, adopted the initialization code into my own sketch, started it, set a few values for the outputs, among them the servo - my device makes a little noise that can mask the servo's - and found the servo pressing against it's internal mechanical endstop, running hot while doing so, and before I knew it, it stopped working for good.
Mind you, this happens well below requesting 180° with the lib's defaults. I am not going to try this again, if you've some HW to spend, fine :-)
There was a problem hiding this comment.
I checked the Arduino issue on this and yes, it's been years without any response or motion, and in those years only had 2 thumbs up (one from dok-net, one from another user). It seems unlikely to move, but frankly that's normal for Arduino stuff.
I'm not really able to find a meaningful datasheet for consumer-type servos. Some websites report 500-2500 for certain models, while others are reporting 900-2200 for the same exact models.
That said, the only thing changing are the defaults, from a wider range to a (safer) narrower one. Attaching with specific mac/min values is still supported, and given the variability in the servos out there (aka clones) for real apps you're going to need an encoder or a manual tweaking anyway.
dok-net
force-pushed
the
servo
branch
2 times, most recently
from
7f99e1f
to
28a6b5e
Compare
|
This PR would fix the issue underlying #7084. |
|
I'm behind dok-net on this issue. The trim pots on my radios would allow up to ~20% deviation from standard timing, maximum. The default shouldn't exceed that without good reason as servos use max current when slammed to either stop. If one or two brands need wider timing, the users are allowed to recompile with whatever they need, but the default should always be the 'normal' range and not significantly wider. The Arduino library is flawed, so compatibility with it is still possible with a recompile. |
libraries/Servo/src/Servo.cpp
Outdated
| digitalWrite(_pin, LOW); | ||
| } | ||
| } | ||
|
|
||
| void Servo::write(int value) | ||
| { | ||
| // treat values less than 544 as angles in degrees (valid values in microseconds are handled as microseconds) | ||
| // treat values less than _minUs as angles in degrees (values equal or larger are handled as microseconds) | ||
| if (value < _minUs) { |
There was a problem hiding this comment.
I checked the Arduino issue on this and yes, it's been years without any response or motion, and in those years only had 2 thumbs up (one from dok-net, one from another user). It seems unlikely to move, but frankly that's normal for Arduino stuff.
I'm not really able to find a meaningful datasheet for consumer-type servos. Some websites report 500-2500 for certain models, while others are reporting 900-2200 for the same exact models.
That said, the only thing changing are the defaults, from a wider range to a (safer) narrower one. Attaching with specific mac/min values is still supported, and given the variability in the servos out there (aka clones) for real apps you're going to need an encoder or a manual tweaking anyway.
|
I haven't checked this latest commit, but with current master and a new-old-stock Futaba FP-S148 servo it buzzes at 180 degrees and heats up; I immediately cut the power rather than burn up the motor. I just received the Futaba from eBay today for $8, free shipping. ;-) This was THE standard servo for many years in planes. I'll re-check the PR Real Soon Now. |
|
With the new (old-school) defaults, all 3 of my servos move ~90 degrees end-to-end (0-180 control values), which is pretty common for a flight servo (+/- 45 degrees travel). Since you're changing an old default, I'd suggest an exposed set of variables so the users can change the desired min/max defaults in their program without modifying the library servo.h file, something like: If the user doesn't override the defaults, then it reverts to the new 1000us/2000us limits. Other than that, no issues found. |
|
@Tech-TX Stan, what are you talking about :-) You must have an outdated version of this PR's code branch. |
| @@ -74,35 +79,36 @@ uint8_t Servo::attach(int pin, uint16_t minUs, uint16_t maxUs) | |||
| _maxUs = max((uint16_t)250, min((uint16_t)3000, maxUs)); | |||
| _minUs = max((uint16_t)200, min(_maxUs, minUs)); | |||
There was a problem hiding this comment.
@Tech-TX These are the hard-coded limits.
| // attach the given pin to the next free channel, sets pinMode, min, max values for writes, | ||
| // and sets the initial value, the same as write. | ||
| // returns channel number or 0 if failure. | ||
| uint8_t attach(int pin, uint16_t min, uint16_t max, int value); |
There was a problem hiding this comment.
@Tech-TX Have you tried either of these two overloads of attach()?
There was a problem hiding this comment.
@dok-net I pulled it this morning, Dirk, but I missed the attach() parameters. 😉
I really need to add a bunch of *.rst or *.md help files or additions for the stuff in the libraries, especially where things differ from the original Arduino libraries. That would help minimize confusion for our Gentle Users (and me!). The current info at ReadTheDocs is woefully incomplete for the servo library. Here's the full quote:
Servo
This library exposes the ability to control RC (hobby) servo motors. It will support up to 24 servos on any available output pin. By default the first 12 servos will use Timer0 and currently this will not interfere with any other support. Servo counts above 12 will use Timer1 and features that use it will be affected. While many RC servo motors will accept the 3.3V IO data pin from a ESP8266, most will not be able to run off 3.3v and will require another power source that matches their specifications. Make sure to connect the grounds between the ESP8266 and the servo motor power supply.
I didn't know we had 24 I/O pins that could drive servos. You learn something every day... and now I need to hunt down the 13 other (missing) GPIO pins. 😄
Here's the info on the Arduino library: https://www.arduino.cc/en/Reference/ServoAttach
so the 8266 library has an INT VALUE option for the initial starting degrees that the older library doesn't. Also, the max servos went from 12 to 9 with this PR, so we should mention that somewhere.
Your PR is working OK with my test bed and servos.
|
The detach function now calls stopWaveform and delay to have a correct last pulse. This is only a workaround, not a solution: not only it is not recommended to have a delay of 20 ms in such a function, but it also doesn't solve the issue. |
dok-net
force-pushed
the
servo
branch
2 times, most recently
from
2f2dd66
to
39457b6
Compare
|
I just stumbled upon this issue. I opened a similar PR in the official Arduino library in April 2019, but sadly there's been no word from the project maintainers. Some of the details from the PR might be relevant / useful here: arduino-libraries/Servo#24. |
|
Is it a silly idea to: That will produce a useful message and encourage users to make more robust code. edit1 It is not intended to remove the function even in a far future |
|
After some discussion, @d-a-v 's proposal has merit, and in conjunction with @dok-net proposal in this PR seems to cover all bases. Our choice of different defaults is meant for safety, to not destroy hardware, and the user should look at setting min/max values that fit his specific servo type based on that servo's specs. A warning when using defaults should cover that, and let the user know that specific min/max values should be used. @dok-net could you please update the PR to include:
|
|
... or rather, there. |
… servos, this causes much confusion for the user, because a wide range of angle starting from 0 degress, and a just a large range of angle leading up to 180 degrees, is dead - in fact, the servo doesn't move at all, from any position. Not investigated further for obvious reasons, as this may have also destroyed servos that ran hot trying to abide by the PWM signal but could not mechanically!
…d safety limits, popular servos could force against internal physical endstops, which could overload and destroy them.
… timings. Internal review, fix/drop legacy comments.
…00 is minimum enforced elsewhere, so use it here too as nearest multiple of 100 from 180.
dok-net
force-pushed
the
servo
branch
2 times, most recently
from
79d312b
to
47ff617
Compare
Comment rationale for changed defaults of min/max pulse widths.
|
@devyte Besides looking really ugly to me (which is just a personal opinion, nevermind), the deprecation breaks sample sketches. Additionally, the Arduino AVR Servo lib has pending changes to the same effect as this PR: arduino-libraries/Servo#24, which was mentioned earlier in this thread by @rileyjshaw. I would like to think that the whole matter of being different from AVR is short-lived, we should focus instead on helping the AVR implementation getting updated. |
Your use of words like "ugly" that we are supposed to omit despite the fact that you wrote it on purpose does not help the discussion. Yes, deprecated messages are supposed to be fixed in sketches, that's why examples must be updated. That is the purpose The vast majority of arduino users do not care at all about warning messages, including deprecated API messages. Examples are here to show users how to use the API. Such a proposal will not make anything not compatible anyway.
Maybe doing this here, and telling them there will help.
.. but you made a PR here instead of highlighting and getting maintainers attention in the PR there. |
|
After discussion and because deprecation might be too intrusive, there's maybe a more acceptable way to emit a warning when the function is used the first time when in debug mode. |
Compatibility to AVR Arduino is not premium in this case, as the very much out of spec defaults for the servo PWM timings cause my servos to RUN HOT and PERMANENTLY FAIL.
The defaults can be overridden, but they must be within public specification for unsuspecting initial use!
Still under investigation is the recently patched maximum servo count limit, 12 seems, from current experience with running fan controls and servos, too high - besides, where can one connect 12 servos to an ESP8266 default GPIOs? AFAIK the servo lib does not apply to I2C servo extension boards.
EDIT: Now also contains a fix for servo jerk on detach. Before, detach would cancel the PWM to servo at a random time, which could be interpreted by the servo as an adjustment command to a random angle.