analogWrite very incorrect

[AlfonsMittelmeyer]

I measured analogWrite for small and also other values.

There is this big bug in the algorithm, that always for two consecutive values the duration is equal.

This I measured:

 1:  3.78 µs, 302 ticks
 2:  3.78 µs, 302 ticks
 3:  5.72 µs, 458 ticks
 4:  5.71 µs, 457 ticks
 5:  7.68 µs, 614 ticks
 6:  7.68 µs, 614 ticks
 7:  9.63 µs, 769 ticks
 8:  9.63 µs, 770 ticks
 9: 11.57 µs, 926 ticks
10: 11.57 µs, 926 ticks

The measurement was done in the pwm_timer_isr itself, by adding some lines of code:

// added ---------------------------------------------
volatile int32_t test_asm_time = 0;
volatile int32_t test_pwm_min_time = 0x7FFFFFFF;
// --------------------------------------------------

void ICACHE_RAM_ATTR pwm_timer_isr() //103-138
{

// added ---------------------------------------------
    int32_t test_time0;
    int32_t test_time1;
 
    asm volatile ("rsr %0, ccount" : "=r"(test_time0));
    test_time1 = test_asm_time;
    if(test_time1 && test_time0 - test_time1 < test_pwm_min_time)
      test_pwm_min_time = test_time0 - test_time1;
// --------------------------------------------------
    
    
    struct pwm_isr_table *table = &(_pwm_isr_data.tables[_pwm_isr_data.active]);
    static uint8_t current_step = 0;
    TEIE &= ~TEIE1;//14
    T1I = 0;//9
    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;
        }
    }

// added ---------------------------------------------
    asm volatile ("rsr %0, ccount" : "=r"(test_time1));
    test_asm_time = test_time1;
// --------------------------------------------------

    T1L = (table->steps[current_step] * pwm_multiplier);//23
    TEIE |= TEIE1;//13
}

And resetting for new analogWrite in __analogWrite:

extern void __analogWrite(uint8_t pin, int value)
{
    bool start_timer = false;

// added ---------------------------------------------
    test_asm_time = 0;
    test_pwm_min_time = 0x7FFFFFFF;
// --------------------------------------------------

And for testing I used this sketch:

extern int32_t test_pwm_min_time;

void setup() {
  Serial.begin(115200);
  while(!Serial);
}

uint16_t input_value = 0;
char inputbuffer[80];

void loop() {
  size_t charcount;
  if(Serial.available()) {

    charcount = Serial.readBytesUntil('\n',inputbuffer,79);
    inputbuffer[charcount] = 0;
    sscanf(inputbuffer,"%d",&input_value);

    analogWrite(D2,input_value);
    delay(10);
    Serial.print(input_value);
    Serial.print(": ");
    Serial.print((float) test_pwm_min_time/80);
    Serial.print(" µs, ");
    Serial.print(test_pwm_min_time);
    Serial.println(" ticks");
  }
}

Besides the same values, there is also the other bug, that the duration is about 3 µs too long for odd values and about 2 µs for even values.

About short enough durations I would like to discuss, because I am working about shared timers for pwm and tone at the same time. Currently I can manage 72 µs from end of the isr until the next beginning of the isr. But also the code in the isr consumes time.

Whith whom may I discuss the interface? For pwm I implemented an own loop for allowing also small values:

          while(true) {

            ((void (*)()) (ptr->callback))(); // otherwise call it without parameter

            int32_t pwm_ccount = expire_counts[TIMER_SHARED_PWM];

            if(!pwm_ccount) {
              timer_index = TIMER_NOT_KNOWN;        // set flag for searching the next timer to expire
              --shared_timer_count;
              break;
            }

            if(pwm_ccount - asm_ccount() >= PWM_MIN_STEP)
              break;
 
            delay_ticks_inline(pwm_ccount - asm_ccount() - ISR_PWR_LOOP_LIMIT);
          }

If the user isr callback would return the expire time, instead of calling a write function and if it's sure, that the isr will not be disabled within the isr itself, then this could save about 40 ticks (30 ticks would then be possible from end of the isr until to the next start) and would allow also the adjustment for the execution time of the pwm isr for the smallest value analogWrite(1)

[earlephilhower]

#4640 redid the whole PWM model to support tone/analogWrite/Servo/others. I believe we pulled out the logic analyzer and measured repeated PWM values much tighter than the older code. Closing this, but if you have something specific you can open a new issue (and PR to fix it would be nice).