Hi, this is my first thread in this forum and at the beginning i want to apologize for my English OK, I've just written small program with timer0 and interrupt. I want to toggle one of the LED on board, but...It doesn't work and I really don't know what's wrong with this code. Each LED is lit and nothing more... Could you help with it ? Thank you in advance and best regards :)
#ifdef __USE_CMSIS #include "LPC17xx.h" #endif #include <cr_section_macros.h> #include <NXP/crp.h> // Variable to store CRP value in. Will be placed automatically // by the linker when "Enable Code Read Protect" selected. // See crp.h header for more information __CRP const unsigned int CRP_WORD = CRP_NO_CRP ; // TODO: insert other include files here // TODO: insert other definitions and declarations here int main(void) { LPC_GPIO2->FIODIR |= 1 << 29; LPC_SC->PCONP |= 1 << 1; LPC_SC->PCLKSEL0 |= 1 << 3; LPC_TIM0->TCR |= 0x00000002; LPC_TIM0->MCR |= 0x00000003; LPC_TIM0->MR0 |= 6000000; NVIC_EnableIRQ(TIMER0_IRQn); LPC_TIM0->TCR |= 0x00000001; while(1){} return 0 ; } void TIMER0_IRQHandler (void) { if((LPC_TIM0->IR & 0x01) == 0x01) { LPC_TIM0->IR |= 1 << 0; LPC_GPIO2->FIOPIN ^= 1 << 29; } }
Exactly how do you think your code should work?
When you get an interrupt, you start the conversion. But how can you get an interrupt if you haven't started a conversion?
And you never ever want the ADC interrupt to wait for a full conversion - at that time, you have the processor 100% locked up in a busy loop.
Haven't you already realized that you do not (!) need any ADC interrupt to handle the ADC? It's enough to manually start a conversion, and then in the main loop poll the state of the ADC until the conversion is done. Then, you can read out the converted value. Then you can start a new conversion.
Right now, you are busy with a trial-and-error methodology. It is well known that such a strategy have a very bad convergence speed. It is many times better to first think, and then write code. Then test the code and compare what happens with what you expected to happen. Then figure out potential reasons for the difference between idea and real worl, and apply fixes.
By the way - can you explain to us how you thought a global variable named "i" would be good for storing the result of the latest ADC conversion? You think "i" sends out a message to the reader making it obvious that it represents the current ADC sample?
if((i >= 0) & (i < 512)) LPC_GPIO2->FIOSET = 0x00000001; //1 led else if((i > 512) & (i < 1024)) LPC_GPIO2->FIOSET = 0x00000003; //2 leds else if((i > 1024) & (i < 1536)) LPC_GPIO2->FIOSET = 0x00000007; //3 leds else if((i > 1536) & (i < 2048)) LPC_GPIO2->FIOSET = 0x0000000F; //4 leds else if((i > 2048) & (i < 2560)) LPC_GPIO2->FIOSET = 0x0000001F; //5 leds else if((i > 2560) & (i < 3072)) LPC_GPIO2->FIOSET = 0x0000003F; //6 leds else if((i > 3072) & (i < 3584)) LPC_GPIO2->FIOSET = 0x0000007F; //7 leds else if((i > 3584) & (i <= 4095)) LPC_GPIO2->FIOSET = 0x000000FF; //8 leds
For some reason, I feel I have seen the above code before. Is it yours, or a cut and paste?
Can you tell me what your code should do for the value 1024? You have one test that only catches values below 1024 and then the rest of the tests looks for values larger than 1024. But what about 1024?
What about 1536? Or 2048? Or 2560? Or 3072? or 3548?. And how often will you have a value larger than 4095 since your last test checks if i is below or equal to 4095?
Don't you think that it is enough with one (1) comparison for every range?
if (i < 512) LPC_GPIO2->FIOSET = 1; else if (i < 1024) LPC_GPIO2->FIOSET = 3; else if (i < 1536) LPC_GPIO2->FIOSET = 7; ...
You think that makes the code too simple?
Next thing - what about dividing i with 512 like:
switch ((i >> 9) & 7) { case 0: LPC_GPIO2->FIOSET = 1; break; case 1: LPC_GPIO2->FIOSET = 3; break; case 2: LPC_GPIO2->FIOSET = 7; break; ... }
or even add a lookup table:
const unsigned set_patterns[] = { 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 0xff }; LPC_GPIO2->FIOSET = set_patterns[(i >> 9) & 7];
But another thing - you have code that sets GPIO pins high. Where do you have any code that clears the other GPIO pins? Don't you feel that your code must handle both turning on and turning off of the diodes? Didn't you consider this when you did sit and sketched down the algorithm to use?
OK, I've just corrected my code and now works better. I know how to read ADC register, I make it in main loop but the logic of LEDs is still bad. It works only one way - you talked about it. The LEDs don't turn off but turning on works very well. Any snippet ? And small prompt ? Thank you for all answers and advices... :)
My code:
/* =============================================================================== Name : main.c Author : Version : Copyright : Copyright (C) Description : main definition =============================================================================== */ #ifdef __USE_CMSIS #include "LPC17xx.h" #endif #include <cr_section_macros.h> #include <NXP/crp.h> // Variable to store CRP value in. Will be placed automatically // by the linker when "Enable Code Read Protect" selected. // See crp.h header for more information __CRP const unsigned int CRP_WORD = CRP_NO_CRP ; // TODO: insert other include files here #include <math.h> #include <stdio.h> // TODO: insert other definitions and declarations here int adc_config(void); uint16_t prevVal,curVal,errCnt; uint16_t readADC(); int main(void) { int i; errCnt = 0; const unsigned int led_state[] = { 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 0xff }; LPC_GPIO2->FIODIR = 0x000000FF; adc_config(); while(1) { i = readADC(); //printf("i = %d",i); LPC_GPIO2->FIOSET = led_state[(i >> 9) & 7]; } return 0 ; } int adc_config(void) { LPC_SC->PCONP |= 1 << 12; // power for adc LPC_ADC->ADCR |= (1 << 5) | (1 << 21); // bit for ADC0.5 input and PDN mode is operational LPC_SC->PCLKSEL0 |= 0; //clock for ADC - adcclk = pclk/4 = 3Mhz LPC_PINCON->PINSEL3 |= (1 << 30) | (1 << 31); //activate adc function on pin P1.31 //LPC_ADC->ADINTEN = 1 << 8; //enable interrupts for channel 0 //NVIC_EnableIRQ(ADC_IRQn); } /*void ADC_IRQHandler(void) { }*/ uint16_t readADC() { LPC_ADC->ADCR |= 1 << 24;// start conversion. while(!(LPC_ADC->ADGDR & (1 << 31))); // wait for completion of coversion curVal = (LPC_ADC->ADGDR >> 4) & 0x0fff; if(errCnt >= 3) { prevVal = curVal; } if(abs((curVal - prevVal) > 20)) { curVal = prevVal; errCnt++; } else { errCnt = 0; } prevVal = curVal; return curVal; }
Not sure if you actually used the processor user manual, or if you just looked at any blinky application.
But when you did find the FIOSET register - didn't you then also find any FIOCLR register?
By the way - I see you wrote a hysterese function to reduce problems with noise from the potentiometer. But why did you select a variable name errCnt? Where is the error?
Another thing - why is curVal a global variable? You give it a value the first thing you do in the readADC() function - and then you return the value of curVal - so no need to retain the value of it until next call...
OK, I did it. Now it works good :) LEDs turns off too :) I only added one line:
LPC_GPIO2->FIOCLR =~(led_state[(i >> 9) & 7]);
When I negate table's value and send it to FIOCLR register LEDs turns off. I knew about this register, but as I said I have many habits from AVR...I forgot that, if I want to turn off LEDs I must write "1" to the bits of FIOCLR.
But AVRs also have SET and CLEAR registers for their port bits - don't they?!
In a previous post he mentioned ATmega32 so no, there is no set/clr. There is only a direction , port and input read register (DDR, PORT,PIN)
Alex