Bugg in servo library with detach(), after can't use analogWrite on pin 9 or 10

[agdl]

From @JLP69 on September 24, 2015 15:31

I'm using the Servo library, and I've finding that detach() method doesn't work as in the reference. I've reading some post about it. It seems that the timers aren't reseted by this method for using PWM output on the pins 9 and 10.
And I've found something in the library code to be changed.
In the Servo.cpp, in the finISR function (that is used for disable the timer) there is no code for doing that for Arduino board.

#else
//For arduino - in future: call here to a currently undefined function to reset the timer
TCCR1B = 0;
TCCR1B=_BV(CS11);
TCCR1B=_BV(CS10);
TCCR1A=_BV(WGM10);
TIMSK1=0;
#endif

After few trials, it seems working. I think it may be included in source code.

Copied from original issue: arduino/Arduino#3860

[agdl]

From @michaelmargolis on November 9, 2015 10:48

The suggested code will not work as expected on boards that have more than one 16 bit timer, such as the mega.

When I submitted this library, I suggested functions added to the core to disable timers, similar to the way the wiring project handles this. That way, any library that needed to restore a timer to it default state would be able to call the same function used by the core when the runtime is initialized.
That would insulate the library from any future changes to the timer initialization in the core.

In the absence of that, you could add the timer init code into the if(timer== ... statements with the #if defined WIRING moved to surround each timerDetach call.

Michael Margolis

[agdl]

From @JLP69 on November 9, 2015 15:54

Thanks for your answer,

But why couldn’t we use a code like this for applying this fix only for the Uno board :

static void finISR(timer16_Sequence_t timer)
{
  //disable use of the given timer
#if defined WIRING   // Wiring
  if (timer == _timer1) {
#if defined(__AVR_ATmega1281__)||defined(__AVR_ATmega2561__)
    TIMSK1 &=  ~_BV(OCIE1A) ;  // disable timer 1 output compare interrupt
#else
    TIMSK &=  ~_BV(OCIE1A) ;  // disable timer 1 output compare interrupt
#endif
    timerDetach(TIMER1OUTCOMPAREA_INT);
  }
  else if (timer == _timer3) {
#if defined(__AVR_ATmega1281__)||defined(__AVR_ATmega2561__)
    TIMSK3 &= ~_BV(OCIE3A);    // disable the timer3 output compare A interrupt
#else
    ETIMSK &= ~_BV(OCIE3A);    // disable the timer3 output compare A interrupt
#endif
    timerDetach(TIMER3OUTCOMPAREA_INT);
  }
#elif defined(__AVR_ATmega328P__)
  //For Arduino Uno
  TCCR1B = 0;
  TCCR1B = _BV(CS11);
  TCCR1B = _BV(CS10);
  TCCR1A = _BV(WGM10);
  TIMSK1 = 0;
#else
  //For other arduino - in future: call here to a currently undefined function to reset the timer
#endif
}

Jean-Luc PELLIGOTTI

[agdl]

From @michaelmargolis on November 9, 2015 20:16

The issue would still exist for users of other boards and would re-occur for all AVR boards if the timer init code in the core was changed without the library also be modified.

If restoring timers to the initial state is considered important then I would hope that that a clean solution would be implemented. In my view, that involves calling a function in the core to set a timer to the initial state. That way, the library code will not go out of sync if the core timer init code is changed.
But given that the library is over six years old and this issue has just come to the fore, perhaps it will be sufficient for now to modify the documentation until resources are available to do this propelry.

[agdl]

From @Cration on January 15, 2016 4:29

It should be like this:

TCCR1B =_BV(CS11) | _BV(CS10);
TCCR1A =_BV(WGM10);
TIMSK1 = 0;

[agdl]

From @michaelmargolis on January 15, 2016 10:59

For reasons stated above, it is not good practice for the servo library to make assumptions about how the core initializes timers at startup. If the current core timer initialization code was moved into separate functions, the servo library could call the appropriate initialization function in its detach method.

If you think this needs addressing, perhaps you could raise an issue to modify wiring.c to add something similar to Wiring's timerAttach and timerDetach methods to handle the initialization of timers. Stubs are already in the servo detach code to call a method to reinitialize a timer if one was available in wiring.c.

[agdl]

From @matthijskooijman on January 15, 2016 13:38

FWIW, I agree with @michaelmargolis here. To fix this, a proper API should be added to the core, instead of hardcoding things in the servo library. Making this API portable might be tricky, though it does not need to be portable to other architectures, only boards, I think.