arduino-libraries / Servo

Servo Library for Arduino
http://arduino.cc/
GNU Lesser General Public License v2.1
252 stars 262 forks source link

bug in SAMD implementation, SAMD51 support #77

Open agrommek opened 3 years ago

agrommek commented 3 years ago

There is a bug in the SAMD implementation of the servo library.

Steps to reproduce Use a SAMD21-based board (Arduino Zero, Adafruit Feather M0, ...) Create 12 servos objects. According to specs, up to 12 servos are allowed. Attach all servos to different pins. Set the pulse width for each servo to at least 1850 μs. This value is a perfectly normal pulse width for hobby servos.

Expected result All pulse width are as configured and the pulse repetition rate is about 50 Hz (i.e. the spacing between the rising edges of the pulses is about 20 ms) on any configured pin.

Observed result The pulse widths are ok, but the pulse spacing for all servos is way off. The pulse spacing is about 41 ms, corresponding to a repetition rate of less than 24 Hz. This can be clearly seen on an oscilloscope. Although the repetition rate for servos is said to be not that critical, those large pulse spacings are problematic.

Detailed bug analysis The bug is caused by the unintentional overflow of the 16 bit counter of the SAMD Timer/Counter (TC) instance. The TC is clocked by a scaled down CPU clock. The CPU clock is 48 MHz (on SAMD21, i.e. Arduino Zero & friends) and the prescaler is 16, resulting in an effective clock speed of 3 MHz for the TC peripheral. 3 MHz correspond to 3 clock ticks per microsecond, or 60000 clock ticks for the desired 20 ms repetition rate (50 Hz). The TC counter is used in 16 bit mode and wraps back to 0 after 2^16 = 65536 clock ticks, so counting to 60000 with a 16-bit-counter is perfectly ok. 65536 clock ticks correspond to 21.845 ms. Trying to count longer than that causes the internal counter to overflow and start from zero again. This is what happens here. When configuring many servos and/or long servo pulses, the total sum of the pulse width cannot be multiplexed (i.e. "squeezed") into the 20 ms interval required for 50 Hz repetition rate. If the total sum of pulse widths is bigger than 20 ms, but still smaller than 21.845 ms, not much happens. The repetition rate only gets slightly longer (21.845 ms correspond to about 45.8 Hz). I would already consider this a bug, although most servos could probably handle 45 Hz just fine. However, "really bad things" happen when longer pulse widths cause the counter to roll over (see example above: 12 * 1850 μs = 22.2 ms --> rollover happens before last pulse has finished). Due to unsigend integer arithmetic, the pulse widths are still ok, even the for the pulse(s) after rollover. The start of the first pulse of the next "multiplexing cycle" is always scheduled to occur when the counter reaches 60000 (again). When counter rollover happened, the total delay between pulses on any given pin is thus not 60000 clock cycles = 20 ms, but 65536+60000 clock cycles = 41.845 ms.

Possible solutions Let's call the fact that pulse repetition rate gets slightly larger for total pulse duration between 20 ms and 21.845 ms "consequence A" Let's call the doubling of the repetition rate due to counter rollover "consequence B".

Suggested solution Personally, I am a strong believer in "when you fix it, fix it properly", so I am in favor of the third option. Accordingly, I have already created some code and would be grateful for review and feedback before I submit a pull request.

Important note on the history of my suggested fix The bug came to my attention because of different behavior between SAMD21 and SAMD51 based boards. SAMD51 uses a different clock frequency and different prescaler, which means the bug is present, but does not show up because rollover happens much later. I use(d) the Adafruit fork of the ArduinoCore-samd repository for my projects (using Adafruit Feather M0/M4 boards) which comes with a local/forked version of the servo library. I started out fixing the bug there and only realized later, that there is still no SAMD51 support in the "official" Arduino servo library and the bug is also present upstream (i.e. in the Arduino servo library).

This is why my code "as is" incidentally also introduces SAMD51 support into the servo library (tested on Adafruit Feather M4 Express), in addition to fixing the present bug. If this is too much to swallow at once, I can take out the SAMD51 specific portions of the code and open a separate issue for also including SAMD51 code later. There actually is already an open issue (https://github.com/arduino-libraries/Servo/issues/21) and a matching pull request (https://github.com/arduino-libraries/Servo/pull/34, pending as of today) for adding SAMD51 support, but this code still contains the bug.

Please let me know what you think.