Skip to content
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

Fix timer + timer testbenches #304

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Fix timer + timer testbenches #304

merged 3 commits into from
Nov 27, 2024

Conversation

dehanj
Copy link
Member

@dehanj dehanj commented Nov 22, 2024

Description

Fix a bug in the timer core, where it misses clock cycles. It is fixed by removing a redundant timer state.
The clock cycle misses every time the prescaler counter reaches 1. This means if one uses a large prescaler, like 18E6, it is barely noticeable, but if one have a low prescaler and a high timer value it becomes significant.

Hardware timers are, according to me, usually a function of:
f_time = f_freq / (prescaler * period)
Our period us our timer register.

So the prescaler is used to divide the system clock to yield a timer with a wanted time for each tick.

To exemplify the issue:
If I use above and want the ticks in microseconds and I want the timer to expire after 1 millisecond.
I use 18 Mhz frequency as an example.

1 ms = 18 * 10^6 / ( 18 * 1000 ).
This means it needs 18 000 CPU clock ticks for 1 ms.
Using our testbench our timer counts 19000. Meaning our current implementation is f_time = f_freq / ((prescaler+1) * period)

Meaning a prescaler of 18 yields a timer frequency of 0.947 MHz instead of 1 Mhz.

This PR also fixes the testbenches, so it actually test against an expected value and makes them selftesting.

This also lowers the LC utilization with about 75 LCs.

I have not updated the digest of the bitstream yet, I will do it as soon as no feedback yields a change in the code.

Fixes # (issues)

Type of change

  • Bugfix (non breaking change which resolve an issue)

Submission checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my changes
  • I have tested and verified my changes on target
  • My changes are well written and CI is passing
  • I have squashed my work to relevant commits and rebased on main for linear history

@dehanj dehanj requested review from agren and jthornblad November 22, 2024 13:00
@dehanj dehanj force-pushed the fix-timer branch 5 times, most recently from 798b26d to cabff02 Compare November 25, 2024 12:50
@dehanj
Copy link
Member Author

dehanj commented Nov 25, 2024

Updated documentation, rebased on main.
Removed another redundant register, pointed out by Mikael.

@dehanj dehanj force-pushed the fix-timer branch 2 times, most recently from 26dfa4c to 14f89e6 Compare November 26, 2024 08:19
@agren agren force-pushed the fix-timer branch 2 times, most recently from 462ff43 to 9f0e073 Compare November 26, 2024 14:36
Remove redundant timer state. This fixes a bug where the timer misses a
clock cycle every time the prescaler counter reaches 1. This means if
one uses a large prescaler, like 18E6, it is barely noticeable, but if
one have a low prescaler and a high timer value it becomes significant.
This also yields the running_* registers redundant, which are removed.

Add clarity to the readme.

Update the timer to default to values of one, for prescaler and timer
count.
- Compare against an expected result and count errors
- Exit with the right error code
- Lower write_word() to 1 clk cycle instead of two. It only requires one
  clock cycle to write, otherwise if it is two one have to compensate for it
  in the tests since we are counting cycles.
- Compare against an expected result and count errors
- Exit with the right error code
- Clean up the output
@dehanj dehanj merged commit 5b49d80 into main Nov 27, 2024
5 checks passed
@dehanj dehanj deleted the fix-timer branch November 27, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants