Problem when using Nano Every at 20MHz

[david-nc]

Servo positioning is incorrect when using Servo library with a Nano Every running at 20MHz. The calculation in Servo.cpp to adapt to the clock frequency is dropping the fractional part and producing an incorrect result.

In the file Servo/src/megaavr/Servo.cpp, line 6 is as follows:

#define usToTicks(_us)    ((clockCyclesPerMicrosecond() / 16 * _us) / 4)                 // converts microseconds to tick

clockCyclesPerMicrosecond() / 16 is rounding off to 1 before multiplying by _us, producing an incorrect result.

Changing the line to the following seems to correct the problem:

#define usToTicks(_us)    (((int32_t)clockCyclesPerMicrosecond() * _us / 16) / 4 )                 // converts microseconds to tick

[dcuartielles]

[Update 20190914 - DISREGARD THIS COMMENT
I was brought to notice that the original bug declaration was made under the assumption that the board was running at 20MHz which is not my case. Therefore, most likely, the error is NOT the same. I am reporting it separately.
]

have found an issue that seems to be related to this report. Have made the following video to document it: https://vimeo.com/359635766 pass: oniudra.

See the following code to replicate the video:

//#define USESERVO

#ifdef USESERVO
  #include <Servo.h>
#endif

const int motorApin = 9;
const int motorBpin = 10;

const int MIN = 500;
const int MAX = 2500;

int motorSpeed = 127; // min speed 0, max speed 255
long motorSpeedUs = 1500;
int serVal = 0;

#ifdef USESERVO
  Servo motorA;
  Servo motorB;
#endif

void setup() {
  Serial.begin(9600);

#ifdef USESERVO
  motorA.attach(motorApin, MIN, MAX);
  motorB.attach(motorBpin);
#endif
}

void loop() {
  if (Serial.available()) {
    serVal = Serial.read();

    switch (serVal) {
      case '+':
        motorSpeed += 10;
        if (motorSpeed >= 255) motorSpeed = 255;
        motorSpeedUs = map(motorSpeed, 0, 255, MIN, MAX);
        Serial.print("Speed: ");
        Serial.println(motorSpeedUs);
        break;
      case '-':
        motorSpeed -= 10;
        if (motorSpeed <= 0) motorSpeed = 0;
        motorSpeedUs = map(motorSpeed, 0, 255, MIN, MAX);
        Serial.print("Speed: ");
        Serial.println(motorSpeedUs);
        break;
      case 's':
        motorSpeed = 127;
        motorSpeedUs = map(motorSpeed, 0, 255, MIN, MAX);
        Serial.println("Stop");
        break;
    }

#ifdef USESERVO
    motorA.writeMicroseconds(motorSpeedUs);
    motorB.writeMicroseconds(1500);
#endif

#ifndef USESERVO
    analogWrite(motorApin, motorSpeed);
    analogWrite(motorBpin, 0);
#endif
  }
}

[khushi02]

Can I take this issue and change line 6?

[Kees-van-der-Oord]

I am also playing with the Nano Every @ 20Mhz and noticed that at this speed the millis() and micros() functions return inconsistent values ...
In MegaAvr20Mhz.h there is a millis/micros implementation that works as expected - but it comes at a cost: the stock micros() execution time is 7 us, my correct one takes 38 us to complete :-(
Kees

[tomalmy]

The problem with micros() with the 20MHz clock is that the timer tick isn't an integer number of microseconds (3.2µs to be exact), which makes it erratic. This can be fixed with a TCA prescaler divisor of 4 instead of 64, a change that has many ramifications. Every attempt I've made to improve on the default code increases the micros() execution time because it needs to perform a division, which is painfully slow in an AVR. The default code gets by with shifts.

[Kees-van-der-Oord]

I 'm curious to see your attempt for implementing a correct micros(), as said, my attempt in MegaAvr20Mhz.h works correctly, but the comes with a price (38us instead of 7us). So I suppose that selecting the 20MHz clock only pays off when the code does not call micros() too often ....

[tomalmy]

As a compromise for the ideal /4, I settled on /8 which gives an ideal 0.5 µs timer tick at 16MHz and a 0.4µs tick at 20MHz. I really wanted an integer number of ticks per microsecond, but the faster rate would have caused problems for me elsewhere. I also wanted an exact 1ms interrupt period to make the ISR cleaner.

Changes to timer.h:

#if F_CPU == 20000000L // 20 MHz
#define TIME_TRACKING_TIMER_PERIOD    2499  // To get exactly 1ms with the /8 prescalar
#else // 16 MHz
#define TIME_TRACKING_TIMER_PERIOD    1999  // To get exactly 1ms with the /8 prescalar
#endif
#define TIME_TRACKING_TICKS_PER_OVF   (TIME_TRACKING_TIMER_PERIOD + 1)  // Timer ticks per overflow of TCB3
#define TIME_TRACKING_TIMER_DIVIDER   8    // Clock divider for TCB0
#define TIME_TRACKING_CYCLES_PER_OVF  (TIME_TRACKING_TICKS_PER_OVF * TIME_TRACKING_TIMER_DIVIDER)

The new ISR:

ISR(TCB3_INT_vect)
{
// We know that there will be no fraction because interrupt is exactly at 1ms intervals
// So we can simplify this code a great deal
	timer_millis++;
	timer_overflow_count++;

	/* Clear flag */
	_timer->INTFLAGS = TCB_CAPT_bm;
}

It is not obvious to me that timer_overflow_count is actually needed, and timer_millis can always be used instead. But that represents a small savings.

The new micros:

unsigned long micros() {
	unsigned long overflows, microseconds;
	uint16_t ticks;
	/* Save current state and disable interrupts */
	uint8_t status = SREG;
	cli();

	/* Get current number of overflows and timer count */
	overflows = timer_overflow_count;
	ticks = _timer->CNT;

	/* If the timer overflow flag is raised, we just missed it,
	increment to account for it, & read new ticks */
	if(_timer->INTFLAGS & TCB_CAPT_bm){
		overflows++;
		ticks = _timer->CNT;
	}
	/* Restore state */
	SREG = status;

	/* Return microseconds of up time  (resets every ~70mins) */
	#if F_CPU == 20000000L // A clock tick is 400ns
	microseconds = overflows*1000 + ticks*2/5;
	#else // a clock tick is half a microsecond
	microseconds = overflows*1000 + ticks/2;
	#endif
	return microseconds;	
}

The divide by 5 is the performance killer at 20MHz.

[david-nc]

Instead of using division in the line

microseconds = overflows*1000 + ticks*2/5;

try the following code, which eliminates the slow division, but does suffer from a slight error because 26214/65536 is not exactly 2/5.

microseconds = overflows * 1000 + (((uint32_t)ticks * 26214) >> 16);

[tomalmy]

Excellent point (I wish I thought of that). The slight error good enough! The maximum number of ticks is 2499. 2499*26214/65536 = 999.584747314 on my calculator, which would be 999µs. That would be fine. We aren't going for greater than 1µs precision or accuracy greater than +/- 0.5µs. The built-in system clock isn't accurate, no where near within a microsecond in a millisecond, anyway.

I just timed micros() in all conditions.

Code                   16MHz            20Mhz
 Original                6.2µs              5.0
 Replaced             5.5                  14.5

Using the 26214 code, execution time dropped from 14.5 to 5.9, now faster than the unmodified 16MHz code. Also saved 44 bytes over the original replaced code (246 over the original library). A win no matter how you look at it.

[tomalmy]

I realized that I missed mentioning a required change. The TCB3 timer must be in Periodic Interrupt Mode and not 8-bit PWM mode. There is a comment in the init() function that it is in the "Default Periodic Interrupt Mode" but that is not the case and it just so happens that the library code works fine even in the wrong mode!

So this line must be added:
_timer->CTRLB = (TCB_CNTMODE_INT_gc); // Not really the default, so set it!