 |
 |
| View previous topic :: View next topic |
| Author |
Message |
spom
Joined: 12 Jul 2007
Posts: 32
|
| ideas for code optimization? |
 Posted: Tue Mar 18, 2008 3:39 am |
 |
|
Hi,
I have a question according to an optimization of a code.
I use a PIC 18F2480, AD7790 as an A/D converter and a L293D as a motor driver.
The problem is that it happens very often that the program stops during reading datas and the current is still high. I suppose that the problem is in the code for reading datas.
Any ideas for improvement?
Thanks.
My code:
| Code: | #include <18f2480.h>
#device *=16
#device ADC=10
#use delay(clock=20000000)
#fuses HS,NOWDT,NOPROTECT,NOLVP
#use rs232(baud=9600,xmit=PIN_C6,rcv=PIN_C7,BRGH1OK,stream=debug)
#define SPI_MODE_0_0 (SPI_L_TO_H | SPI_XMIT_L_TO_H)
#define SPI_MODE_0_1 (SPI_L_TO_H)
#define SPI_MODE_1_0 (SPI_H_TO_L)
#define SPI_MODE_1_1 (SPI_H_TO_L | SPI_XMIT_L_TO_H)
reading()
{
#byte PORTC = 0xf82
#bit RC0 = PORTC.0
#bit RC1 = PORTC.1
#bit RC2 = PORTC.2
#bit RC3 = PORTC.3
#bit RC4 = PORTC.4
#bit RC5 = PORTC.5
#bit RC6 = PORTC.6
#bit RC7 = PORTC.7
int8 ad_in1;
int8 ad_in2;
int16 ad_in3;
RC2 = 0; //CS to low
delay_us(20);
spi_write(0x3C);
delay_us(20);
while(RC4 == 1)
{
//wait for DOUT low
};
delay_us(20);
RC3 = 1; //SCLK to high
ad_in1 = spi_read(0xFF);
ad_in2 = spi_read(0xFF);
if (ad_in1 >= 128)
{
ad_in1 = ad_in1 - 128;
};
ad_in3 = make16(ad_in1,ad_in2);
while(RC4 == 0)
{
//wait for DOUT high
}
delay_us(20);
RC2 = 1; //CS to high
delay_us(50);
}
#BYTE port_b = 0xf81
BYTE const POSITIONS[8] = {0b0101,
0b0001,
0b1001,
0b1000,
0b1010,
0b0010,
0b0110,
0b0100};
drive_stepper(BYTE speed, char dir, int16 steps)
{
static BYTE stepper_state = 0;
int16 i;
for(i=0; i<steps; ++i) {
delay_ms(speed);
reading();
set_tris_b(0xf0);
port_b = POSITIONS[ stepper_state ];
if(dir!='R')
stepper_state=(stepper_state+1)&(sizeof(POSITIONS)-1);
else
stepper_state=(stepper_state-1)&(sizeof(POSITIONS)-1);
}
}
void main()
{
#byte PORTC = 0xf82
#bit RC0 = PORTC.0
#bit RC1 = PORTC.1
#bit RC2 = PORTC.2
#bit RC3 = PORTC.3
#bit RC4 = PORTC.4
#bit RC5 = PORTC.5
#bit RC6 = PORTC.6
#bit RC7 = PORTC.7
BYTE speed;
int16 steps;
char dir;
setup_spi(spi_master |SPI_MODE_1_1 | spi_clk_div_64 | SPI_SAMPLE_AT_END );
while(TRUE)
{
speed = 50;
steps = 520;
dir='R';
drive_stepper(speed,dir,steps);
output_b(0x00);
delay_ms(1000);
speed = 50;
steps = 520;
dir='F';
drive_stepper(speed,dir,steps);
output_b(0x00);
}
}
|
|
|
 |
jma_1
Joined: 08 Feb 2005
Posts: 147
Location: Wisconsin
|
|
| Posted: Tue Mar 18, 2008 6:50 am |
|
|
I'm assuming this is a test program. The 'f' and 'r' appear to only use the last two states of the position array 0b0110 and 0b0100. Without the hardware connections the code is hard to troubleshoot and see the big picture on how things are connected and your intent.
The SPI read in readings() is never used. Are the successive SPI readings failing? Perhaps delay between the two.
Try explicitly disabling interrupts. Also, try including the 'errors' in the RS232 setup. I cannot tell if you are using RS232 or not based on your sample program.
I would figure out exactly where the program stops. Perhaps RC4 is not changing states as you expect and the program halts waiting for the operation.
Perhaps a simpler test program would be appropriate. |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Tue Mar 18, 2008 9:27 am |
|
|
| jma_1 wrote: | | I'm assuming this is a test program. The 'f' and 'r' appear to only use the last two states of the position array 0b0110 and 0b0100. |
Are you sure about this? To me it looks like all states are used.
| jma_1 wrote: | | The SPI read in readings() is never used. |
I don't understand what you want to say here. There are two calls to spi_read(), both returned values are being used.
| Code: | | setup_spi(spi_master |SPI_MODE_1_1 | spi_clk_div_64 | SPI_SAMPLE_AT_END ); |
It's unusual to see the SPI_SAMPLE_AT_END being used. This is a Microchip addition to the SPI 'standard' and I'm quiet sure Analog Devices did not intend their A/D-converter only to be used in combination with PIC processors. Your program should be more stable removing this directive.
| Code: | #byte PORTC = 0xf82
#bit RC0 = PORTC.0
#bit RC1 = PORTC.1
#bit RC2 = PORTC.2
#bit RC3 = PORTC.3
#bit RC4 = PORTC.4
#bit RC5 = PORTC.5
#bit RC6 = PORTC.6
#bit RC7 = PORTC.7 |
I don't like this way of coding:
1) You have the same table appear in your code twice (difficult for maintenance).
2) Difficult to port. On another processor the PORTC address might be different.
3) You forgot (?) to set the TRIS register and #use fast_io. This is a bug!
Using the CCS supplied functions (output_low & output_high) you don't have these problems.
Just a small programming tip: | Code: | | stepper_state=(stepper_state+1)&(sizeof(POSITIONS)-1); |
This looks very nice, you are not using hard coded numbers. But it is dangerous as your algorithm only works for state tables which have a multiple of 2 size (2, 4, 8, 16, etc). This can lead to very hard to find bugs.
Try instead coding like: | Code: | | stepper_state=(stepper_state+1) % (sizeof(POSITIONS)); |
Now it is more clear what you intend to do and the code also works for all table sizes. The compiler is smart enough to replace the '% 8' command by an '& 7'. |
|
|
Neutone
Joined: 08 Sep 2003
Posts: 839
Location: Houston
|
| Re: ideas for code optimization? |
| Posted: Tue Mar 18, 2008 11:23 am |
|
|
| spom wrote: | Hi,
I have a question according to an optimization of a code.
I use a PIC 18F2480, AD7790 as an A/D converter and a L293D as a motor driver.
The problem is that it happens very often that the program stops during reading datas and the current is still high. I suppose that the problem is in the code for reading datas.
Any ideas for improvement?
Thanks.
My code:
| Code: | #include <18f2480.h>
#device *=16
#device ADC=10
#use delay(clock=20000000)
#fuses HS,NOWDT,NOPROTECT,NOLVP
#use rs232(baud=9600,xmit=PIN_C6,rcv=PIN_C7,BRGH1OK,stream=debug)
#define SPI_MODE_0_0 (SPI_L_TO_H | SPI_XMIT_L_TO_H)
#define SPI_MODE_0_1 (SPI_L_TO_H)
#define SPI_MODE_1_0 (SPI_H_TO_L)
#define SPI_MODE_1_1 (SPI_H_TO_L | SPI_XMIT_L_TO_H)
reading()
{
#byte PORTC = 0xf82
#bit RC0 = PORTC.0
#bit RC1 = PORTC.1
#bit RC2 = PORTC.2
#bit RC3 = PORTC.3
#bit RC4 = PORTC.4
#bit RC5 = PORTC.5
#bit RC6 = PORTC.6
#bit RC7 = PORTC.7
int8 ad_in1;
int8 ad_in2;
int16 ad_in3;
RC2 = 0; //CS to low
delay_us(20);
spi_write(0x3C);
delay_us(20);
while(RC4 == 1)
{
//wait for DOUT low
};
delay_us(20);
RC3 = 1; //SCLK to high
ad_in1 = spi_read(0xFF);
ad_in2 = spi_read(0xFF);
if (ad_in1 >= 128)
{
ad_in1 = ad_in1 - 128;
};
ad_in3 = make16(ad_in1,ad_in2);
while(RC4 == 0)
{
//wait for DOUT high
}
delay_us(20);
RC2 = 1; //CS to high
delay_us(50);
}
#BYTE port_b = 0xf81
BYTE const POSITIONS[8] = {0b0101,
0b0001,
0b1001,
0b1000,
0b1010,
0b0010,
0b0110,
0b0100};
drive_stepper(BYTE speed, char dir, int16 steps)
{
static BYTE stepper_state = 0;
int16 i;
for(i=0; i<steps; ++i) {
delay_ms(speed);
reading();
set_tris_b(0xf0);
port_b = POSITIONS[ stepper_state ];
if(dir!='R')
stepper_state=(stepper_state+1)&(sizeof(POSITIONS)-1);
else
stepper_state=(stepper_state-1)&(sizeof(POSITIONS)-1);
}
}
void main()
{
#byte PORTC = 0xf82
#bit RC0 = PORTC.0
#bit RC1 = PORTC.1
#bit RC2 = PORTC.2
#bit RC3 = PORTC.3
#bit RC4 = PORTC.4
#bit RC5 = PORTC.5
#bit RC6 = PORTC.6
#bit RC7 = PORTC.7
BYTE speed;
int16 steps;
char dir;
setup_spi(spi_master |SPI_MODE_1_1 | spi_clk_div_64 | SPI_SAMPLE_AT_END );
while(TRUE)
{
speed = 50;
steps = 520;
dir='R';
drive_stepper(speed,dir,steps);
output_b(0x00);
delay_ms(1000);
speed = 50;
steps = 520;
dir='F';
drive_stepper(speed,dir,steps);
output_b(0x00);
}
}
|
|
if RC2 is CS declare it as such and use that in your code. It helps readability and reduces errors.
RC3 = 1; //SCLK to high
If you need this line you probably have the port setup wrong. |
|
|
jma_1
Joined: 08 Feb 2005
Posts: 147
Location: Wisconsin
|
|
| Posted: Tue Mar 18, 2008 12:38 pm |
|
|
ckielstra,
I will look again at the states. I believe I did not see stepper_state is assigned inside the for loop. 
The spi_read(0xFF) return values are assigned to a local variable. The make16 function takes both as arguments and assigns this to ad_in3. However ad_in3 is also a local variable and never used. I was just pointing out I did not understand why you would do this unless the code is merely one small part of something larger (which might have been removed).
Cheers |
|
|
spom
Joined: 12 Jul 2007
Posts: 32
|
|
| Posted: Thu Mar 20, 2008 6:15 am |
|
|
Thanks for your reply!
I took some changes. But now the problem is that after about 10 steps of the motor the current goes low and the main programm starts again. Is this a fault in software or hardware? |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Thu Mar 20, 2008 11:15 am |
|
|
Are you really sure the PIC is resetting? How do you know this?
A PIC is a very simple processor with almost no error detection hardware inside. If it is resetting than the most likely source is a power supply voltage problem.
Stepper motors use large currents and are a well known source of hardware problems.
Do you have all the decoupling capacitors, etc. in place?
Often overlooked is the ground wiring. Make sure the return current of the motors is not running through the same wire your PIC is using as ground, use a star-like connection instead. |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
