arduino / ArduinoCore-megaavr

Arduino Core for the ATMEGA4809 CPU
103 stars 62 forks source link

Use (CLK_PER / 2) instead of CLKTCA for millis / micros clock #72

Open harryjph opened 4 years ago

harryjph commented 4 years ago

This PR switches the TCB clock select from TCA (which is, by default, CPU clock / 64) to the CPU clock / 2. Then it maintains a counter in the ISR to only increment the micros / millis count every 32 interrupts, because by switching the clock source, we get interrupts 32x more frequently.

The whole reason for this is that without this the millis clock depends on TCA0, which is supposed to drive PWM pins. Without this, users cannot increase the frequency of the PWM without increasing the speed at which millis counts.

Say a user wants to drive the PWM with 4x the default frequency. They set TCA0 to be CPU clock / 16, up from CPU clock / 64, and the PWM will work at 4x the frequency. One problem: millis() ticks 4x faster, and delay() delays for 4x shorter time. This PR corrects this behaviour.

This PR has been written to add as little overhead as possible to the timer ISR, which is important as this is now called 32x as often.

harryjph commented 4 years ago

I have tested this using the following code and an oscilloscope:

void setup() {
  // Increase TCA0 speed to F_CPU
  TCA0.SINGLE.CTRLA = TCA_SINGLE_CLKSEL_DIV1_gc | TCA_SINGLE_ENABLE_bm;

  // Timing test - Drive a pin high for one second
  pinMode(4, OUTPUT);
  digitalWrite(4, HIGH);
  delay(1000);
  digitalWrite(4, LOW);
}

void loop() {}

This is the output:

After image

So I am confident that this maintains correct timing. This test, without this patch, produces the following output:

Before image

CLAassistant commented 3 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

softhack007 commented 2 years ago

Just curious - does "CPU clock / 2" mean that the ISR would be called at 8Mhz on a 16Mhz processor? Hope not, otherwise the ISR would only have two asm instructions before the next interrupt is generated...

softhack007 commented 2 years ago

another idea - it seems that MegaCoreX has a different implementation, see https://github.com/MCUdude/MegaCoreX/blob/master/megaavr/cores/coreX-corefiles/wiring.c. Their ISR is very small, several clock freqs are supported, and other fancy stuff.

https://github.com/MCUdude/MegaCoreX/commit/298601ba1c0cf83d1ee26130d7afed0f27a51b9d

Does MegaCoreX have the problem described by this PR, i.e. changing the PWM freq will make millis() and friends run faster/slower? Would it be an option to adopt wiring.c from MegaCoreX?