SpenceKonde / megaTinyCore

Arduino core for the tinyAVR 0/1/2-series - Ones's digit 2,4,5,7 (pincount, 8,14,20,24), tens digit 0, 1, or 2 (featureset), preceded by flash in kb. Library maintainers: porting help available!
Other
558 stars 146 forks source link

micros issue after 1.1.8 #189

Closed ynitagem closed 3 years ago

ynitagem commented 4 years ago

Hi,

I'm having an issue that may be related with micros() changes made on 1.1.9. My test case is a simple frequency counter (comes from mains zero crossing - 50Hz). I'm measuring microseconds (with micros()) which is triggered by an interrupt.

Screenshot below is from board package v1.1.8, the result is kinda clean. Screenshot 2020-05-09 16 00 47

This screenshot is from v1.1.9 and after (including latest v2.x): Screenshot 2020-05-09 16 01 27

It still is centered on 50Hz but there are glitches, repeating with same values (probably related with micros() resolution)

Only thing changed between the screenshots is the board package version. Is this issue related with changes from 1.1.9 or I'm missing something else? Trying with ATTiny1604 /w 20MHz.

Here is the code that I grabbed the screenshots from:

volatile uint32_t last_zc, zc_intv;
volatile uint8_t zcoccured;

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

  VPORTA.DIR &= ~(PIN2_bm | PIN6_bm);
  VPORTA.OUT &= ~(PIN2_bm | PIN6_bm);
  PORTA.PIN2CTRL = PORT_PULLUPEN_bm | PORT_ISC_BOTHEDGES_gc;
  PORTA.PIN6CTRL = PORT_PULLUPEN_bm | PORT_ISC_BOTHEDGES_gc;
}

void loop() {
  if (zcoccured) {
    float freq = 500000.0 / zc_intv;

    Serial.println(freq);
    //Serial.print(",");
    //Serial.println(zc_intv);

    // PA5 toggle LED
    PORTA.OUTTGL  = PIN5_bm;

    zcoccured = 0;
  }
}

ISR(PORTA_PORT_vect) {
  byte flags = PORTA.INTFLAGS;
  PORTA.INTFLAGS = flags; //clear flags

  if (flags & PIN2_bm) {
    if (VPORTA.IN & PIN2_bm) {
      // rising edge
      const uint32_t now = micros();

      zc_intv = now - last_zc;
      last_zc = now;

      zcoccured = 1;
    }
  }
}

Thanks for the all your hard work!

SpenceKonde commented 4 years ago

What clock frequency are you running it at? What are you using for the millis timer? And, which part is that?

That's the kind of thing I was fixing in 1.1.8!

On Sat, May 9, 2020 at 9:10 AM ynitagem notifications@github.com wrote:

Hi,

I'm having an issue that may be related with micros() changes made on 1.1.9. My test case is a simple frequency counter (comes from mains zero crossing

  • 50Hz). I'm measuring microseconds (with micros()) which is triggered by an interrupt.

Screenshot below is from board package v1.1.8, the result is kinda clean. [image: Screenshot 2020-05-09 16 00 47] https://user-images.githubusercontent.com/65074555/81474480-5c107000-920e-11ea-8635-3534be2d3b04.png

This screenshot is from v1.1.9 and after (including latest v2.x): [image: Screenshot 2020-05-09 16 01 27] https://user-images.githubusercontent.com/65074555/81474479-5b77d980-920e-11ea-8084-7f90adb2ca6f.png

It still is centered on 50Hz but there are glitches, repeating with same values (probably related with micros() resolution)

Only thing changed between the screenshots is the board package version. Is this issue related with changes from 1.1.9 or I'm missing something else?

Here is the code that I grabbed the screenshots from:

volatile uint32_t last_zc, zc_intv;volatile uint8_t zcoccured; void setup() { Serial.begin(115200);

VPORTA.DIR &= ~(PIN2_bm | PIN6_bm); VPORTA.OUT &= ~(PIN2_bm | PIN6_bm); PORTA.PIN2CTRL = PORT_PULLUPEN_bm | PORT_ISC_BOTHEDGES_gc; PORTA.PIN6CTRL = PORT_PULLUPEN_bm | PORT_ISC_BOTHEDGES_gc; } void loop() { if (zcoccured) { float freq = 500000.0 / zc_intv;

Serial.println(freq);
//Serial.print(",");
//Serial.println(zc_intv);

// PA5 toggle LED
PORTA.OUTTGL  = PIN5_bm;

zcoccured = 0;

} } ISR(PORTA_PORT_vect) { byte flags = PORTA.INTFLAGS; PORTA.INTFLAGS = flags; //clear flags

if (flags & PIN2_bm) { if (VPORTA.IN & PIN2_bm) { // rising edge const uint32_t now = micros();

  zc_intv = now - last_zc;
  last_zc = now;

  zcoccured = 1;
}

} }

Thanks for the all your hard work!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SpenceKonde/megaTinyCore/issues/189, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTXEWZJGAWK76NBLN7SSP3RQVI5HANCNFSM4M4ZOLPA .

--


Spence Konde Azzy’S Electronics

New products! Check them out at tindie.com/stores/DrAzzy GitHub: github.com/SpenceKonde ATTinyCore: Arduino support for almost every ATTiny microcontroller Contact: spencekonde@gmail.com

ynitagem commented 4 years ago

Trying with ATTiny1604 /w 20MHz. millis timer is on default (TCA).

Side note: I can't select TCB, it gives a compile error regarding tone() even if I'm not using tone(). ("This part only has one type B timer, but it is selected as millis source; tone cannot be used.") It is not an issue for me because I already utilizing TCB for a different purpose.

Side note 2: If I select 16MHz system clock, Serial.print becomes gibberish (tried with 115200 baud).

SpenceKonde commented 4 years ago

Ugh, okay, I see where the problem with tone is coming from. In my attempt to give better error messages, I forgot that the build process compiles everything, but won't link the vectors in a file if nothing else in that file is used.... so the fact that the errors keep it from compiling is not acceptable. This is why I need CI testing,... I tested it quite exhaustively - with the 1614 and 3216 which both have two type B timers...

SpenceKonde commented 4 years ago

The reason that 16MHz makes serial gibberish is that you need to do burn bootloader to set the fuses to switch from 16MHz-derived and 20MHz derived clocks, as described in the documentation.

ynitagem commented 4 years ago

The reason that 16MHz makes serial gibberish is that you need to do burn bootloader to set the fuses to switch from 16MHz-derived and 20MHz derived clocks, as described in the documentation.

My fault, sorry. I wrongly assumed that it always rewrite fuses when programming with UPDI (without a bootloader).

I wanted to check if this is happening on other clock speeds, normally I intend to use 20MHz. This is with 16MHz. Screenshot 2020-05-09 17 44 48

This is with 8MHz. Screenshot 2020-05-09 17 52 21

With lower clock speeds the peaks are higher but outputs the correct values more often.

SpenceKonde commented 4 years ago

No problem - does running at 16 help?

On Sat, May 9, 2020 at 10:40 AM ynitagem notifications@github.com wrote:

The reason that 16MHz makes serial gibberish is that you need to do burn bootloader to set the fuses to switch from 16MHz-derived and 20MHz derived clocks, as described in the documentation.

My fault, sorry. I wanted to check if this is happening on other clock speeds, normally I intend to use 20MHz.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/SpenceKonde/megaTinyCore/issues/189#issuecomment-626186131, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTXEW2QEYG55JH5KUJG36DRQVTODANCNFSM4M4ZOLPA .

--


Spence Konde Azzy’S Electronics

New products! Check them out at tindie.com/stores/DrAzzy GitHub: github.com/SpenceKonde ATTinyCore: Arduino support for almost every ATTiny microcontroller Contact: spencekonde@gmail.com

SpenceKonde commented 4 years ago

Oh, now I see the scope traces (didn't see them in email)

That is... very concerning. I thought I had fixed the issues like that in 1.1.8... I suspect there is also negative time-travel happening.

SpenceKonde commented 4 years ago

Regarding it not writing fuses when programming via UPDI not using bootloader - sometimes I wonder why I don't have it do that... but like, Arduino did set a clear precedent (though the impetus to not do that in order to avoid bricking parts is greatly lessened on the tinies, and absent entirely on the mega/DA series)

ynitagem commented 4 years ago

I compared 1.1.8 and 1.1.9. On hardware\megaavr\1.1.9\cores\arduino\wiring.c, if I find&replace "HUNF" to "LUNF" (lines 102, 149, 545) the result is same as with 1.1.8 as in my very first screenshot (which I believe is the correct result). They was also "LUNF" in 1.1.8.

Is this interrupt change from high byte overflow to low byte overflow was intended?

ynitagem commented 4 years ago

If I do this same treatment with latest version (v2.0.2) there are occasional blips.

Screenshot 2020-05-09 21 28 03

These blips occurs both directions and are very close, these may be related with cycle/overflow correction. Definitely something is not happy about my spartan approach of changing things..

SpenceKonde commented 4 years ago

The change from LUNF to HUNF was intended, but not completed correctly. I will correct that change.

ynitagem commented 4 years ago

I didn't notice your commit until now. Here's the latest result:

(left side v2.0.2, right c7dc2e5 ) Screenshot 2020-05-12 20 44 57

There are still occasional / random blips but %99 of the issue seems to be solved. I can filter these outliers in my application but there may be still something to look at.

Should I close this issue? Thank you for your efforts.

SpenceKonde commented 4 years ago

No, issue should not be closed - micros glitches are not acceptable.

SpenceKonde commented 4 years ago

I think I may have fixed this, saw the mistake on my new core....

ynitagem commented 4 years ago

0e30b04 is not fixed that occasional blips in my testing. Since my original test code depends on a external source, I created a version that uses a timer for triggering. Result and the code is below:

image

I don't expect a absolutely flat line due to timing difference created by Serial but there is some repeating results that may be indicative of some over/underflow. May not be critical, I'm just sharing my findings.

volatile uint32_t last_zc, zc_intv;
volatile uint8_t zcoccured;

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

  PORTA.DIRSET = PIN5_bm;

  TCB0.CNT = 0;                       /* Count: 0x0 */
  TCB0.CTRLB = TCB_CNTMODE_SINGLE_gc; /* Single Shot */
  TCB0.INTCTRL = TCB_CAPT_bm;         /* Capture or Timeout: enabled */
  TCB0.CTRLA = TCB_CLKSEL_CLKTCA_gc;  /* Use Clock from TCA */
  TCB0.CCMP = 33333;
  TCB0.CTRLA |= TCB_ENABLE_bm; // enable timer
}

void loop() {
  if (zcoccured) {
    //float freq = 500000.0 / zc_intv;

    //Serial.println(freq);
    //Serial.print(",");
    Serial.println(zc_intv);

    // PA5 toggle LED
    PORTA.OUTTGL  = PIN5_bm;

    zcoccured = 0;
  }
}

ISR(TCB0_INT_vect) {
  const uint32_t now = micros();

  zc_intv = now - last_zc;
  last_zc = now;

  TCB0.CTRLA &= ~TCB_ENABLE_bm; // disable timer
  TCB0.CNT = 0;
  TCB0.INTFLAGS = TCB_CAPT_bm; // clear interrupt
  TCB0.CTRLA |= TCB_ENABLE_bm; // enable timer

  zcoccured = 1;
}
SpenceKonde commented 4 years ago

Thanks, investigating - the glitch in your comment from 21 days ago is extremely concerning, as it looks like the signature of backwards time travel, which should absolutely 100% never happen.

The more recent trace worries me less - but your test methodology will hide many of the most concerning types of micros timing issues, because it guarantees that the millis overflow will never occur during the TCB interrupt. TCB0 must be clocked from CLK_PER with the period set to a value which is not divisible by the period of TCA0. I think I need to set TCB0.CCNT=16318, and TCB0.CTRLA=TCB_CLKSEL_CLKPER_gc, so it will have a period of 16319 system clocks, vs 16320 for TCA0, then run it for at least 266,326,080 system clocks to see behavior across every possible alignment of the event and micros() timer...

On a different note, did you ever try out whether using a type B timer as the timing source for millis() helps in your application?

SpenceKonde commented 3 years ago

I think I realized what is going on there... the maximum scatter is only 6.... that's... pretty small. I think what you are seeing is just aliasing. Single digit microsecond error which does not entail long-term drift is not a problem.

SpenceKonde commented 3 years ago

Nope, there was actually a bug in here. It is fixed in 2.3.0. timing of getting the OVF intflag was wrong.