Working code Timer1 interrupt & debounce & timer del

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

Hi

3 weeks ago I started with atmel for the first time. First time with studio & winavr. This is my first c code for a micro. After some very long nights of reading & trying all the tools software & hardware I seemed to achieved the goal in this real live project ( due in 2 weeks fully installed).

Theres a working timer1 interrupt, there's debouncing all pins of a port or many ports for that matter. Also delay routine using interrupt.

The code may not be optimised or may be non professional, nevertheless it will get me over the mark for now.

Please comment on it, I really need improve in any way.

Attachment(s): 

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

Some comments:

Layout.
1. You use a strange tabsize of 2. I had to experiment.
2. blank lines are handy to separate blocks or functions. You have a few extra ones where they seem unnecessary.
3. You could remove a few redundant comments.
4. Try to layout your code so that you can read the basic logic of a function in one screen window. This may involve creating a macro or subfunction that is only called once.
5. Use descriptive names for each function. Be terse when obvious.

Code.
6. You have auto variables declared in the middle of your ISR. GCC may allow this but C should not.
7. You have a single source file with lots of static variables. These are accessible in the whole of this file. They would not be accessible to another source file. You may as well keep them global unless you specifically want to hide them from the rest of your module. If you want them hidden from other functions, then declare as statics inside your function (at the beginning).
8. You could define a macro for handling your output bits. So you could say OFF_E(WalkPole1_PE2) instead of the explicit bitmasks. This is a question of personal style that you might find readable. (Save a comment too).

These comments are my own personal opinion. Many people will disagree.

David.

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

I wouldn't have used 'i' for a variable the way you have, it is usually used for counters. Something more meaningful would be better.

Switch-case would be better than several ifs.

Leon

Leon Heller G1HSM

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

Thanks David
1. Changed tab to 4
2. Removed many extra lines
3. Removed redundant comments. Only I noticed how many there are.
4. Do you mean that main should have a function call such as "entrycam();"?
5. I will shave off some comments or make it smaller.
6. Variables moved to top.
7. Variable such as "static volatile uint8_t entrycam = 0;. Should that be without static?. I have no reason to hide any of it.
8. Not sure how to do this. If i want

PORTE |= (1 << GoGreen_PE5);

how would that be done?

thanks for taking the time.

Hi Leon
"i" has been changed. Thanks

Regards
Richard

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

Re:4 Yes say if (entryCam > 0) entrycam = svc_entrycam();

Re:7 Just remove the static qualifier. The variable then becomes global. Most debuggers are much more visible with globals.
If you want to hide the variable, then declare as static inside the function and return its value from the function. (known as data hiding).

Re:8
// assuming that you have active low LED or relay.
#define OFF_E(bitno) PORTE |= (1<<bitno)
#define ON_E(bitno) PORTE &= ~(1<<bitno)

This is all a matter of style. Which styles are you happier with?
Reducing the logic of your main function to a few lines of code is known as top down design. The extra function calls lose a little efficiency but will make no significant difference to your execution speed.
It is not wise to do this in an ISR. Generally these should be designed to be lean and mean. Your ISR code is fine.

David.

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

Cool David Thanks for that.

I remember reading your reply to some other thread about isr being mean & lean.

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

The quote was from clawson.

Is it not 2AM in Australia at the moment?

David.

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

Its now 11;35 pm
Australia is playing england in the rugby world cup

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

richard@adam.com.au wrote:
Australia is playing england in the rugby world cup

Oh? How did that turn out? :lol:

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

Quote:
Australia is playing england in the rugby world cup

Oh? How did that turn out?


Wicked!

Quote:

//re-set all outputs to off then let the while delay set the outputs
PORTE &= ~(1 << WalkPole1_PE2); //WalkPole1 OFF;red walkman1 on pole 1
PORTE &= ~(1 << WalkRamp_PE3); //WalkRamp OFF;red walkman on ramp at walkman2
PORTE &= ~(1 << WalkCorridor_PE4);//WalkCorridor OFF;red walkman 3,4,5,6 in corridor on
PORTE &= ~(1 << GoGreen_PE5); //GoGreen OFF;all 3 traffic lights switch to red

PORTC |= (1 << WrongWay_PC0); //WrongWay OFF;check phase 2. Ok to pass
PORTC |= (1 << NotSafe_PC1); //NotSafe OFF;Info sign for prince exit cars. Safe
PORTC |= (1 << BoomOpen_PC2); //BoomOpen_PC2 OFF;boom gate close
PORTC |= (1 << SwingOpen_PC3); //SwingOpen OFF; swing gate close
PORTC |= (1 << NoRightTurn_PC4); //NoRightTurn oFF;right turn permitted on pole 1

You are probably setting enough bits here that you can update the whole byte as one. Is this a state or an action? then something like
"PORTC = NORTH_TRAFFIC_LEFT_TURN"
might make the code clearer, and easier to maintain.

General thoughts:-
Nice balance of code to comments.

isr/signal comment confusing. I think ISR should work with the compiler version you have. Comments MUST be up to date!

Don't leave commented out code around too long!

Tabs of 2 spaces are nice (IMHO). Make them spaces in your editor too. And make sure all people on the development team have the same editor settings, otherwise it becomes hard to diff changes.

Add a History: section to the file header

On comments, //at the end of a single line

/*
 * multi line comment
 * (block comment) elsewhere
 */

indent your comments to match the code

These are all personal opinions. We all develop a style eventually (individual or Company influenced), and maintaining a consistent approach is good.

Cheers.

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

You have several macros of the form:

#define WrongWay_PC0 PC0

But the only way that you ever use them is

...(1 << WrongWay_PC0)...

So why not just define them that way:

#define WrongWay_PC0 ( << PC0  )

Then you can consolidate:

PORTC |= (1 << BoomOpen_PC2); 
 PORTC |= (1 << SwingOpen_PC3);
 PORTC |= (1 << NoRightTurn_PC4);

to:

PORTC |=  BoomOpen_PC2 | SwingOpen_PC3 | NoRightTurn_PC4);

In fact, I would even remove the "_PC2", etc, from the macro names. They really serve no purpose, and if at sometime you want to change the physical layout, you'll have to go back and change all the names.

You notice that most of the suggestions are on the form of the code rather than the function. This is because the code is so unreadable that we haven't gotten to the function yet.

Regards,
Steve A.

The Board helps those that help themselves.

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

Hey guys

I made the changes. major changes
Check it out.
Thanks for your input

ISR(TIM1_COMPA_vect) // this does not work . Any ideas?
SIGNAL (SIG_OUTPUT_COMPARE1A)//this works

Regards
Richard

Attachment(s): 

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

Some countries did not do so well in the Rugby.
I have a sweepstake on Fiji. But it wold be nice if England wins.

Further comments;
1. I still struggled with your formatting. I just re-formatted. The important thing is what YOU see in your editor.
2. Use descriptive names for phase1(). Why not entry_phase() ?
3. Use macro names for your sensor bits. A common convention is to use UPPERCASE names for macros, lowercase or mixed case names for functions. You will have a neat list in one place. Any hardware changes will get edited in one place. Your ISR will have lines like:

    if (PortA_input & ENTRY_DETECT) entrycam--;
    if (PortA_input & PRINCE_DETECT) princecam--;
    ...
    PortA_input = 0;
    }

These points are all a matter of style. I still have not bothered to look at your program logic. However I feel that it must look more readable now that it is split into smaller blocks.

You will regret asking for comments.

David.

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

Hi David

3 big names are out the comp. uk, au,nz. If fiji play as they did with wales then they can create waves.

It makes more sense to use descriptive names.I wasen't too sure how to do it then but i now know.

I made the changes purely because of the comments & suggestions. I think it has improved since then.

Regards
Richard

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

The boys from Fiji did well. It was a brilliant game, even if I have lost the sweepstake.

David.