Skip to content

Bugfix: micros() returns smaller number when timer wraps #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 8, 2023

Conversation

Kees-van-der-Oord
Copy link
Contributor

See issue 49

  1. Fix for anomaly in micros() when the counter wrapped just before it was read (same solution as in R3 code)
  2. Specify timer configuration as constants to make micros() considerably faster

1. Fix for anomaly in micros() whwn the counter wrapped just before it was read
2. Specify timer configuration as constants to make micros() considerably faster
@CLAassistant
Copy link

CLAassistant commented Jul 16, 2023

CLA assistant check
All committers have signed the CLA.

@aentinger
Copy link
Contributor

This fixes #49.

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Kees-van-der-Oord ☕ 👋

Thank you for your PR. I'm inclined to merge it but there are a couple of suggestions I'd like you to tackle first.

@aentinger aentinger changed the title fix for issue 49: micros() returns smaller number when timer wraps Bugfix: micros() returns smaller number when timer wraps Jul 24, 2023
@aentinger aentinger added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself type: enhancement Proposed improvement and removed type: imperfection Perceived defect in any part of project labels Jul 24, 2023
#define _timer_index 0
#define _timer_get_underflow_bit() R_AGT0->AGTCR_b.TUNDF
#define _timer_clock_divider TIMER_SOURCE_DIV_8 // dividers 1, 2 and 8 work because _timer_period is < 16-bit. the divider 4 seems not supported: acts as 1
#define _timer_clock_freq 24000000UL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not work on all FSP supported boards (like Portenta C33). It's very important to fix this before merging since otherwise the number of ifdefs in the core would soon become unbearable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the defines have to go @Kees-van-der-Oord , as already mentioned here.

I don't see how there can be any runtime speed-up by having them in the code, as its simple string search and replace done by the preprocessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comment here. I will try to find the most efficient implementation that also works for the Portenta C33.

#define _timer_index 0
#define _timer_get_underflow_bit() R_AGT0->AGTCR_b.TUNDF
#define _timer_clock_divider TIMER_SOURCE_DIV_8 // dividers 1, 2 and 8 work because _timer_period is < 16-bit. the divider 4 seems not supported: acts as 1
#define _timer_clock_freq 24000000UL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the defines have to go @Kees-van-der-Oord , as already mentioned here.

I don't see how there can be any runtime speed-up by having them in the code, as its simple string search and replace done by the preprocessor.

@Kees-van-der-Oord
Copy link
Contributor Author

Kees-van-der-Oord commented Aug 5, 2023

I just pushed a new version that is compatible with both the Uno R4 and the Portenta C33 (and any FSP chip with a reasonable AGT clock frequency). Optimizing the code a little more, I was able to almost match the execution time of micros() on the R4 to that on the R3:

               average     max
Uno R3:        3.46 us     24 us
Uno R4:        3.61 us      7 us
Portenta C33:  1.26 us      3 us

the code to test the micros() function:

void test_micros3() {
  Serial.println("testing micros3 - send input to stop");
  Serial.flush();
  uint32_t count = 0;
  uint32_t total = 0;
  uint32_t max = 0;
  uint32_t min = 0xFFFFFFFF;
  while(!Serial.available() && (count < 1000000)) {
    uint32_t t1 = micros();
    uint32_t t2 = micros();
    uint32_t period = t2 - t1;
    total += period;
    if(period > max) max = period;
    if(period < min) min = period;
    ++count;
  }
  double ave = -1;
  if(count) {
    ave = double(total) / count;
  }
  Serial.print("testing micros3 average: ");Serial.print(ave);Serial.print(" min: ");Serial.print(min);p(" max: ");p(max);Serial.println(" us");
  while(Serial.available()) Serial.read();
}

@Kees-van-der-Oord
Copy link
Contributor Author

I found one of reasons why the performance is so slow: the function FspTimer::get_counter(). I just pushed a version in whch the call to this function is replaced with a direct counter registry read,
The performance is now much better:

               average     max
Uno R3:        3.46 us     24 us
Uno R4:        2.12 us      5 us
Portenta C33:  0.60 us      2 us

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work @Kees-van-der-Oord.

We are getting there, but there has been a misunderstanding between you and me. I did not ask you to replace macros with variables and inline functions, I asked you to get rid of the macros alltogether and just inline those statements directly into the code.

optimized micros() even more ...
@aentinger
Copy link
Contributor

Successfully tested for

  • Portenta C33 ✔️
  • Uno R4 WiFi ✔️
  • Uno R4 Minima ✔️

@aentinger aentinger merged commit 14783dc into arduino:main Aug 8, 2023
pennam pushed a commit to pennam/ArduinoCore-renesas that referenced this pull request Aug 9, 2023
cristidragomir97 pushed a commit to cristidragomir97/ArduinoCore-renesas that referenced this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants