Increase micros () accuracy for 18.432 and 20 MHz

[cburstedde]

Hi, adding just a few more adds and shifts bumps micros accuracy quite a bit.

[MCUdude]

Haven't had the chance to look through this PR yet. Quick question though, will 32 MHz work? Even though none of my cores supports this frequency, the timing functions do.

EDIT: Sorry, wrong PR!

[MCUdude]

But thanks for submitting this PR and #24. It looks like you know what you're doing since very few people are willing to touch this almost-cycle-accurate code.

BTW have you tested this on actual hardware? It is a while since I wrote this code, but I used math, a very stripped-down program, and my oscilloscope to tune micros() to work reasonably well for all frequencies, even the weird UART-compatible ones like 18.432 MHz.

[cburstedde]

I have an 18.432M crystal running and will try on 8M during the next couple days. I don't have any accurate measurement equipment, so additional testing will be nice. I'm confident concerning my math though.

[MCUdude]

While you're at it, would you like to add 22.1184 MHz support too? AFAIK this is the only "weird" clock that isn't currently supported. If it isn't obvious already, it's just 11.0592 MHz * 2.

I have an 18.432M crystal running and will try on 8M during the next couple of days. I don't have any accurate measurement equipment, so additional testing will be nice. I'm confident concerning my math though.

Previously I've been using a signal generator attached to the XTAL1 pin. It doesn't seem like any of your PRs will affect timing for an 8 MHz clock.

[cburstedde]

While you're at it, would you like to add 22.1184 MHz support too? AFAIK this is the only "weird" clock that isn't currently supported. If it isn't obvious already, it's just 11.0592 MHz * 2.

Sure, I can look into it.

I have an 18.432M crystal running and will try on 8M during the next couple of days. I don't have any accurate measurement equipment, so additional testing will be nice. I'm confident concerning my math though.

Previously I've been using a signal generator attached to the XTAL1 pin. It doesn't seem like any of your PRs will affect timing for an 8 MHz clock.

True. Sorry I can't help you much with hardware testing.

[cburstedde]

This seems to be ready and running fine on an 18.432 MHz crystal. I've added the 18 MHz and 22.1184 MHz speeds.

Did not touch delayMicroseconds(), which may need an update for the two new clock speeds.

[MCUdude]

Thank you for doing all this! Sorry for not being more active, but I've been busy with work and family. Do you feel like these two PRs can/should be merged or closed, or do you have more improvements that fit into these two PRs?

[MCUdude]

Oh, another thing. Compared to the existing code, how much overhead will these PR's add to the code? How much longer time will it take to execute the ISR now that the timing calculations have been improved? And is the numbers of cycles in the ISR the same regardless of the count (not that it has ever been like this)?

[cburstedde]

Hi, my pleasure. I got carried away a bit, thanks for putting up with my constantly appending to PRs.

I've been running an 18 and an 18.432 crystal all day with a 3-second blink sequence, and the micros error seems to be within the specs of the README, just taking repeated measurements of my laptop time whenever the 3-second cycle restarts.

I think this would be a good point to merge. Of course, if you could run a little sketch to confirm the basics at one or two frequencies that would be nice. I've ordered a bunch of different crystals, but we can fix any of those later if required.

Can I ask you to officially support 18 MHz since I have a couple of these crystals? delayMicroseconds() is still open for this one.

(In fact I have new code that puts micros() at exact zero drift, just as millis(). But it's a different algorithm, and I think merging this one here first gives us good progress on the traditional implementation, and we can wait and see how comfortable people are with it before proceeding.)

[cburstedde]

For the micros() correction, the ISR is not changed. The calculation of micros() goes up from 3 to 4 long right shifts to 4 or 5 for the odd frequencies. So we get a 10x to 100x accuracy improvement for below 1.5x the cost. But I'm (so far) not an assembly guy, so I'd appreciate your opinion.

[MCUdude]

I've been running an 18 and an 18.432 crystal all day with a 3-second blink sequence, and the micros error seems to be within the specs of the README, just taking repeated measurements of my laptop time whenever the 3-second cycle restarts.

sounds very promising!

I think this would be a good point to merge. Of course, if you could run a little sketch to confirm the basics at one or two frequencies that would be nice. I've ordered a bunch of different crystals, but we can fix any of those later if required.

I'll see what I can do!

Can I ask you to officially support 18 MHz since I have a couple of these crystals? delayMicroseconds() is still open for this one.

Since 18 MHz isn't really a common frequency for AVR microcontrollers, I don't think I'll be adding support for this frequency in the Arduino IDE package, since the list of supported clock frequencies is already long enough. But this doesn't mean the core files can't support 18 MHz!

As long as the core files support 18 MHz, you can either edit the Arduino boards.txt file or define board_build.f_cpu = 18000000L in the PlatformIO configuration. Is this OK for you?

For the micros() correction, the ISR is not changed. The calculation of micros() goes up from 3 to 4 long left shifts to 4 or 5 for the odd frequencies. So we get a 10x to 100x accuracy improvement for below 1.5x the cost. But I'm (so far) not an assembly guy, so I'd appreciate your opinion.

I'm not an assembler guy either. But I'll do some measurements just to get a hint on how many cycles the ISR takes with and without these PRs. I'll probably just toggle a pin as the first and last thing to do in the ISR. By measuring the output with an oscilloscope I'll at least get an idea.

I'm not particularly afraid of the number of executions it takes to calculate millis() or micros(). It's periodic ISR that really eats away CPU time. When I started working on MicroCore, the Arduino core I forked, core13 had a terrible micros() implementation where the ISR fired every 256th clock cycle (no timer prescaler). All the ISR did was to increment an unsigned long by one, but that's still a lot of average CPU time wasted!

[cburstedde]

I've been running an 18 and an 18.432 crystal all day with a 3-second blink sequence, and the micros error seems to be within the specs of the README, just taking repeated measurements of my laptop time whenever the 3-second cycle restarts.

sounds very promising!

I hope so. An independent check would be great. It's awesome watching the blink drift by one second in half a day or so. :)

I think this would be a good point to merge. Of course, if you could run a little sketch to confirm the basics at one or two frequencies that would be nice. I've ordered a bunch of different crystals, but we can fix any of those later if required.

I'll see what I can do!

Can I ask you to officially support 18 MHz since I have a couple of these crystals? delayMicroseconds() is still open for this one.

Since 18 MHz isn't really a common frequency for AVR microcontrollers, I don't think I'll be adding support for this frequency in the Arduino IDE package, since the list of supported clock frequencies is already long enough. But this doesn't mean the core files can't support 18 MHz!

As long as the core files support 18 MHz, you can either edit the Arduino boards.txt file or define board_build.f_cpu = 18000000L in the PlatformIO configuration. Is this OK for you?

Yes, thanks, no problem, I can look into the boards/platform files.

For the micros() correction, the ISR is not changed. The calculation of micros() goes up from 3 to 4 long left shifts to 4 or 5 for the odd frequencies. So we get a 10x to 100x accuracy improvement for below 1.5x the cost. But I'm (so far) not an assembly guy, so I'd appreciate your opinion.

I'm not an assembler guy either. But I'll do some measurements just to get a hint on how many cycles the ISR takes with and without these PRs. I'll probably just toggle a pin as the first and last thing to do in the ISR. By measuring the output with an oscilloscope I'll at least get an idea.

I'm not particularly afraid of the number of executions it takes to calculate millis() or micros(). It's periodic ISR that really eats away CPU time. When I started working on MicroCore, the Arduino core I forked, core13 had a terrible micros() implementation where the ISR fired every 256th clock cycle (no timer prescaler). All the ISR did was to increment an unsigned long by one, but that's still a lot of average CPU time wasted!

So in this light the micros PR has no change to the ISR, but the millis PR does. The new code I have not pushed yet (for making micros exact) actually removes one long variable from global memory and one long increment from each ISR, so we'll be cheaper than before when it's all done. Still I'd be curious about how the current status works for you. Thanks for looking into it!

[MCUdude]

As long as the core files support 18 MHz, you can either edit the Arduino boards.txt file or define board_build.f_cpu = 18000000L in the PlatformIO configuration. Is this OK for you?

Yes, thanks, no problem, I can look into the boards/platform files.

I will consider a clock frequency fully supported when we have accurate millis, micros, delay, and delayMicroseconds for it. As for the 18 and 22.1184 MHz clock, we still have to figure out accurate delayMicroseconds for these frequencies. When this is done they can be added to the supported clock frequencies list.

The new code I have not pushed yet (for making micros exact) actually removes one long variable from global memory and one long increment from each ISR, so we'll be cheaper than before when it's all done. Still I'd be curious about how the current status works for you. Thanks for looking into it!

I'll see if I can manage to give your micros() implementation a timing test tomorrow evening. It has to be tested properly because this code will be bundled with (almost) all my Arduino cores, which combined have a pretty large user mass. This being said, if there are inherent issues, they will probably be found by some sharp-eyed user!