I've been working on converting a simple C program and compiling it using SDCC instead of Keil C. The program uses the AT89C51 watchdog timer, the serial port, and blinks an LED. When compiled using Keil, the program works perfectly. The program simply outputs 21 exes ('X') in response to receiving a 'D' from a notebook PC. To compile the program using SDCC, I first converted the AT89C51xDS header file from Atmel so that sfr and sbit lines had the correct syntax. I also toggled the the LED bit using ! rather than ~. Finally, I used packihx to create the hex file.
The program responds to receiving a 'D' from the notebook PC, but it only transmits 8 characters instead of 21.
Any suggestions? Thanks for your help in advance!!
With apologies for the clutter, I've attached the code below:
#define UART_TXD_DELAY_COUNT 10 // transmit delay cnt => 40*6/30MHz = 8mics/cnt for 115200 bps
#define UNIT_TIMER_RELOAD 0x06 //250 mac. cycles/super timer cnt or 50mics res. at 30MHz
#define MAX_WAIT 4400 //4400*50mics=220ms conversion time at 30 mHz
#define LED0_PORT_BIT P2_0 // setup/power and while(1) loop cycle flip-flop LED
#define WATCHDOG_ON // comment this line out to turn off watchdog timer for debugging
#include "AT89C51xD2SDCC.h" // From Atmel: definition for the register names of the AT89C51xD2 chips
unsigned long data g_ul_clock_count=0;
unsigned long data main_clock_count=0;
// Interrupt Vectors and Interrupt Support Functions
// timer 0 super clock count interrupt routine
void timer_0_interrupt(void) interrupt 1 using 3
{
TF0=0; // reset interrupt flag
if(!(~(((unsigned char data *)(&g_ul_clock_count))[3])))
if(!(~(((unsigned char data *)(&g_ul_clock_count))[2])))
if(!(~(((unsigned char data *)(&g_ul_clock_count))[1])))
if(!(~(((unsigned char data *)(&g_ul_clock_count))[0]))) g_ul_clock_count=0;
g_ul_clock_count++;
}
// UART Serial Port Interrupt 4 Function (input: RxD)(output: TxD)
// with SBUF (serial port) array output (256 bytes max) for interrupt 4
void UART_interrupt(void) interrupt 4 using 2
{
unsigned char UART_data=0;
unsigned char array_index=0;
unsigned long delay_index=0;
if(RI==1) // if reception occurs
{
RI=0; // clear reception flag for next reception
UART_data=SBUF; // read receive data
if(UART_data=='D')
{
for(array_index=0; array_index<=20; array_index++)
{
SBUF='X'; // send each byte to UART
delay_index=0;
while(delay_index<UART_TXD_DELAY_COUNT) delay_index++;
}
}
} else TI=0; // if emission occurs, clear emission flag for next emission
#ifdef WATCHDOG_ON
WDTRST=0x1E; // reset watchdog sequence
WDTRST=0xE1; // reset watchdog sequence
#endif
}
// Initialize relevant system devices and defaults
void setup_ThermoMIC(void)
{
// CPU Clock in X2 mode (peripherals are also at X2 or 6 clocks per mac. cyc.)
CKCON0|=0x01;
// UART settings using the internal baud rate generator
PCON|=0x80; // SMOD1=1, selects double baud rate mode for UART in mode 1,2,3
SCON=0x50; // UART in mode 1 (8-bit), REN=1
BDRCON&=0xEC; // BRR=0, SRC=0
BDRCON|=0x0E; // TBCK=1, RBCK=1, SPD=1
BRL=0xF0; // 115200 bps at 30MHz in X2 mode
// Watchdog time-out setup
// For mode X1: 12 clock periods per machine cycle
// For mode X2: 6 clock periods per machine cycle
WDTPRG|=0x07; // (2^(14+0x07)-1) machine cycles [0x07=MAX] or 0.419s @ Fosc=30MHz & X2
// setup timer 0 in mode 2 with auto-reload & software gate
// to create a 100 microsec unit time count
TMOD&=0xF0; // timer 0 mode 2 with software gate
TMOD|=0x02; // GATE0=0, C/T0#=0, M10=1, M00=0
TH0=UNIT_TIMER_RELOAD; // reload value
TL0=UNIT_TIMER_RELOAD; // init value
IPL0|=0x02; // set timer 0 interrupt priority to 3 (highest)
IPH0|=0x02; // set timer 0 interrupt priority to 3 (highest)
// after setups are done then enable interrupts and start peripherals
ES=1; // enable UART serial port interrupt
ET0=1; // enable timer 0 interrupt
EA=1; // enable all interrupts (global mask)
TR0=1; // run timer 0
SPCON|=0x40; // run SPI
BDRCON|=0x10; // run internal baud rate generator
}
ISRs in sdcc are non-reentrant! To see exactly what is going on check the generated asm code. In your code the variables UART_data, array_index, and delay_index are reset to zero everytime the ISR is activated. So, when a byte is transmited inside the loops of the ISR, a new interrupt is generated and the variables controling the loop are reset to zero messing up everything! Either make the variables static, or even better don't initialize the variables at all. I tested your program with actual hardware with an AT89C51ED2 running at 22 MHZ. It sends 21 'X' everytime I press 'D'. Some changes I made to your program:
1) Used SDCC's <at89c51ed2.h> after fixing a typo in the declaration of CKCON0. I'll be updating the subversion repository shortly.
2) BRL=256-12; // 115200 bps at 22MHz in X2 mode
3) #define UART_TXD_DELAY_COUNT 20 //Because of slower clock
4) Commented out //#define WATCHDOG_ON
5) The most important, I think:
void UART_interrupt(void) interrupt 4 using 2
{
unsigned char UART_data; //Do not initialize!!!
unsigned char array_index; //Do not initialize!!!
unsigned long delay_index; //Do not initialize!!!
[your code here...]
}
(By the way, I don't know exactly what do you want to do with your code, but the ISR looks ugly and dangerous!)
Jesus
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Jesus said:
Either make the variables static, or even better don't initialize the variables at all.
I don't agree at all. Not initializing makes for bad programming practices. They should be static. That's what the keyword was invented for.
Even if all variables in SDCC are already sort-of static with the non-reentrant default behaviour, they can still be overlayed! If you just don't initialize them you still risk losing the contents. You were lucky SDCC knows interrupt local variables should not be overlayed, but using it inside a function called from the ISR could easily put you in trouble.
Maarten
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've just now had a chance to try your suggestions. But the program still does not work for me.
You mentioned that you used SDCC's <at89c51ed2.h> after fixing a type in the declaration of CKCON0. Where can I find SDCC's version <at89c51ed2.h>, and what is the typo that you fixed in the declaration of CKCON0?
Thanks!
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I wrote CKCKON0 (incorrect) instead of CKCON0 (correct). Same for CKCKON1 and CKCON1. You can always get the newest version directly from the subversion repository:
Thanks. I downloaded at89c51ed2.h and modified the #include line in my c source file to include this file name. I also added the statement P2_1=!P2_1; to the interrupt. Every time I transmit a "D" to the 89c51, the LED connected to P2_1 toggles on or off. However, I still get only 8 characters back, and none of them are "X".
I'm sure I'm doing something stupid, but I'm not sure what it is.
Gary
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The program works fine if I change the line #define UART_TXD_DELAY_COUNT 10 to #define UART_TXD_DELAY_COUNT 40 to allow more time between writing each byte to the UART. For some reason, a greater delay between writing bytes is necessary when using the SDCC compiler versus the Keil compiler. Not sure why this is the case!
Gary
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've been working on converting a simple C program and compiling it using SDCC instead of Keil C. The program uses the AT89C51 watchdog timer, the serial port, and blinks an LED. When compiled using Keil, the program works perfectly. The program simply outputs 21 exes ('X') in response to receiving a 'D' from a notebook PC. To compile the program using SDCC, I first converted the AT89C51xDS header file from Atmel so that sfr and sbit lines had the correct syntax. I also toggled the the LED bit using ! rather than ~. Finally, I used packihx to create the hex file.
The program responds to receiving a 'D' from the notebook PC, but it only transmits 8 characters instead of 21.
Any suggestions? Thanks for your help in advance!!
With apologies for the clutter, I've attached the code below:
#define UART_TXD_DELAY_COUNT 10 // transmit delay cnt => 40*6/30MHz = 8mics/cnt for 115200 bps
#define UNIT_TIMER_RELOAD 0x06 //250 mac. cycles/super timer cnt or 50mics res. at 30MHz
#define MAX_WAIT 4400 //4400*50mics=220ms conversion time at 30 mHz
#define LED0_PORT_BIT P2_0 // setup/power and while(1) loop cycle flip-flop LED
#define WATCHDOG_ON // comment this line out to turn off watchdog timer for debugging
#include "AT89C51xD2SDCC.h" // From Atmel: definition for the register names of the AT89C51xD2 chips
unsigned long data g_ul_clock_count=0;
unsigned long data main_clock_count=0;
// Interrupt Vectors and Interrupt Support Functions
// timer 0 super clock count interrupt routine
void timer_0_interrupt(void) interrupt 1 using 3
{
TF0=0; // reset interrupt flag
if(!(~(((unsigned char data *)(&g_ul_clock_count))[3])))
if(!(~(((unsigned char data *)(&g_ul_clock_count))[2])))
if(!(~(((unsigned char data *)(&g_ul_clock_count))[1])))
if(!(~(((unsigned char data *)(&g_ul_clock_count))[0]))) g_ul_clock_count=0;
g_ul_clock_count++;
}
// UART Serial Port Interrupt 4 Function (input: RxD)(output: TxD)
// with SBUF (serial port) array output (256 bytes max) for interrupt 4
void UART_interrupt(void) interrupt 4 using 2
{
unsigned char UART_data=0;
unsigned char array_index=0;
unsigned long delay_index=0;
if(RI==1) // if reception occurs
{
RI=0; // clear reception flag for next reception
UART_data=SBUF; // read receive data
if(UART_data=='D')
{
for(array_index=0; array_index<=20; array_index++)
{
SBUF='X'; // send each byte to UART
delay_index=0;
while(delay_index<UART_TXD_DELAY_COUNT) delay_index++;
}
}
} else TI=0; // if emission occurs, clear emission flag for next emission
#ifdef WATCHDOG_ON
WDTRST=0x1E; // reset watchdog sequence
WDTRST=0xE1; // reset watchdog sequence
#endif
}
// Initialize relevant system devices and defaults
void setup_ThermoMIC(void)
{
// CPU Clock in X2 mode (peripherals are also at X2 or 6 clocks per mac. cyc.)
CKCON0|=0x01;
// UART settings using the internal baud rate generator
PCON|=0x80; // SMOD1=1, selects double baud rate mode for UART in mode 1,2,3
SCON=0x50; // UART in mode 1 (8-bit), REN=1
BDRCON&=0xEC; // BRR=0, SRC=0
BDRCON|=0x0E; // TBCK=1, RBCK=1, SPD=1
BRL=0xF0; // 115200 bps at 30MHz in X2 mode
// Watchdog time-out setup
// For mode X1: 12 clock periods per machine cycle
// For mode X2: 6 clock periods per machine cycle
WDTPRG|=0x07; // (2^(14+0x07)-1) machine cycles [0x07=MAX] or 0.419s @ Fosc=30MHz & X2
// setup timer 0 in mode 2 with auto-reload & software gate
// to create a 100 microsec unit time count
TMOD&=0xF0; // timer 0 mode 2 with software gate
TMOD|=0x02; // GATE0=0, C/T0#=0, M10=1, M00=0
TH0=UNIT_TIMER_RELOAD; // reload value
TL0=UNIT_TIMER_RELOAD; // init value
IPL0|=0x02; // set timer 0 interrupt priority to 3 (highest)
IPH0|=0x02; // set timer 0 interrupt priority to 3 (highest)
// after setups are done then enable interrupts and start peripherals
ES=1; // enable UART serial port interrupt
ET0=1; // enable timer 0 interrupt
EA=1; // enable all interrupts (global mask)
TR0=1; // run timer 0
SPCON|=0x40; // run SPI
BDRCON|=0x10; // run internal baud rate generator
}
// watchdog restart sequence
void restart_watchdog(void)
{
#ifdef WATCHDOG_ON
WDTRST=0x1E;
WDTRST=0xE1;
#endif
}
// superclock count reset
void reset_superclock(void)
{
ET0=0; // disable timer 0 interrupt
g_ul_clock_count=0;
ET0=1; // enable timer 0 interrupt
}
// reset program clock count vars
void reset_clock_counts(void)
{
main_clock_count=0;
}
///////////////////////////////////////////////////////////////////////////////
// Main entry point of looping controller program
void main(void)
{
unsigned char indexer=0;
unsigned char check_flag=0;
// setup functions
setup_ThermoMIC();
reset_superclock();
restart_watchdog();
LED0_PORT_BIT=~1;
// primary loop
while(g_ul_clock_count<main_clock_count) { restart_watchdog(); } // wait if early
main_clock_count=g_ul_clock_count+MAX_WAIT;
while(1)
{
LED0_PORT_BIT=!LED0_PORT_BIT;
if(g_ul_clock_count>0x7FFFFFFF)
{
reset_superclock();
reset_clock_counts();
}
while(g_ul_clock_count<main_clock_count) { restart_watchdog(); } // wait if early
main_clock_count=g_ul_clock_count+MAX_WAIT;
}
}
ISRs in sdcc are non-reentrant! To see exactly what is going on check the generated asm code. In your code the variables UART_data, array_index, and delay_index are reset to zero everytime the ISR is activated. So, when a byte is transmited inside the loops of the ISR, a new interrupt is generated and the variables controling the loop are reset to zero messing up everything! Either make the variables static, or even better don't initialize the variables at all. I tested your program with actual hardware with an AT89C51ED2 running at 22 MHZ. It sends 21 'X' everytime I press 'D'. Some changes I made to your program:
1) Used SDCC's <at89c51ed2.h> after fixing a typo in the declaration of CKCON0. I'll be updating the subversion repository shortly.
2) BRL=256-12; // 115200 bps at 22MHz in X2 mode
3) #define UART_TXD_DELAY_COUNT 20 //Because of slower clock
4) Commented out //#define WATCHDOG_ON
5) The most important, I think:
void UART_interrupt(void) interrupt 4 using 2
{
unsigned char UART_data; //Do not initialize!!!
unsigned char array_index; //Do not initialize!!!
unsigned long delay_index; //Do not initialize!!!
[your code here...]
}
(By the way, I don't know exactly what do you want to do with your code, but the ISR looks ugly and dangerous!)
Jesus
Jesus,
Thanks for your suggestions. I'll try them out.
The ISR is not mine. I'm attempting to use someone else's code. I'll check out some examples and attempt to clean up the code.
Thanks for the help!
Gary
Jesus said:
Either make the variables static, or even better don't initialize the variables at all.
I don't agree at all. Not initializing makes for bad programming practices. They should be static. That's what the keyword was invented for.
Even if all variables in SDCC are already sort-of static with the non-reentrant default behaviour, they can still be overlayed! If you just don't initialize them you still risk losing the contents. You were lucky SDCC knows interrupt local variables should not be overlayed, but using it inside a function called from the ISR could easily put you in trouble.
Maarten
Jesus,
I've just now had a chance to try your suggestions. But the program still does not work for me.
You mentioned that you used SDCC's <at89c51ed2.h> after fixing a type in the declaration of CKCON0. Where can I find SDCC's version <at89c51ed2.h>, and what is the typo that you fixed in the declaration of CKCON0?
Thanks!
I wrote CKCKON0 (incorrect) instead of CKCON0 (correct). Same for CKCKON1 and CKCON1. You can always get the newest version directly from the subversion repository:
http://sdcc.svn.sourceforge.net/viewvc/sdcc/trunk/sdcc/device/include/mcs51/
Jesus,
Thanks. I downloaded at89c51ed2.h and modified the #include line in my c source file to include this file name. I also added the statement P2_1=!P2_1; to the interrupt. Every time I transmit a "D" to the 89c51, the LED connected to P2_1 toggles on or off. However, I still get only 8 characters back, and none of them are "X".
I'm sure I'm doing something stupid, but I'm not sure what it is.
Gary
Jesus,
The program works fine if I change the line #define UART_TXD_DELAY_COUNT 10 to #define UART_TXD_DELAY_COUNT 40 to allow more time between writing each byte to the UART. For some reason, a greater delay between writing bytes is necessary when using the SDCC compiler versus the Keil compiler. Not sure why this is the case!
Gary