Proposed update

[GoogleCodeExporter]

I'm new to this and not sure of the etiquette for proposing a change to 
existing libraries.

I started development of a timer using Timer1, but then came across your 
TimerOne library.  Rather than create something new, I'd like to propose some 
amendments, but I'm not sure how I go about it.

To give you a feel, my suggested amendments are as follows:

 *  - Add (long) assignments and casts to TimerOne::read() to ensure calculations involving tmp, ICR1 and TCNT1 aren't truncated
 *  - Ensure 16 bit registers accesses are atomic - run with interrupts disabled when accessing
 *  - Remove global enable of interrupts (sei())- could be running within an interrupt routine)
 *  - Counter reset to 1 rather than zero.  Datasheet vague on this, but experiment shows that overflow interrupt 
 *  - flag gets set whilst TCNT1 == BOTTOM, resulting in a phantom interrupt.  Could code around, but unnecessary complexity
 *  - Start() amended to start counter and handle all interrupt enabling.  
 *  - Point restart() at start().  Can't see any real difference

Andrew

Original issue reported on code.google.com by pittsfo...@gmail.com on 7 Oct 2011 at 7:31

[GoogleCodeExporter]

Sorry, should have made it clear that I've already made the amendments and have 
the revised code available.  My question was more how I get the amendments 
incorporated into the official release.

Original comment by pittsfo...@gmail.com on 8 Oct 2011 at 11:01

[GoogleCodeExporter]

Heya Andrew,

You can attach a file to your post here and I'd be happy to merge your changes 
into the mainline within the next week or so.  I am trying to move all of the 
libraries I maintain to the new Arduino libraries github organization, so it 
may not go though until after that.

Most of these changes sound pretty good, but the last one will certainly break 
some functionality that I currently use.  start() should continue counting from 
the point where the timer was stop()'ed.  restart() should start counting from 
zero.

-Lex

Original comment by lex.v.ta...@gmail.com on 8 Oct 2011 at 2:54

[GoogleCodeExporter]

Hi Lex

I did wonder about start().  As you say, it currently resumes from wherever the 
count has got to.  However, I thought that might be an oversight, as start() is 
used by both attachInterrupt() and pwm() to commence timing. 

To avoid breaking things, I've created a new function called startBottom(), and 
pointed attachInterrupt(), pwm() and restart() to that.  That means start() is 
functionally unchanged and resumes from the current count.  However (and I'm 
not sure how much this breaks your current code), but the current naming might 
confuse people (as it did me).  The word 'start' and 'restart' both imply a 
start from zero, whereas something like 'resume' might be more appropriate for 
continuing from before.  Just a thought.

My original comment reset the counter to 1 to avoid the phantom interrupt 
problem.  However, whilst this won't cause any problems in vast majority of 
cases, when the timing is very short (and ICR1 set closer to zero) then 
starting at 1 might introduce some minor timing errors.  So I've set TCNT1 to 0 
and added a loop to wait until it moves off zero before enabling interrupts.

.h and .cpp attached.  Hope you are able to incorporate these.  By the way, 
great idea to use mode 8.  I'd started out using mode 2 (CTC), but your 
approach doubles the max time and allows for PWM (although I'm not using that). 

Original comment by pittsfo...@gmail.com on 8 Oct 2011 at 6:26

Attachments:

• TimerOne.h
• TimerOne.cpp

[GoogleCodeExporter]

Hi Lex

Have uploaded the suggested files, and hopefully addressed your concerns
about start().  Hope you are able to use them.

Andrew 

Original comment by pittsfo...@gmail.com on 8 Oct 2011 at 6:31

[GoogleCodeExporter]

Heya Andrew,

The patch looks good! Especially the atomic read and write stuff, I'd never 
even thought it could be a problem, but reading over the relevant parts of the 
datasheet and your code makes it pretty clear why it's needed.

I've made a couple of changes to your changes (heh), I hope for the better:

char oldSREG is defined in the header as a public variable for the whole class. 
 I'd rather inflate the lib's compiled size by 1 byte than waste cycles 
creating and destroying a variable used in half the functions.

start() has been renamed resume() & startBottom() has been renamed start().  
I'm not so worried about breaking functionality with old code.  This library is 
still beta, and not certified for arduino v1.0 yet, so I'd rather change it now 
than have to deal with cruft later.  And as you suggested it is a lot clearer.

Also, inside of startBottom() - now start() - I've removed the section that 
enables the interrupts, as this breaks the ability to use start and stop with 
PWM generation.  Setting and clearing TOIE1 is strictly the domain of 
attachInterrupt() & detachInterrupt().

Oh, and I don't have a bench setup right now, so I've only done the most basic 
testing on this edit.  I won't be merging it into the mainline until I have 
tested it further.

-Lex

Original comment by lex.v.ta...@gmail.com on 10 Oct 2011 at 1:32

Attachments:

• TimerOneTesting.h
• TimerOneTesting.cpp

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Original comment by lex.v.ta...@gmail.com on 10 Oct 2011 at 1:35

[GoogleCodeExporter]

Hi Lex

You're quite right, I can see that interrupts should only be enabled/disabled 
by attach and detach.  However, this does give an interesting issue with very 
short one-shot timings.  Because start() needs to disable interrupts to set 
TCNT1 == 0, it means the interrupts have to be enabled again using 
attachInterrupt().  However, if the desired delay is very short (eg 1uS, ICR1 
== 8) I suspect the time taken between setting TCNT1 == 0 in start() and the 
interrupts being enabled by attachInterrupt() might exceed the 16 clock ticks 
available. 

The only way round this that I can see would be to set TCNT1 = 1 in start(), 
rather than 0.  This would remove the need to disable interrupts, but sacrifice 
some accuracy at short intervals (1/16th in the worst case).  

Perhaps too theoretical a possibility?

Just noticed, by the way, that the registers (and pins) for the Mega 2560 are 
different to the Uno pins assumed in the code.  OC1A is PB5 and OC1B is PB6, 
corresponding to pins 11 and 12 respectively.  Conditional compile statements?

By the way, happy for you to remove my sprinkling of "AR modified" from the 
comments if it will improve readability.  The header comments are quite 
sufficient :-)

All the best, Andrew

Original comment by pittsfo...@gmail.com on 10 Oct 2011 at 11:03

[GoogleCodeExporter]

Hey again pittsfolly.  These changes were first incorperated into v7, so I'm 
closing this particular issue.  If you want to open one about Mega support go 
for it, but officially, Timer1 does not support the Mega.  Some of the code 
references the Mega, but that's mostly just little odds and ends that are 
totally untested.  There is a lib out there that is meant for the Mega, called 
Timer3. It's available at the Arduino playground Timer1 site.  It is pretty out 
of date though, but it's a place to start.

http://arduino.cc/playground/Code/Timer1

Original comment by lex.v.ta...@gmail.com on 1 Feb 2012 at 5:59

• Changed state: Fixed