First program, looking for critique...

Go To Last Post
18 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Please, be gentle. :)

 

Not much to it.  Just trying to create a simple clock timer.  The flashing part has been tested.  I assume the counter variables are working.  I had a little trouble wrapping my head around it, but I think I have it.  Made some elementary comments to help me remember what I did.  Only output is an LED that flashes marking the minute marks.  Looking for advice, or hints in convention, syntax, use, shortcuts, better ways to implement, etc.

 

Device is an attiny85, burned with Atmel Studio 7 on a AVRISP mkii.  No external clock, obviously.

 

/*
 * timer.c
 *
 * Created: 7/19/2016 10:51:59 AM
 * Author : Sherri
 */ 

#include <avr/io.h>
#include <avr/interrupt.h>
#define F_CPU 1000000UL
int tim_seg=0, mins = 0, ten_mins = 0, hrs = 0;  //initialize global clock variables
int main(void)
{
	// ---------timer setup--------------
	// USE Timer 0
	// Step #1 set the parameters for the timer TCCR0 registers
	TCCR0A = (1<<WGM01);  //value sets timer to CTC mode
	TCCR0B = (1<<CS01);  // value sets prescaler to clkio/8 will give 250 ticks; 2ms @1Mhz
	// Step #2 Choose the number of ticks to compare to the timer
	OCR0A = F_CPU/8 * 0.002 - 1;;  // value timer is compared to = number of ticks-1, in this case 249
	// OCR0B is a second value for a second compare if desired.
	// Step #3 Enable the interrupt for the timer OCR0A.  When OCR0A matches the timer, the ISR function will execute
	TIMSK = (1<<OCIE0A);  
	// Step #4 enable the AVR interrupt bit
	sei();
	// Step #5 Add the ISR routine for OCR0A (See below Main module)
	
	// -------------LED setup----------------
	DDRB = 0x01;
	PORTB = (1<<PB1);
    /* Replace with your application code */
    while (1) 
    {
    }
}
ISR(TIMER0_COMPA_vect){
	if(tim_seg<29999){
		tim_seg++;
		if(mins<9){
			mins++;
		}else{
			mins=0;
			
			if(ten_mins<5){
				ten_mins++;
			}else{
				ten_mins=0;
				hrs++;
			}
		}
	}else{
		tim_seg=0;
		PORTB ^= (1<<PB1);
	}
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Overall for a simple program, it looks fine.....

  moving forward, I would shorten the ISR so it just sets a flag, then in main if the flag is set, do the hours/mins/sec's code.   Be sure to use volatile qualifier when you create the flag as it will be shared by main() and ISR().

 

 

 

Jim

 

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
stack gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi Jim, thanks for the comments. If you set the flag, cant you just do that with CTC and eliminate the ISR altogether?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not a criticism, just a point of information.

Writing a 1 to the PIN register will toggle the associated bit, so changing the

PORTB ^= (1<<PB1);

to

PINB |= (1<<PB1);

will toggle PB1 and requires 1 AVR instruction (sbi) vs 4 for the XOR operation.

(When you are desperately trying to find some extra speed or bytes it can help!)

David (aka frog_jr)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

frog_jr wrote:

Not a criticism, just a point of information.

Writing a 1 to the PIN register will toggle the associated bit, so changing the

PORTB ^= (1<<PB1);

to

PINB |= (1<<PB1);

will toggle PB1 and requires 1 AVR instruction (sbi) vs 4 for the XOR operation.

(When you are desperately trying to find some extra speed or bytes it can help!)

 

How about if I just use:

 

PORTB = 2;

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I read that and I stil don't know what tim_seg is supposed to mean or what its doing and what the signifcance of the 29999 magic number is. Equally there's the intriguing:

	OCR0A = F_CPU/8 * 0.002 - 1;;  // value timer is compared to = number of ticks-1, in this case 249

no idea what the /8 significance is or what the .002 is for (and isn't that that same as the slightly more obvious /500 ?)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

@ki0bk, I guess I'm a little confused about your suggestion.  Are you suggesting no lines of code in the ISR and then in the main loop continually checking for the flag to be set?  If so, wouldn't the flag just be reset before the interrupt returned to the main loop?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

I read that and I stil don't know what tim_seg is supposed to mean or what its doing and what the signifcance of the 29999 magic number is. Equally there's the intriguing:

	OCR0A = F_CPU/8 * 0.002 - 1;;  // value timer is compared to = number of ticks-1, in this case 249

no idea what the /8 significance is or what the .002 is for (and isn't that that same as the slightly more obvious /500 ?)

 

tim_seg is my time segment.  It's equal to a minute.  250 ticks represents 2ms.  30000 X 2ms is one minute (less one for wraparound though I wasn't sure if it should have been decremented or not).  At that time (1 min), the number of minutes increments and the LED toggles.  There's a ten_min variable that marks 10's of minutes.  hrs marks hours.

 

CPU speed/8 because I set the prescaler to clkio/8.  .002 is milliseconds.  -1 for the total ticks to account for the wraparound when the timer resets?

 

<edit> oh, and yes I see your point about using a constant value (/500).  I didn't give that much thought.  I suppose I was more focused on registers and bits. <blush>

Last Edited: Tue. Jul 19, 2016 - 10:11 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

My point is that calling something tim_seq tells the reader little if nothing about what it is. If you had called it count2ms or something I could immediately under what it was doing but I think I'd still make the 30000 (or rather 29999) something like 60 * 500 to give a hint to the reader that it is 500 lots of 2ms (ie 1s) and then the 60 * 1s is obvious.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chipz wrote:
How about if I just use: PORTB = 2;

 

That would just set the PORTB to 0b00000010, (PB2 high, all other PB pins low, except input pins would have their pull-up resistors disabled), whereas:

PINB |= 0x2;
or
PINB |= (1<<PB1);

 

will toggle bit 2 of PORTB.

 

Also noticed:

	DDRB = 0x01;
	PORTB = (1<<PB1);

The first line sets PB0 as an output, the second line is (trying) to set PB1 high (it actually is turning on the pull-up resistor for PB1).

I believe (assuming the LED is on PB1) you need:

	DDRB = (1<<PB1);
	PORTB = (1<<PB1);
or 
	DDRB = 0x02;
	PORTB = (1<<PB1);

(I prefer the first.)

David (aka frog_jr)

Last Edited: Wed. Jul 20, 2016 - 01:06 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@clawson.  Okay, thank you for the suggestions.

 

frog_jr wrote:

chipz wrote:
How about if I just use: PORTB = 2;

 

That would just set the PORTB to 0b00000010, (PB2 high, all other PB pins low, except input pins would have their pull-up resistors disabled), whereas:

PINB |= 0x2;
or
PINB |= (1<<PB1);

 

will toggle bit 2 of PORTB.

 

I found the following in the datasheet.  It's a bit confusing, but I accept that it's just the way they behave.

 

Three I/O memory address locations are allocated for each port, one each for the Data Register – PORTx, Data
Direction Register – DDRx, and the Port Input Pins – PINx. The Port Input Pins I/O location is read only, while the
Data Register and the Data Direction Register are read/write. However, writing a logic one to a bit in the PINx Register,
will result in a toggle in the corresponding bit in the Data Register. In addition, the Pull-up Disable – PUD bit
in MCUCR disables the pull-up function for all pins in all ports when set.

Is this intentionally planned into the chip architecture?  It seems counter-intuitive to set an input register bit to toggle a data register bit.  Do you know the reason it was designed like this?

 

Are the two yellow highlighted registers the same register?  That's how I took it, which means it's contradictory, if not for the "however" in blue.

 

Quote:

Also noticed:

	DDRB = 0x01;
	PORTB = (1<<PB1);

The first line sets PB0 as an output, the second line is (trying) to set PB1 high (it actually is turning on the pull-up resistor for PB1).

I believe (assuming the LED is on PB1) you need:

	DDRB = (1<<PB1);
	PORTB = (1<<PB1);
or 
	DDRB = 0x02;
	PORTB = (1<<PB1);

(I prefer the first.)

 

I didn't catch that.  Why did it work (DDRB=0x01)?  Looking at it now I realize it was wrong.  It was intended to do it a lazy way by setting all the bits on DDRB to 1.  I had seen someone do that but \I remembered it incorrectly?  It was rote (no typo) wrong, so I wrote it wrong?  ;)  I would have sworn it was DDRB set equal to 1, but that should have only set the least significant bit to one, no?

 

Earlier you said toggling using the xor approach required 4 instructions vs 1 for the PINB method.  I assume this would be obvious if I knew assembly?  Is there another way I can see the number of instructions being used somewhere in the IDE (sans assembly).  That would be a useful little tool.

 

I really appreciate the pointers.

Last Edited: Wed. Jul 20, 2016 - 03:20 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I usually start my programs with this:

#define F_CPU xxxxxxxxxul
#include <avr/io.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <string.h>
#include <avr/pgmspace.h>

where xxxxxxxxxx is the cpu clock frequency.   make sure you put this first as where it is located, and where delay.h is located is important.  F_CPU MUST be defined BEFORE you include delay.h.

delay.h is for when you want to use  _delay_ms()

then you have the standard I/O, standard library, and standard integer header files.

you may not need string.h or pgmspace.h, but they are handy to have there just in case you plan on accessing constants stored in FLASH, or are working with strings.

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

Last Edited: Wed. Jul 20, 2016 - 06:26 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chipz wrote:
Is this intentionally planned into the chip architecture?
Yes, definitely!
chipz wrote:
Are the two yellow highlighted registers the same register?
No, if you look at a diagram of the I/O logic (page 54 of the ATtiny85 datasheet):

ATtiny85 IO

 

You can see (1-bit of each of) the 3 registers. The physical package pin is shown at the extreme left of the diagram. From the top, I have indicated the DDR, PORT and PIN registers. Of interest is the AND gate (with 2 inverting inputs above left of the DDR) which shows how writing a "1" to the PORT register, when the DDR is configured as input and Pull-ups aren't disabled will turn on the pull-up resistor. Also the WPx (write PIN) signal to the right of the PORT register shows how a write of a "1" to the PIN register address (not the register itself) can toggle the PORT register. (If this is "clear as mud" to you don't worry, it just works!)

 

 

chipz wrote:
Is there another way I can see the number of instructions being used
If you are using Atmel Studio, and the checkbox "Generate lss file" is checked (see Project/Properties/Toolchain/GNU Common), then an .lss file will be generated in the Debug (or Release) directory that will show the C code and the Assembly code generated. Other compiler tools have methods for obtaining this information as well.

 

Edit: Beware of the .lss making a lot of sense depending on how the compiler optimizes the C code.

Edit2: Another thing to notice is that if a pin is configured as an output and it is set high, and then you change the DDR to make the pin an input, the pull-up will be active (assuming pull-ups are enabled via PUD) until a "0" is written to the PORT register.

David (aka frog_jr)

Last Edited: Wed. Jul 20, 2016 - 04:35 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

A more general comment:

 

if you use a variable like tim_seg both inside and outside the ISR it has to be volatile.

I guess that you only use it in the ISR and then it should be local to the ISR (but declared static).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I just want you guys to know I really appreciate the thorough explanations.  Your effort isn't wasted.  It's been a big help.  I can't say I understand it all (especially the logic diagrams) but it's starting to seep in.  "Registers" are a concept.  Each individual "PORTxn" or "PINx" or whatever is also a concept.  Its hard to learn this stuff serially.  You almost have to absorb it all in one big clump, at least that's how I perceive it.  Quite a few times I've had someone here intuit that something I didn't ask for might be useful.  That information has been at least as valuable as the answers I've received to my questions!  @jgmdesign, what can I say?  Seeing the libraries laid out in the header the way you did in your last post... it's silly that something so simple, yet something I hadn't visualized in that way, showing all the most important libraries I should be familiar with, just compartmentalized and broke it down for me.  It helps.

 

@frog_jr, I have a lot of trouble following the logic diagrams.  Simple ones are no problem, but I get lost in the spaghetti diagrams in the datasheet.  I spent about an hour studying the one you posted for me.  Still a long way to go for me, but it went from "clear as mud" to something in the range of a beef broth.  I can't thank you enough.  BTW... just noticed your location... sympathies.  I hope your loved ones are safe.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your code and your description do not match.  The code increments the minutes, tens of minutes, and hours on every single ISR execution except when the LED is toggled.  It seems to me that's exactly the opposite of the desired behavior that you described, wherein minutes etc should only be incremented when the LED toggles once per minute.  Also, you appear to be doing your calculations in BCD format.  That's fine if you're into that sort of thing, but there's really no reason to otherwise.  It's easier to follow program execution if you compute numbers with multiple digits as whole entities and just separate out the individual digits when you're ready to display them.

 

By the way, what's the driver behind 30,000 interrupts per minute?  You should only be taking an action once per minute if I'm correct about your desired behavior, so there's 29,999 do nothing interrupts per one that actually does something.  That seems a bit excessive.

 

Edit:  Nevermind that last part, I just realized you only have the 8 bit timer so the interrupts are gonna be short, my mistake.

Last Edited: Wed. Jul 20, 2016 - 05:49 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Rezer, you're RIGHT!  The minutes and hours should all be incrementing in the else{} block.  That part I didn't test.

 

The reason I went with 2ms is for future button debouncing.  Use one timer for to manage two things.  But the debouncing is why I need the resolution.

 

I was just sitting down to rewrite the code with all the suggestions made here. The next version will just have a single variable, minutes an unsigned int, and then as you say, calculate the output.  Much better approach.  Thanks for the tips

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@rezer, posting this after your <edit>

 

  I never mentioned my intentions so your point is still valid, I could have prescaled to about 4 interrupts/sec had it not been for needing the resolution.