Cleaning up the Servo library's output

Hi all,

Hopefully I am not violating netiquette by putting a few things in one post, but they seem to be interrelated.

Firstly, a BUG (the other things are just enhancements): at line 92 in libraries/Servo/Servo.cpp, the "+ 4" was probably intended to be a "- 4". The effect of the bug: when it is triggered, there will be an unnaturally long delay before the next series of pulses is sent, as the timer does not trigger at REFRESH_INTERVAL as it has already passed it. According to my (maybe wrong) math the timer will overflow at 30 ms, then count up to REFRESH_INTERVAL (20 ms) before starting to send pulses again. So pulses would be delayed by at least 30 ms.

There is another bug explained below having to do with micros().

Now, for the enhancements. With an unmodified Arduino-0018 installation, and a Mega (but assuredly with other boards as well), attaching to a servo and doing a "writeMicroseconds(1500)" will give no better than +/- 4 uS accurate pulses, which do not average to exactly 1500 uS, and which average to different values (1495 to 1505 uS) depending on which pin is being used for output. The core problem is that the compare match timer interrupt which should stop a pulse can sometimes be delayed by other interrupts in the system. I have observed the HardwareSerial.cpp RX interrupt taking as long as 20 microseconds (due to the "%" operator being used instead of a binary operation), although when I captured the scope traces below I couldn't reproduce it, maybe because I wasn't using as much serial buffer.

The fix that I have included involves setting up the compare match to hit some time (e.g. 25 uS or so, depending on how long other interrupts are expected to take) before the expected end of the pulse. Then, when this precursor interrupt hits, the Timer0 overflow interrupt and Serial RX interrupts are temporarily disabled. They are re-enabled after the pulse is turned off. As you can see in the the file "modded.png", this cleans things up pretty nicely (to +/ 1 uS). The traces were each captured over several minutes. Even with my modified codebase there will still be an occassional pulse 2 or 3 uS too long, but they are very rare. The program outputting these pulses is doing a lot of serial communication at 115200 baud. Since we have ~70 uS between characters at 115200 baud, 25 uS is not going to cause us to miss any characters. Higher baud rates would probably suffer. Also, millis() doesn't lose any time (the Timer0 ISR is called right after the pulse ends, if needed).

One other change to Servo.cpp moves the line "*OCRnA = *TCNTn + SERVO(timer,Channel[timer]).ticks;" to inside the "isActive == true" check, so that servos that have been unattached (or created but never attached) will not potentially contribute to a bank of servos receiving updates less frequently than 20 ms. This is probably already generally known, but people who are concerned about getting updates out every 20 ms with lots of servos in use at higher uS values (> 1666 uS on average) should change SERVOS_PER_TIMER in libraries/Servo.cpp to something lower than 12.

To make the pulses be exactly 1500 uS for every pin, I adjusted the TRIM_DURATION constant, and removed the call to turnOffPWM() in digitalWrite(), which takes a variable amount of time depending on the pin. I am not using analogWrite() in my projects. turnOffPWM() could probably be made more consistent by inserting some noop's for certain pins.

These mods are not an immaculate method for a few reasons:

1) In its current form, it will not support more than SERVOS_PER_TIMER servos (one timer's worth, 12). Extending it to multiple timers will require some thought or may not be feasible. Only turning off interrupts for Timer1 servos is an easier option. 2) Since we are disabling the Timer0 OVF interrupt, this breaks micros(). Actually, it just excaccerbates an existing bug. With arduino-0018 stock, if micros() is called within an ISR, the value returned could be 1.024 ms too small, if Timer0 overflows while we are in the ISR but before the micros() call. With my mods, during the 25 uS period when Timer0 OVF is disabled the same thing could happen from the main loop. 3) As written it's only for the Mega, and needs some #defs for other boards. TRIM_DURATION might need to be different. 4) It could interfere with digitalWrite/analogWrite if a pin is used with both. 5) ??? more ???

To fix the micros() bug should probably involve: 1) disable global interrupts 2) manually check for the Timer0 OVF flag 3) if set, add 1024 uS to the result 4) restore SREG

I haven't needed micros() so I haven't made this change. I realize micros() could be used for code performance or other tight measurements, so maybe the above fix is not appropriate for some reason.

Some of the changes in my wiring_digital.diff.c are from a recent svn copy, because of the bug talked about here: (see next post) . I also increased my RX buffer size.

One other thing that I have accounted for in my sketch but not in the linked files is that calling detach() on a servo does not guarantee that the last pulse will be the correct length. In my sketch I do e.g. "while(!digitalRead(pin)); while(digitalRead(pin)); servo1.detach();". That should probably be incorporated into detach() with an optional parameter. For a servo it might not matter (they go crazy anyway) but with Parallax HB-25s it can lead to problems.

Thoughts? BTW Arduino is pretty great!

The other bug:

Stock Servo library:

Modified Servo library:

Diffs between my codebase and stock arduino-0018:

My modded Servo library:

Another thing that may belong in its own thread:

The ATmega1280 documentation, on page 212, says:

"...setting the baud rate... For interrupt driven USART operation, the Global Interrupt Flag should be cleared (and interrupts globally disabled) when doing the initialization."

That seems to be missing in HardwareSerial.cpp, Serial::begin().