| View previous topic :: View next topic |
| Author |
Message |
jemly
Joined: 13 May 2007
Posts: 34
|
| Newbie Hoping For Code Evaluation! |
 Posted: Fri May 18, 2007 10:57 am |
 |
|
Hi,
I'm originally a C# developer but have recently begun embedded C programming. I have written a small bit of code which produces 2 PWM outputs from a PIC18F452. A switch is pressed to 'cycle' through different pwm duty cycles for each motor.
Basically I am wondering if I've written this badly / well? What could I improve to make the code more efficient? Many thanks in advance for anyone who takes the time to look!
Here's the code:
| Code: |
#include "J:\My Documents\CCS Projects\GNSMotors01\main.h"
#define LED0 PIN_B0
#define TOGGLE_BTN PIN_A4
const long int delayLength = 2000;
unsigned int current_stateX;
unsigned int current_stateY;
short keyPressed;
short keyReleased;
short down = 0;
struct COORD {
long int x;
long int y;
};
struct COORD coords[5] = {
0,0,
256,256,
512,512,
767,767,
1023,1023 };
void led_on()
{
output_high(LED0);
}
void led_off()
{
output_low(LED0);
}
int SwitchPressed(int buttonDown)
{
int buttonPressDone = 0;
//check the switch is low, only return a 1 if
//the switch has gone low again
if(!buttonDown & down == 1)
buttonPressDone = 1;
if (buttonDown)
down = 1;
else
down = 0;
return buttonPressDone;
}
void go_home()
{
set_pwm1_duty (coords[1].x);
set_pwm2_duty (coords[1].x);
}
void SetMotorPositions()
{
//cycle through each coordinate in the pattern
int16 i;
for(i=1; i < 5; i++)
{
set_pwm1_duty (coords[i].x);
current_stateX ++;
set_pwm2_duty (coords[i].y);
current_stateY ++;
//delay before moving to next coordinate
delay_ms(delayLength);
}
go_home();
}
void main()
{
setup_adc_ports(NO_ANALOGS);
setup_adc(ADC_OFF);
setup_psp(PSP_DISABLED);
setup_spi(SPI_SS_DISABLED);
setup_wdt(WDT_OFF);
setup_timer_0(RTCC_INTERNAL);
setup_timer_1(T1_DISABLED);
//timer 2 set up for maximum frequency of 4MHz clock
//4000000 / 1024 3906.25Hz
setup_timer_2(T2_DIV_BY_1,255,1);
setup_timer_3(T3_DISABLED|T3_DIV_BY_1);
setup_ccp1(CCP_PWM);
setup_ccp2(CCP_PWM);
set_pwm1_duty (0);
set_pwm2_duty (0);
current_stateX = 0;
current_stateY = 0;
keyReleased = 0;
while(TRUE) // flash LEDs at 1Hz
{
if(SwitchPressed(!input(TOGGLE_BTN)))
{
SetMotorPositions();
}
}
}
|
|
|
 |
rwyoung
Joined: 12 Nov 2003
Posts: 563
Location: Lawrence, KS USA
|
|
| Posted: Fri May 18, 2007 1:03 pm |
|
|
Four quick comments:
1) The PIC18F452 is a mature product and not recommended for new designs. Consider the PIC18F4520 instead.
2) Debounce your pushbutton either in hardware or with software.
3) Comment your code. It may make sense to you now and this is a relatively simple application but what if you were hit by a bus and somebody else had to start maintaining your source pool?
4) My personal preference is for #use io_fast and setting TRIS register bits myself. Then using #byte or #bit so I can say things like LED = ON where ON has been #defined to 0 (assuming port pin sinks current to turn on LED) or 1 (sourcing current). YMMYV
Otherwise it looks reasonable, have you gotten a clean compile and tested with hardware?
_________________
Rob Young
The Screw-Up Fairy may just visit you but he has crashed on my couch for the last month! |
|
|
rnielsen
Joined: 23 Sep 2003
Posts: 852
Location: Utah
|
|
| Posted: Fri May 18, 2007 2:18 pm |
|
|
| Quote: | if(!buttonDown & down == 1)
|
You are doing a bit-wise And here. I think you might want to put two '&&' instead to do a logical And.
Ronald
_________
Never open up a new can of worms, unless you're going fishing. |
|
|
Neutone
Joined: 08 Sep 2003
Posts: 839
Location: Houston
|
| Re: Newbie Hoping For Code Evaluation! |
| Posted: Fri May 18, 2007 3:35 pm |
|
|
| jemly wrote: | Hi,
I'm originally a C# developer but have recently begun embedded C programming. I have written a small bit of code which produces 2 PWM outputs from a PIC18F452. A switch is pressed to 'cycle' through different pwm duty cycles for each motor.
Basically I am wondering if I've written this badly / well? What could I improve to make the code more efficient? Many thanks in advance for anyone who takes the time to look!
Here's the code:
| Code: |
unsigned int current_stateX;
unsigned int current_stateY;
short keyPressed;
short keyReleased;
short down = 0;
int16 i;
for(i=1; i < 5; i++)
|
|
I would recommend that you use explicit declarations for variables. Terms like short, long and Int are not always the same for 8, 16, 32bit processor types. It helps insure everyone who reads your code knows what size the variables are. You don't have to declare unsigned because it's the default but it is more clear than not declaring it.
For any variable that is only used in the range of a byte, use a byte variable. Where you have i as an Int16 an Int8 would have been better. Compare the list file associated with the for statement with both Int8 and Int16 to see the difference. It is a very small performance improvement but if your using it throughout the code you write, you code would be several % faster and smaller. |
|
|
libor
Joined: 14 Dec 2004
Posts: 288
Location: Hungary
|
|
| Posted: Fri May 18, 2007 3:49 pm |
|
|
A program logic advice.
Though this particular application may not need this, I suggest to get used early to a more event-driven-like, or as another alternative: a state-machine programming style.
When the button is pressed you begin a quite lengthy sequence of actions with plenty of delay loops, 8 secs in total if I got it right, and your program becomes totally irresponsive to any user interaction during this long time.
I understand that this is what you wanted to program now, but it'll get difficult and will result a hard-to-understand code if you want to add e.g. an emergency-stop-button function later (you would call SwitchPressed from many places in your code ?)
What I suggest is to make a separate thread (e.g. using event-driven or timer-based interrupts) for the 'user interface' constantly updating some global variables with the user's wish.
And in the main loop you can act upon the state of these variables whereever you want. |
|
|
jemly
Joined: 13 May 2007
Posts: 34
|
|
| Posted: Mon May 21, 2007 3:34 am |
|
|
Wow thanks for all the time you have all taken, this is the kind of advice I was looking for! I've got a few questions though.
Rwyoung - do you have any example code for debouncing a switch or can you point me to somewhere that I can understand how to do this better? And yes this has been compiled and tested with hardware and it does what I want, I am just concerend that if I'm doing things badly now when the software grows considerably the performance effects will be big.
libor - your comments are really useful. This application may grow quite rapidly and I want to make sure it's efficient now wheteher it needs it or not so that I can be sure I'm not going to have to make any massive changes later. I sort of understand what you are talking about but wondered if you know of any good resources I can look at that will help me understand it better?
I think from what you're saying that I would be better to have the switch trigger an interrupt but I'm not quite clear on how the main loop would be working? Can you clarify this a little? Thanks! |
|
|
rwyoung
Joined: 12 Nov 2003
Posts: 563
Location: Lawrence, KS USA
|
|
| Posted: Mon May 21, 2007 7:20 am |
|
|
Search this forum for switch debouncing.
That said, in software one method is to set up the edge/level interrupt to trigger when the switch is pressed. The interrupt enables a timer which when it exires (say 20ms later, perhaps 40ms for really sloppy switches) will check to see if the switch is still in the depressed condition. A variable is set or cleared to indicate switch condition, the edge/level interrupt is reenabled and the timer reset for its next one-shot count.
Do not make the timer a software one inside the edge/level interrupt. The ISR should be short and sweet.
In hardware you can do lots of different things such as latching the switch edge or level, then clearing it after processing. You can use a debouncing IC (several available). You can use an SR flip-flop. You can use an RC filter. Lots of hardware choices too.
_________________
Rob Young
The Screw-Up Fairy may just visit you but he has crashed on my couch for the last month! |
|
|
rnielsen
Joined: 23 Sep 2003
Posts: 852
Location: Utah
|
|
|
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
