bug in SAMD implementation, SAMD51 support

[agrommek]

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".

• Use a larger clock prescaler, e.g. 64 instead of 16 (note: there is no prescaler with value 32 on TC instances...). This would cause the clock to run slower. The wraparound would occur much later (i.e. not at all, realistically). However, the resolution would be decreased unnecessarily. "Consequence B" would be fixed, "consequence A" would still be present.
• Decrease the number of maximum possible servos for SAMD. If we want to be able to handle up to 2400 μs per servo, we can squeeze up to eight back-to-back pulses into our 20 ms period. However, the 2400 μs limit is not that hard. Users could easily use even longer pulses (probably not healthy for their servos, but who knows what users will do...). To be safe, one should only allow seven or maybe even only six servos. In my opinion, this would be a shame. Users often use ARM Cortex-M based boards because the higher performance is needed for more complex projects. It would be sad if those boards supported much fewer servos than the "good old AVR boards".
• Use both Capture/Compare registers of the TC instance. Right now, only one CC register is used. Every CC register can safely take care of only six servos, giving 12 servos per TC instance. However, to coordinate to CC registers with a single counter, a bit of book-keeping is necessary. This causes some code refactoring.

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 (#21) and a matching pull request (#34, pending as of today) for adding SAMD51 support, but this code still contains the bug.

Please let me know what you think.