[BUG] tone doesn't work after analogWrite

[AlfonsMittelmeyer]

In core version 2.3.0 analogWrite was implemented by a normal timer1 interrupt. In core version 2.4.0 a timer1 NMI interrupt is used (core_esp8266_timer.c). Therefore in tools/sdk/include/ets_sys.h the function ETS_FRC_TIMER1_NMI_INTR_ATTACH(func) exists. The timer1 NMI interrupt may be detached by using a NULL pointer as parameter for this function. But a normal timer1 interrupt via ETS_FRC_TIMER1_INTR_ATTACH(func, arg) and ETS_FRC1_INTR_ENABLE() doesn't work anymore after. Looks like a bug of the espressif sdk.

[devyte]

@igrr do you know anything about this?

[igrr]

Just to clarify, what is the sequence of calls to Arduino functions which triggers this problem?

[AlfonsMittelmeyer]

This is very simple. Just call:

ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL);

After this a normal timer1 interrupt, which is initialized by:

ETS_FRC_TIMER1_INTR_ATTACH(some_timer_isr,NULL);
ETS_FRC1_INTR_ENABLE();

doesn't work anymore

So when I use analogWrite in core version 2.4.1, which uses this NMI interupt now, tone doesn't work anymore. It needs a reset, before tone works again.

[igrr]

ETS_blah aren't Arduino APIs, but analogWrite and tone are. Probably a bug in analogWrite that we don't detach timer from the NMI interrupt. Will check.

On a separate note, it probably makes sense for tone implementation to be merged with analogWrite and also use NMI...

[AlfonsMittelmeyer]

Why is now a timer1 NMI interrupt used in PWM? A NMI interrupt has a reaction time of nearly 10 µs. A normal timer1 interrupt needs only 4 µs. But there needn't be changes done now, because I now have implemented a module timer_shared.cpp:

//=======================================================================
//                               INCLUDES
//=======================================================================
extern "C" {
	#include "timer_shared.h"
}

#include "ets_sys.h"
#include "wiring_private.h"

//=======================================================================
//                               defines
//=======================================================================

#define TIMER_SHARED_MAX 4 // number of shared timers
#define TIMER1_MIN_STEP 10 // limit of ticks. For ticks above or equal a new timer interrupt will be triggered. For tick count below, the timer isr loops.
#define MAX_WAIT_US 30 // maximum wait time until modified timer data should have updated by the timer isr

#define TIMER_NOT_KNOWN 255
#define STOP_TIMER 254
#define ARM_TIMER 253
#define ARM_RELOAD_TIMER 252

//=======================================================================
//                               TYPEDEFS
//=======================================================================

// structure for timer data
typedef struct {
  uint8_t enabled = false; // if disabled, write will not have any effect, only for compatibility with timer1_disable(), because normally timer_shared_stop would be sufficient
  uint8_t no_overflow = false; // allows timer ticks until 0x7FFFFFFF instead only until 0x7FFFFF
  uint8_t divider = 0; // divider expressed by shift: 0 for TIM_DIV1, 4 for TIM_DIV16, 8 for TIM_DIV256
  uint8_t is_reload = 0; // contains TIM_SINGLE or TIM_LOOP
  uint32_t reload = 0; // CPU ticks for reload in case of TIM_LOOP. Because we have now a 31 bit timer counter instead only a 23 bit timer counter, the timer works internally always with TIM_DIV11
  void (*callback)(void *) = NULL; // user isr callback, optionally with additional void * parameters
  void * parameters = NULL; // contains a pointer to optional callback parameters. If paramers == NULL, the user callback has to be of type void callback(), otherwise of type void callback(void * parameters)
} timer_struct ;


//=======================================================================
//                               VARIABLES
//=======================================================================

// timer data for TIMER_SHARED_MAX shared timers
static timer_struct timer_data[TIMER_SHARED_MAX];

// expire times in CPU ticks for the timers or 0 for timer has stopped
// therefore the register CCOUNT is used which is a 32 bit counter for CPU ticks
// the remaining ticks are calculated by expire_counts[TIMER_SHARED_X] - CCOUNT
// if the result is positive (>= TIMER1_MIN_STEP), the hardware timer is triggered for the user callback (for the shared timer with the lowest result)
// if the result is negative (< TIMER1_MIN_STEP) the user callback will be executed within a loop (for the shared timer with the lowest result)
static uint32_t expire_counts[TIMER_SHARED_MAX];

// number of timers, which are active (not stopped)
// if there is only one timer active, the isr performs more quick, because a search (for loop) for the next timer to expire isn't needed
static uint8_t volatile shared_timer_count = 0;

// contains the index of the timer, for which the callback has to be executed, if value < TIMER_SHARED_MAX
// other values are indicators for some jobs to be performed
// TIMER_NOT_KNOWN: the next timer to expire isn't currently known and has to be searched
// other values indicate jobs, which were initiated in non isr context and shall be performed during the timer isr
// STOP_TIMER: a timer shall be stopped (timer_shared_stop_nonisr)
// ARM_TIMER: timer_shared_write_nonisr
static uint8_t volatile timer_index = TIMER_NOT_KNOWN;


// flag whether the interrupt already was initialized. Set by shared_timer_isr_init or shared_timer_nmi_isr_init.
// Used by timer_shared_attachInterrupt or timer_shared_attachInterrupt. These functions call shared_timer_isr_init, if the shared timers are not initialized.
static uint8_t initialized = false;


static struct {
  uint8_t shared_timer;
  uint32_t ticks;
  uint32_t reload;
} volatile isr_job_parameters;



//=======================================================================
//                  function prototypes: local functions
//=======================================================================
static void trigger_isr_job(uint8_t job_id);
static inline uint32_t asm_ccount(void);
static void ICACHE_RAM_ATTR shared_timer_isr(void *para);
static void ICACHE_RAM_ATTR shared_timer_nmi_isr(void);
//=======================================================================
//                  initializing the timer module
//=======================================================================

/* 
 It isn't neccessary to use these routines, because the attach routines call shared_timer_isr_init, if not already initialized.
 But if timer1 of the timer1 module was used, a new initialization of the timer_shared module may be forced again.

 Because the normal timer interrupt performs better (response time about 4 µs) than the nmi timer interrupt (response time about 10 µs), the normal timer interrupt should be used.
 Currently the nmi timer interrupt kills the normal timer interrupt and because of a bug in the sdk there isn't a way to switch back to the normal timer interrupt.
 If a nmi timer interrupt was used stand alone for other purposes, then afterward the shared timers may be used by calling shared_timer_nmi_isr_init, which lets the shared timers run in NMI mode
*/


extern "C" void shared_timer_isr_init(void) {
  // ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); // doesn't work, bug in sdk for core version 2.4.0 and 2.4.1
  ETS_FRC_TIMER1_INTR_ATTACH(shared_timer_isr,NULL);
  ETS_FRC1_INTR_ENABLE();
  T1C = 0x80; // always TIM_DIV1, TIM_EDGE, and TIM_SINGLE is used internally
  TEIE |= TEIE1; // enable the edge interrupt
  initialized = true;
}

extern "C" void shared_timer_nmi_isr_init(void) {
  ETS_FRC_TIMER1_NMI_INTR_ATTACH(shared_timer_nmi_isr);
  T1C = 0x80; // always TIM_DIV1, TIM_EDGE, and TIM_SINGLE is used internally
  TEIE |= TEIE1; // enable the edge interrupt
  initialized = true;
}

//=======================================================================
//                  attaching user isr callback functions
//=======================================================================

/*
  Callback functions may have an optional parameter.  They may be defined with and without an optional parameter.

  For attaching callback functions with parameters, the function timer_shared_attachParamInterrupt exists. Fo example:

  int parameter = 10;
  void callback_with_parameter(void * parameter);

  timer_shared_attachParamInterrupt(TIMER_SHARED_TIMER1,callback_with_parameter,(void *) &parameter);


  For callback functions without parameters the function timer_shared_attachInterrupt exists. For example:
  
  void callback_without_parameter();

  timer_shared_attachInterrupt(TIMER_SHARED_TIMER1,callback_without_parameter);
  
*/


extern "C" void timer_shared_attachParamInterrupt(uint8_t shared_timer, void (* userFunc)(void *), void * parameters ) {

  if(!initialized)
    shared_timer_isr_init();

  if(shared_timer < TIMER_SHARED_MAX) {
    timer_data[shared_timer].callback = userFunc;
    timer_data[shared_timer].parameters = parameters;
  }
}

/*
extern "C" void inline timer_shared_attachInterrupt(uint8_t shared_timer, void (* userFunc)(void) ) {
  timer_shared_attachParamInterrupt(shared_timer, (void (*)(void *)) userFunc, NULL);
}
*/
//=======================================================================
//                  enabling timer interrupts
//=======================================================================

/*
  For parameter compatibility with timer1_enable also the parameter int_type exists. But only TIM_EDGE is used internally.
  TIM_DIV1 (CPU ticks), TIM_DIV16 (CPU ticks/16) and TIM_DIV256 may be used as dividers. Internally the shared timers work only with TIM_DIV1.
  But using the 32 bit counter CCOUNT allows a 31 bit count, which also includes the range of TIM_DIV256.
*/

extern "C" void timer_shared_enable(uint8_t shared_timer, uint8_t divider, uint8_t int_type, uint8_t reload){

  if(shared_timer < TIMER_SHARED_MAX) {
    
    timer_struct * ptr = timer_data + shared_timer;

    switch(divider) {

      // dividers implemented by shift (which is faster than multiplication)
      case TIM_DIV1:
        ptr->divider = 0;
        break;
      case TIM_DIV16:
        ptr->divider = 4;
        break;
      case TIM_DIV256:
        ptr->divider = 8;
        break;
    }
    ptr->is_reload = reload; // flag for reload (TIM_SINGLE or TIM_LOOP)
    ptr->enabled = true; // enabling the timer for write
  }
}

//=======================================================================
//                  stopping, disabling, detaching timer interrupts
//=======================================================================

/*
  Normally stopping a timer would be sufficient. Disabling a timer exists only for compatibility timer1_disable.
  Stopping a timer would simply be done by expire_counts[shared_timer] = 0. But for letting the timer isr perform better in case of only one used shared timer,
  also the number of active timers and the index of the current timer is updated.

  There are two functions availyable for stopping a timer, the functions:

  timer_shared_stop_inisr
  timer_shared_stop_nonisr

  Within a timer isr callback the function timer_shared_stop_inisr
  and in non isr context, the function timer_shared_stop_nonisr has to be used.

  The reason for this is, that a timer interrupt shouldn't happen during changing different timer isr data in non isr context.
  Disabling the interrupt for changing the data isn't also a good idea, because PWM and other time critical routines should run uneffected.
  So in non isr context the data for the timer isr will only be prepared und the timer isr will do the modification afterwards.

*/

extern "C" void ICACHE_RAM_ATTR timer_shared_stop_inisr(uint8_t shared_timer) {

  if(expire_counts[shared_timer]) { // if timer not already stopped
    expire_counts[shared_timer] = 0; // stop timer
    --shared_timer_count; // decrease number of not stopped timers
  }
  if(timer_index == shared_timer) // if the current active timer is identical with this stopped timer
    timer_index = TIMER_NOT_KNOWN; // the next timer to expire has to be searched
}

extern "C" void timer_shared_stop_nonisr(uint8_t shared_timer) {
  isr_job_parameters.shared_timer = shared_timer;
  trigger_isr_job(STOP_TIMER);
}

static void trigger_isr_job(uint8_t job_id) {
  timer_index = job_id;
  T1L = 1;
  uint32_t start = micros();
  while(timer_index == job_id && micros()-start < MAX_WAIT_US);
}

/*
  Disabling a timer isn't normaly done during the timer interrupt, so one function is sufficient
*/

extern "C" void timer_shared_disable_nonisr(uint8_t shared_timer) {
  timer_data[shared_timer].enabled = false;
  timer_shared_stop_nonisr(shared_timer);
}

/*
  Detaching the callback is really not needed. For compatibility with timer1_detachInterrupt, we simply use timer_shared_disable_nonisr
*/

//=======================================================================
//                  timer reload
//=======================================================================

/*
  If a timer is running in mode TIM_LOOP, which is a good idea for tone, the frequency may be changed easily by changing the number of ticks to be reloaded.
  The change takes effect, when the next reload cycle begins.
*/

extern "C" void timer_shared_reload(uint8_t shared_timer,uint32_t ticks) {
  timer_data[shared_timer].reload = ticks << timer_data[shared_timer].divider;
}

//=======================================================================
//                               CCOUNT
//=======================================================================

// the heart of timer_shared: the 32 bit CPU tick counter
static inline uint32_t asm_ccount(void) {
    uint32_t r;
    asm volatile ("rsr %0, ccount" : "=r"(r));
    return r;
}

//=======================================================================
//                     timer read, enabled, interrupted
//=======================================================================

// returns the remaining ticks considering the divider
extern "C" uint32_t timer_shared_read(uint8_t shared_timer) {

  uint32_t value = expire_counts[shared_timer];
  if(value) {
    value -= asm_ccount(); // remaining ticks
    if(value & 0x80000000) // if negative
      value=0;
    else
      value >>= timer_data[shared_timer].divider;
  }
  return value;
}

extern "C" uint8_t timer_shared_enabled(uint8_t shared_timer) {
  return timer_data[shared_timer].enabled;
}

// for timer1_interrupted() there isn't a replacement, because we use TIM_EDGE

//=======================================================================
//                               timer write
//=======================================================================


extern "C" void ICACHE_RAM_ATTR timer_shared_write_inisr(uint8_t shared_timer , uint32_t ticks){

  uint32_t ccount = asm_ccount(); // current time in CPU ticks
  timer_struct * ptr = timer_data + shared_timer;

  if(ptr->enabled) { // write only shall work, if the timer is enabled, for compatibility with timer1_disable()

    if(!ticks) { // if ticks == 0, stop the timer
      timer_shared_stop_inisr(shared_timer);
      return;
    }

    uint32_t counts = expire_counts[shared_timer];
    if(!counts) // if the timer was stopped
      ++shared_timer_count; // then there is now an active timer more (needed for quick timer isr in case of only one active timer)

    uint32_t div1_ticks = ticks << ptr->divider; // conversion of timer ticks to TIM_DIV1 timer ticks
    if(ptr->is_reload) // in case of TIM_LOOP, save the timer ticks for reload
      ptr->reload = div1_ticks;

    counts = ccount + div1_ticks; // expire time in CPU ticks
    if(!counts) // in case of 0, which marks a stopped timer
      --counts; // change to not equal 0
    expire_counts[shared_timer] = counts; // set expire time

    timer_index = shared_timer; // timer for the case of only one timer
  }
}

extern "C" void timer_shared_write_nonisr(uint8_t shared_timer , uint32_t ticks){
  isr_job_parameters.shared_timer = shared_timer;
  isr_job_parameters.ticks = ticks;
  trigger_isr_job(ARM_TIMER);
}


// special function, for executing the callback as soon as possible in case of TIM_LOOP
/*

  for example if an LED shall blink in a rhythm of one second and this shall begin as soon as possible, then for example call:
  timer_shared_write_reload_nonisr(TIMER_SHARED_TIMER1,1,80000000);
  ticks are the ticks until the callback will be called the first time, reload ticks are the ticks for further callback calls
*/

extern "C" void timer_shared_write_reload_nonisr(uint8_t shared_timer , uint32_t ticks, uint32_t reload_ticks){
  isr_job_parameters.shared_timer = shared_timer;
  isr_job_parameters.ticks = ticks;
  isr_job_parameters.reload = reload_ticks;
  trigger_isr_job(ARM_RELOAD_TIMER);
}

//=======================================================================
//                     timer ISR routines
//=======================================================================


// ISR with parameter for non NMI interrupt
static void ICACHE_RAM_ATTR shared_timer_isr(void *para) {

  uint8_t index = timer_index; // flag for job or index for the shared timer, which shall be executed
  
  while(true) { // loop for ticks below TIMER1_MIN_STEP

    if(index < TIMER_SHARED_MAX) { // if it's a valit timer index

      if(expire_counts[index]) { // and if the timer isn't stopped

        timer_struct * ptr = timer_data + index;
      
        if(ptr->no_overflow) { // if no ticks above 23 bit range are left, then the timer has expired and the callback has to be executed
  
          if(ptr->reload) { // if the timer is of type TIM_LOOP
            expire_counts[index] += ptr->reload; // then the next expire time has to be set
            ptr->no_overflow = false; // initializing with values also above 23 bit range
            if(!expire_counts[index]) // if the expire time is 0, which is the mark for a stopped timmer
              --expire_counts[index]; // set it to not equal 0
          }
          else { // in  case of TIM_SINGLE
            expire_counts[index] = 0; // stop the timer
            --shared_timer_count;     // update number of active timers: there is one timer less now
            timer_index = TIMER_NOT_KNOWN;        // set flag for searching the next timer to expire
          }
      
          // call the callback function
          if(ptr->parameters) { // if it has a parameter
            ptr->callback(ptr->parameters); // call it with parameter
          }
          else
            ((void (*)()) (ptr->callback))(); // otherwise call it without parameter
      
          index = timer_index; // update the index after callback execution or after job done
        }
      }
    } else { // if it isn't a valid timer index (job to be done or simply an unknown timer index - TIMER_NOT_KNOWN)
      switch(index) {
        case STOP_TIMER: // stop_timer
          timer_shared_stop_inisr(isr_job_parameters.shared_timer);
          index = timer_index; // update the index after callback execution or after job done
          break;
        case ARM_TIMER:
          timer_shared_write_inisr(isr_job_parameters.shared_timer,isr_job_parameters.ticks);
          index = timer_index; // update the index after callback execution or after job done
          break;
        case ARM_RELOAD_TIMER:
          timer_shared_write_inisr(isr_job_parameters.shared_timer,isr_job_parameters.ticks);
          timer_data[isr_job_parameters.shared_timer].reload = isr_job_parameters.reload << timer_data[isr_job_parameters.shared_timer].divider;
          index = timer_index; // update the index after callback execution or after job done
          break;
      }
    }
    
    // if no timer active, then exit
    uint8_t timers = shared_timer_count;
    if(!timers) {
      return;
    }

    // next timer to expire and remaining timer ticks
    int32_t min_count; // remaining timer ticks
    uint32_t current_count = asm_ccount(); // current time in CPU ticks
  
    // in case of only one active timer
    if(timers == 1 && index < TIMER_SHARED_MAX) {
      min_count = expire_counts[index] - current_count; // remaining ticks are expire time - current time
    }
    else { // otherwise a search is done over all timers

      int min_index=0; // index for the next timer to expire
      uint32_t count; // expire time of the processed timer
      timers = 0; // start value for counting active timers
      min_count = 0x7FFFFFFF; // start value for the next timer to expire, search for minimum value
      index = TIMER_NOT_KNOWN; // start index for timer to search

      for(int i = 0; i < TIMER_SHARED_MAX ; ++i) { // loop over all timers
        count = expire_counts[i]; // expire time
        
        if(count) { // it timer is active
          ++timers; // increase number of active timers
          if((int32_t) (count - current_count) < min_count) { // if remaining ticks below remaining ticks of timers before
            index = i; // update index for next timer to expire
            min_count = count - current_count; // update remaining ticks
          }
        }
      }

     shared_timer_count = timers; // update active timer count
     timer_index = index; // and next timer to expire
    }
  
    // whether the timer was searched or whether there is only one active timer
    if(min_count > 0x7FFFFF) { // if the remaining ticks are more than a 23 bit value
      min_count = 0x7FFFFF; // take only the maximum 23 bit value
      timer_data[index].no_overflow = false; // and set the flag, for remaining ticks not complete handled
    }
    else {
      timer_data[index].no_overflow = true; // otherwise set the flag for calling the callback function after the hardware timer has expired
    }
  
    min_count -= asm_ccount() - current_count; // recalculate the remaining ticks once more
    if(min_count >= TIMER1_MIN_STEP) { // if ticks not below loswer limit
      T1L = min_count; // trigger the next timer interrupt, otherwise a loop is performed.
      return;
    }
  }
}


// ISR without parameter for NMI interrupt

static void ICACHE_RAM_ATTR shared_timer_nmi_isr(void) {
  shared_timer_isr(NULL);
}

//=======================================================================
//                              EOF
//=======================================================================

And this is the header file timer_shared.h:

#include "c_types.h"
//=======================================================================
//                        defines 
//=======================================================================

#define TIMER_SHARED_PWM 0
#define TIMER_SHARED_TONE 1
#define TIMER_SHARED_TIMER1 2
#define TIMER_SHARED_USR 3

//=======================================================================
//                  function prototypes: interface functions
//=======================================================================

void shared_timer_isr_init(void);
void shared_timer_nmi_isr_init(void);

void timer_shared_attachParamInterrupt(uint8_t shared_timer, void (* userFunc)(void *), void * parameters );
#define timer_shared_attachInterrupt(x,y) timer_shared_attachParamInterrupt( (x),(void (*)(void *)) (y),NULL)

void timer_shared_enable(uint8_t shared_timer, uint8_t divider, uint8_t int_type, uint8_t reload);

void ICACHE_RAM_ATTR timer_shared_stop_inisr(uint8_t shared_timer);
void timer_shared_stop_nonisr(uint8_t shared_timer);
void timer_shared_disable_nonisr(uint8_t shared_timer);
#define timer_shared_detachInterrupt timer_shared_disable_nonisr

void ICACHE_RAM_ATTR timer_shared_write_inisr(uint8_t shared_timer , uint32_t ticks);
void timer_shared_write_nonisr(uint8_t shared_timer , uint32_t ticks);
void timer_shared_reload(uint8_t shared_timer,uint32_t ticks);
void timer_shared_write_reload_nonisr(uint8_t shared_timer , uint32_t ticks, uint32_t reload_ticks);

uint32_t timer_shared_read(uint8_t shared_timer);
uint8_t timer_shared_enabled(uint8_t shared_timer);

After using these timer interface functions in core_esp8266_wiring_pwm.c:

/*
  pwm.c - analogWrite implementation for esp8266

  Copyright (c) 2015 Hristo Gochkov. All rights reserved.
  This file is part of the esp8266 core for Arduino environment.

  This library is free software; you can redistribute it and/or
  modify it under the terms of the GNU Lesser General Public
  License as published by the Free Software Foundation; either
  version 2.1 of the License, or (at your option) any later version.

  This library is distributed in the hope that it will be useful,
  but WITHOUT ANY WARRANTY; without even the implied warranty of
  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  Lesser General Public License for more details.

  You should have received a copy of the GNU Lesser General Public
  License along with this library; if not, write to the Free Software
  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
*/
#include "wiring_private.h"
#include "pins_arduino.h"
#include "c_types.h"
#include "eagle_soc.h"
#include "ets_sys.h"
#include "timer_shared.h"

#ifndef F_CPU
#define F_CPU 800000000L
#endif

struct pwm_isr_table {
    uint8_t len;
    uint16_t steps[17];
    uint32_t masks[17];
};

struct pwm_isr_data {
    struct pwm_isr_table tables[2];
    uint8_t active;//0 or 1, which table is active in ISR
};

static struct pwm_isr_data _pwm_isr_data;

uint32_t pwm_mask = 0;
uint16_t pwm_values[17] = {0,};
uint32_t pwm_freq = 1000;
uint32_t pwm_range = PWMRANGE;

uint8_t pwm_steps_changed = 0;
uint32_t pwm_multiplier = 0;

int pwm_sort_array(uint16_t a[], uint16_t al)
{
    uint16_t i, j;
    for (i = 1; i < al; i++) {
        uint16_t tmp = a[i];
        for (j = i; j >= 1 && tmp < a[j-1]; j--) {
            a[j] = a[j-1];
        }
        a[j] = tmp;
    }
    int bl = 1;
    for(i = 1; i < al; i++) {
        if(a[i] != a[i-1]) {
            a[bl++] = a[i];
        }
    }
    return bl;
}

uint32_t pwm_get_mask(uint16_t value)
{
    uint32_t mask = 0;
    int i;
    for(i=0; i<17; i++) {
        if((pwm_mask & (1 << i)) != 0 && pwm_values[i] == value) {
            mask |= (1 << i);
        }
    }
    return mask;
}

void prep_pwm_steps()
{
    if(pwm_mask == 0) {
        return;
    }

    int pwm_temp_steps_len = 0;
    uint16_t pwm_temp_steps[17];
    uint32_t pwm_temp_masks[17];
    uint32_t range = pwm_range;

    if((F_CPU / ESP8266_CLOCK) == 1) {
        range /= 2;
    }

    int i;
    for(i=0; i<17; i++) {
        if((pwm_mask & (1 << i)) != 0 && pwm_values[i] != 0) {
            pwm_temp_steps[pwm_temp_steps_len++] = pwm_values[i];
        }
    }
    pwm_temp_steps[pwm_temp_steps_len++] = range;
    pwm_temp_steps_len = pwm_sort_array(pwm_temp_steps, pwm_temp_steps_len) - 1;
    for(i=0; i<pwm_temp_steps_len; i++) {
        pwm_temp_masks[i] = pwm_get_mask(pwm_temp_steps[i]);
    }
    for(i=pwm_temp_steps_len; i>0; i--) {
        pwm_temp_steps[i] = pwm_temp_steps[i] - pwm_temp_steps[i-1];
    }

    pwm_steps_changed = 0;
    struct pwm_isr_table *table = &(_pwm_isr_data.tables[!_pwm_isr_data.active]);
    table->len = pwm_temp_steps_len;
    ets_memcpy(table->steps, pwm_temp_steps, (pwm_temp_steps_len + 1) * 2);
    ets_memcpy(table->masks, pwm_temp_masks, pwm_temp_steps_len * 4);
    pwm_multiplier = ESP8266_CLOCK/(range * pwm_freq);
    pwm_steps_changed = 1;
}

void ICACHE_RAM_ATTR pwm_timer_isr() //103-138
{
    struct pwm_isr_table *table = &(_pwm_isr_data.tables[_pwm_isr_data.active]);
    static uint8_t current_step = 0;
    if(current_step < table->len) { //20/21
        uint32_t mask = table->masks[current_step] & pwm_mask;
        if(mask & 0xFFFF) {
            GPOC = mask & 0xFFFF;    //15/21
        }
        if(mask & 0x10000) {
            GP16O = 0;    //6/13
        }
        current_step++;//1
    } else {
        current_step = 0;//1
        if(pwm_mask == 0) { //12
            table->len = 0;
            return;
        }
        if(pwm_mask & 0xFFFF) {
            GPOS = pwm_mask & 0xFFFF;    //11
        }
        if(pwm_mask & 0x10000) {
            GP16O = 1;    //5/13
        }
        if(pwm_steps_changed) { //12/21
            _pwm_isr_data.active = !_pwm_isr_data.active;
            table = &(_pwm_isr_data.tables[_pwm_isr_data.active]);
            pwm_steps_changed = 0;
        }
    }
	timer_shared_write_inisr(TIMER_SHARED_PWM,table->steps[current_step] * pwm_multiplier);
}

void pwm_start_timer()
{
	timer_shared_attachInterrupt(TIMER_SHARED_PWM,pwm_timer_isr);
	timer_shared_enable(TIMER_SHARED_PWM,TIM_DIV1, TIM_EDGE, TIM_SINGLE);
	timer_shared_write_nonisr(TIMER_SHARED_PWM,1);
}

void ICACHE_RAM_ATTR pwm_stop_pin(uint8_t pin)
{
    if(pwm_mask){
        pwm_mask &= ~(1 << pin);
        if(pwm_mask == 0) {
			timer_shared_stop_nonisr(TIMER_SHARED_PWM);
        }
    }
}

extern void __analogWrite(uint8_t pin, int value)
{
    bool start_timer = false;
    if(value == 0) {
        digitalWrite(pin, LOW);
        prep_pwm_steps();
        return;
    }
    if((pwm_mask & (1 << pin)) == 0) {
        if(pwm_mask == 0) {
            memset(&_pwm_isr_data, 0, sizeof(_pwm_isr_data));
            start_timer = true;
        }
        pinMode(pin, OUTPUT);
        digitalWrite(pin, LOW);
        pwm_mask |= (1 << pin);
    }
    if((F_CPU / ESP8266_CLOCK) == 1) {
        value = (value+1) / 2;
    }
    pwm_values[pin] = value % (pwm_range + 1);
    prep_pwm_steps();
    if(start_timer) {
        pwm_start_timer();
    }
}

extern void __analogWriteFreq(uint32_t freq)
{
    pwm_freq = freq;
    prep_pwm_steps();
}

extern void __analogWriteRange(uint32_t range)
{
    pwm_range = range;
    prep_pwm_steps();
}

extern void analogWrite(uint8_t pin, int val) __attribute__ ((weak, alias("__analogWrite")));
extern void analogWriteFreq(uint32_t freq) __attribute__ ((weak, alias("__analogWriteFreq")));
extern void analogWriteRange(uint32_t range) __attribute__ ((weak, alias("__analogWriteRange")));

And in Tone.cpp:

/*
  Tone.cpp

  A Tone Generator Library for the ESP8266

  Copyright (c) 2016 Ben Pirt. All rights reserved.
  This file is part of the esp8266 core for Arduino environment.

  This library is free software; you can redistribute it and/or
  modify it under the terms of the GNU Lesser General Public
  License as published by the Free Software Foundation; either
  version 2.1 of the License, or (at your option) any later version.

  This library is distributed in the hope that it will be useful,
  but WITHOUT ANY WARRANTY; without even the implied warranty of
  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  Lesser General Public License for more details.

  You should have received a copy of the GNU Lesser General Public
  License along with this library; if not, write to the Free Software
  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
*/

#include "Arduino.h"
#include "pins_arduino.h"
extern "C" {
#include "timer_shared.h"
}
#define AVAILABLE_TONE_PINS 1
const uint8_t tone_timers[] = { 1 };
static uint8_t tone_pins[AVAILABLE_TONE_PINS] = { 255, };
static long toggle_counts[AVAILABLE_TONE_PINS] = { 0, };
#define T1INDEX 0

void t1IntHandler();

static int8_t toneBegin(uint8_t _pin) {
  int8_t _index = -1;

  // if we're already using the pin, reuse it.
  for (int i = 0; i < AVAILABLE_TONE_PINS; i++) {
    if (tone_pins[i] == _pin) {
      return i;
    }
  }

  // search for an unused timer.
  for (int i = 0; i < AVAILABLE_TONE_PINS; i++) {
    if (tone_pins[i] == 255) {
      tone_pins[i] = _pin;
      _index = i;
      break;
    }
  }

  return _index;
}

// frequency (in hertz) and duration (in milliseconds).
void tone(uint8_t _pin, unsigned int frequency, unsigned long duration) {
  int8_t _index;

  _index = toneBegin(_pin);

  if (_index >= 0) {
    // Set the pinMode as OUTPUT
    pinMode(_pin, OUTPUT);

    // Alternate handling of zero freqency to avoid divide by zero errors
    if (frequency == 0)
    {
        noTone(_pin);
        return;
    }

    // Calculate the toggle count
    if (duration > 0) {
      toggle_counts[_index] = 2 * frequency * duration / 1000;
    } else {
      toggle_counts[_index] = -1;
    }

    // set up the interrupt frequency
    switch (tone_timers[_index]) {
      case 0:
        // Not currently supported
        break;

      case 1:
		timer_shared_attachInterrupt(TIMER_SHARED_TONE,t1IntHandler);
		timer_shared_enable(TIMER_SHARED_TONE,TIM_DIV1, TIM_EDGE, TIM_LOOP);
		timer_shared_write_nonisr(TIMER_SHARED_TONE,(clockCyclesPerMicrosecond() * 500000) / frequency);
        break;
    }
  }
}

void disableTimer(uint8_t _index) {
  tone_pins[_index] = 255;

  switch (tone_timers[_index]) {
    case 0:
      // Not currently supported
      break;

    case 1:
	  timer_shared_disable_nonisr(TIMER_SHARED_TONE);
      break;
  }
}

void noTone(uint8_t _pin) {
  for (int i = 0; i < AVAILABLE_TONE_PINS; i++) {
    if (tone_pins[i] == _pin) {
      tone_pins[i] = 255;
      disableTimer(i);
      break;
    }
  }
  digitalWrite(_pin, LOW);
}

ICACHE_RAM_ATTR void t1IntHandler() {
  if (toggle_counts[T1INDEX] != 0){
    // toggle the pin
 
    digitalWrite(tone_pins[T1INDEX], toggle_counts[T1INDEX] % 2);
 
    toggle_counts[T1INDEX]--;
    // handle the case of indefinite duration
    if (toggle_counts[T1INDEX] < -2){
      toggle_counts[T1INDEX] = -1;
    }
  }else{
    disableTimer(T1INDEX);
    digitalWrite(tone_pins[T1INDEX], LOW);
  }
}

Tone and PWM may work both at the same time!!!

[AlfonsMittelmeyer]

Besides of this, tone should be more optimized. Tone uses digitalWrite. digitalWrite calls pwm_stop_pin and so needs 69 CPU ticks. Without pwm_stop_pin, it's 37 CPU ticks as ICACHE_RAM_ATTR and only 30 CPU ticks otherwise. If we make two functions, one for pins except D0 and one for D0, this only needs 10 CPU ticks. I didn't change this, but only inserted my timer_shared interface.

[igrr]

analogWrite uses NMI in order to produce correct waveform even when interrupts are disabled (via INTLEVEL or INTENABLE).

[AlfonsMittelmeyer]

Yes, this is a reason, but timer1 cannot be used thereafter anymore, because there is a bug in the SDK for ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL). And for small PWM values below 10 the NMI timer1 interrupt isn't accurate enough. But if somebody disables the interrupts, an NMI interrupt would be better. For an NMI interrupt for my shared_timer module additionally shared_timer_nmi_isr_init() should be called in the tone and pwm module.

Or the user may decide self, whether he wants NMI or not. If he calls shared_timer_nmi_isr_init(), then timer_shared runs as NMI timer1 interrupt.

[igrr]

but timer1 cannot be used thereafter anymore

Thank you, this is the bug you just reported. I'll have a look :)

And for small PWM values below 10 the NMI timer1 interrupt isn't accurate enough.

That is true. I think for low PWM values we can optimize the PWM implementation to busy-loop for a few microseconds, instead of setting up a timer.

[AlfonsMittelmeyer]

Before implementation there should be thoughts. Why would a user want to disable Interrupts? Could be because he had implemented an own ISR yand wants to change values for the ISR. If the user has implemented an own ISR, he wants, that it performs well and doesn't want some busy loop within another ISR. Should the user use noInterrupts() and interupts(). Measure the execution time. This isn't a good idea. But uint32_t savedPS = xt_rsil(15) and xt_wsr_ps(savedPS) each need only 9 ticks. But stopping interrupts isn't necessary. Parameters may be prepared for the interrupt, and when the interrupt is triggered, it may take them over. When the user knows this, then there isn't a reason for NMI timer interrupts in PWM. And a busy loop will also have negative consequences for other functionalities like pulseIn() and who knows, what else. The problem is, when many people or departments develop a software without enough consultation, and many people have such ideas as different busy loops, the system will not perform well. Maybe this is also a problem of the SDK. Or how can somebody explain 358 ticks for disabling a normal timer1 isr ETS_FRC1_INTR_DISABLE(); // 358 ticks?

[devyte]

It's not necessarily the user who has disabled the interrupts, it could be e.g. he's calling an api with timing critical code under the hood, like OneWire, where the interrupts must be disabled for the generation of the protocol pulses, or it won't work.

[igrr]

Besides, even if user is not disabling interrupts, some interrupt (like WiFi interrupt) might be consuming (some) CPU time. Since both wifi and timer interrupts are level 1 interrupts, one can not preempt the other, so PWM output will be disturbed when there is a lot of wifi traffic. Attaching the timer to a different interrupt (which is above level 1) is done to allow preemption and get less jitter when WiFi is handling traffic.

[AlfonsMittelmeyer]

@igrr I didn't measure a difference between NMI and non NMI timer ISR. But there is a bug in PWM. What would you think about values near maximum?

For a value of 1020 I measured these times in microseconds via pulseIn:

1000, 1998, 1001, 4007, 3006, 1001, 1001, 1001, 3000, 1998

Seems to be a lot of jitter, but of course, it shouldn't matter much, whether we have a full cycle without switchung some microseconds to LOW.

Another question is, why is it so important, that we don't have some jitter and what about the accuracy of pulseIn? Could also be, that pulseIn jitters. So we don't know, whether jitter measured by pulseIn is jitter of the signal or of pulseIn. Of course the values above are not jitter of pulseIn, but a missing LOW for values near maximum.

No, this also could be jitter of pulseIn: if the flank is too short, pulseIn maybe doesn't catch it.

[devyte]

@AlfonsMittelmeyer you probably should be aware that there is reported flickering in the current pwm implementation, reference: #836 #2592 #2621 #2675 . There are reports of a better implementation for pwm ( see discussion in #836 ), but that particular implementation can't be integrated here due to licensing incompatibility.
I mention this, because maybe some ideas from there are useful for your work here. I haven't looked at either code in detail, so I can't really tell myself.

[AlfonsMittelmeyer]

@devyte I noticed such flickering, when I tested with blinking for cycles below 300 ticks when using non NMI timer isr and for cycles below 800 ticks when using NMI timer isr. We need to find out about the reason.

@igrr Because I didn't notice a difference between non NMI timer isr and NMI timer isr for PWM in the case of Wifi not running, there seems to be no reason, why we shouldn't use NMI. And my opinion now is also, that NMI should be used for PWM.

Of course, measuring via pulseIn produces jitter, if there is an ISR running. We cannot have PWM and PulseIn performing well at the same time, when using only one ESP8266. For exact measurements timer0 could be used in NMI mode and timer1 in non NMI mode for PWM, in the case, if some jitter would be acceptable. What the priorities are, depend what the user wants to do. I would think, that the best way would be, if the user could configure the priorities for his application.

I will set NMI mode as default for my timer_shared module und I will do measurements about ticks for implementing exact time spans. There are some questions to be answered like how many ticks does it take from timer becoming 0 until the timer isr called and some further similar questions.

[AlfonsMittelmeyer]

@igrr currently the time spans for small PWM values are too long, because it wasn't considered, that it takes time until the ISR is called. I made some tests by using this sketch:

volatile uint32_t ccount2 = 0;
volatile bool isr_ready = false;

static void ICACHE_RAM_ATTR timer_nmi_isr(void) {
  ccount2 = asm_ccount();
  isr_ready = true;
}

static void ICACHE_RAM_ATTR timer_isr(void * para) {
  ccount2 = asm_ccount();
  isr_ready = true;
}

static inline uint32_t asm_ccount(void) {
    uint32_t r;
    asm volatile ("rsr %0, ccount" : "=r"(r));
    return r;
}


void setup() {
  Serial.begin(115200);
  ETS_FRC_TIMER1_NMI_INTR_ATTACH(timer_nmi_isr);
  //ETS_FRC_TIMER1_INTR_ATTACH(timer_isr,NULL);
  //ETS_FRC1_INTR_ENABLE();
  T1C = 0x80;
  TEIE |= TEIE1;
}

void loop() {
  if(Serial.available()) {

    while(Serial.available())
      Serial.read();

     isr_ready = false;
     uint32_t ccount0 = asm_ccount();
     T1L = 1;
     uint32_t ccount1 = asm_ccount();

     while(!isr_ready);
     Serial.print(ccount1-ccount0);
     Serial.print(" ");
     Serial.println(ccount2-ccount0);
  }
}

This is the result for non NMI isr for 80 MHz CPU clock:

11 121

1 tick for calling asm_ccount(), 10 ticks for calling T1L. Normally the code doesn't have asm_ccount(), so swe have to subtract 2 ticks for two times asm_ccount and one tick, for value 1 used in T1L = 1.

This means, that we have a delay of 118 ticks, when using non NMI isr.

The results for NMI isr were:

325 447
11 133
11 133
11 133
11 133
11 133

When T1L is used the first time for a NMI isr, it has an additional delay of 314 ticks. But this shouldn't matter. The delay for NMI isr is 130 ticks instead of 118 ticks for non NMI or 119 ticks, if we disable other interrupts via uint32_t savedPS = xt_rsil(15;

This behaviour I will consider for the tick calculation in my timer shared module. For smaller values an idle loop within the isr would make sense.

Most probably the delay for 160 MHz CPU clock in ticks will be different, because not every part of the ESP8266 will become more fast. Storing a value into a variable in the RAM costs 8 ticks, when using 80 MHz, how many ticks will this be for 160 MHz?

[AlfonsMittelmeyer]

NMI and not NMI isr shall have the same timing. This we may do by:

volatile uint32_t time_consuming = 0;

static void ICACHE_RAM_ATTR timer_isr(void * para) {
  uint32_t savedPS = xt_rsil(15);
  time_consuming = asm_ccount() + 1; // consuming 11 ticks (80 MHz CPU clock) , so that the timing is the same as for NMI isr
  timer_nmi_isr();
  xt_wsr_ps(savedPS);
}

[igrr]

@AlfonsMittelmeyer I see you are sharing valuable progress here, but i ask you to keep discussion in issues on topic, for everyone's sanity. Specifically, this one tracks the bug in SDK that Timer can not be used with non-NMI interrupt once NMI has been used. For other issues (such as which interrupt to use, NMI vs non-NMI, and so on), please open separate tickets.

FWIW, I have reported the issue about NmiTimSetFunc to ESP8266 SDK maintainers, so we now wait for an update there.

[earlephilhower]

@AlfonsMittelmeyer we've released 2.4.2 which doesn't use NMI and which lets tone/analogWrite run in any order or at the same time. So this specific bug is fixed as far as I understand the initial problem.

I know you had a good start on optimizing things, so if you have a PR to submit that helps bring down jitter using some of the techniques you were discussing, we'd love to see it!