Continue to Site

Welcome to our site!

Electro Tech is an online community (with over 170,000 members) who enjoy talking about and building electronic circuits, projects and gadgets. To participate you need to register. Registration is free. Click here to register now.

  • Welcome to our site! Electro Tech is an online community (with over 170,000 members) who enjoy talking about and building electronic circuits, projects and gadgets. To participate you need to register. Registration is free. Click here to register now.

Function not returning a value

Status
Not open for further replies.

johnl69

Member
Im relatively new to C programming and have been following books and online tutorials but i cant find an answer any where.

Ive written a program to adjust the brightness of an LED, when you hold switch RA4 the program goes to a function to adjust the PWM value "green_val" eveerything works as it should in the function but when I return to the main program the "green_val" registry returns to 0 rather then what it was set to in the function.

C:
#include <stdio.h>
#include <stdlib.h>
#include "pic16f874a_Config.h"
#include "lcd.h"
#include "pic16f874a.h"

#include "delays.h"

#define duty 1


void main(void)
{
  
    /*
         * configure CCP module as 4000 Hz PWM output
         */
PIR1 =255;
T2CON  =0b00000101 ;
CCP1CON  =0b00001100 ;      //Configure PWM Mode, Set DB bits to 10
CCPR1L  =0b00000000 ;       // reset to zero

Init_Pic();
lcd_init();
RC0=1;

  
    unsigned int green_val;


      
    for(;;)                       
        {
            CCPR1L = green_val;
          
          
            lcd_goto(0);
            lcd_puts("green_val1");
      
            if(RA4 == 0)
                {
                lcd_clear();
                Set_Green(green_val);
                }
        }
}


unsigned int Set_Green(unsigned int green_val)
{
      
  
    lcd_goto(0);
    lcd_puts("Green");
    while(RA4 == 0)
        {       // digital adjustment of duty cycle
      
                if(RA5 == 0)//Brightness up
                {
                    if(green_val == 255)
                        {

                        }
                    else
                        {

                        green_val++;

                        lcd_goto(0x40);
                        lcd_putch(green_val);

                        __delay_ms(50);
                      
                        }
                  
                }
                if(RE0 == 0)//Brightness down
                {
                    if(green_val == 0)
                        {
                          
                        }
                    else
                        {
                      
                        green_val--;
                        lcd_goto(0x40);
                        lcd_putch(green_val);

                        __delay_ms(50);
                      
                        }
                }
      
        CCPR1L = green_val;
          
        }
              
    lcd_clear();
    return green_val;
}
 
Last edited:
The function returns the variable...
it should be..
C:
green_val = Set_Green();
If you pass a parameter ie.. Set_Green(green_val); You need to point to it.
C:
unsigned int green_val;

Set_Green(&green_val);

And the function would need no return..
C:
void Set_Green( unsigned int* green_val)
   {
   // adjusting code...
   }
 
Thanks for that not sure if I understood correctly because now im getting "error (984) type redeclared" and "error: (1098) conflicting declarations for variable "Set_Green".

C:
#include <stdio.h>
#include <stdlib.h>
#include "pic16f874a_Config.h"
#include "lcd.h"
#include "pic16f874a.h"

#include "delays.h"

#define duty 1


void main(void)
{
  
    /*
         * configure CCP module as 4000 Hz PWM output
         */
PIR1 =255;
T2CON  =0b00000101 ;
CCP1CON  =0b00001100 ;      //Configure PWM Mode, Set DB bits to 10
CCPR1L  =0b00000000 ;       // reset to zero

Init_Pic();
lcd_init();
RC0=1;

  
    unsigned int green_val;
  
    Set_Green(&green_val);
  


      
    for(;;)                       
        {
            CCPR1L = green_val;
          
          
            lcd_goto(0);
            lcd_puts("green_val1");
      
            if(RA4 == 0)
                {
                lcd_clear();
                Set_Green(&green_val);;
                }
        }
}


void Set_Green( unsigned int* green_val)
{
      
  
    lcd_goto(0);
    lcd_puts("Green");
    while(RA4 == 0)
        {       // digital adjustment of duty cycle
      
                if(RA5 == 0)//Brightness up
                {
                    if(green_val == 255)
                        {

                        }
                    else
                        {

                        green_val++;

                        lcd_goto(0x40);
                        lcd_putch(green_val);

                        __delay_ms(50);
                      
                        }
                  
                }
                if(RE0 == 0)//Brightness down
                {
                    if(green_val == 0)
                        {
                          
                        }
                    else
                        {
                      
                        green_val--;
                        lcd_goto(0x40);
                        lcd_putch(green_val);

                        __delay_ms(50);
                      
                        }
                }


                CCPR1L = green_val;
        }
              
    lcd_clear();
  
}
 
Last edited:
Where is the function definition???

It was:-

unsigned int Set_Green(unsigned int green_val);

It needs to be:-

void Set_Green(unsigned int* green_val);

C:
#include <stdio.h>
#include <stdlib.h>
#include "pic16f874a_Config.h"
#include "lcd.h"
#include "pic16f874a.h"
#include "delays.h"
#define duty 1

void main(void)
{
   unsigned int green_val; 
    /*
         * configure CCP module as 4000 Hz PWM output
         */
    PIR1 =255;
    T2CON  =0b00000101 ;
    CCP1CON  =0b00001100 ;      //Configure PWM Mode, Set DB bits to 10
    CCPR1L  =0b00000000 ;       // reset to zero

    Init_Pic();
    lcd_init();
    RC0=1;  
 
    for(;;)                       
        {
            CCPR1L = green_val;          
            lcd_goto(0);
            lcd_puts("green_val1");      
            if(RA4 == 0)
                {
                lcd_clear();
                Set_Green(&green_val);;
                }
        }
}

void Set_Green( unsigned int* green_val)
{  
    lcd_goto(0);
    lcd_puts("Green");
    while(RA4 == 0)
        {       // digital adjustment of duty cycle
      
                if(RA5 == 0)//Brightness up
                {
                    if(green_val == 255)
                        {
                        }
                    else
                        {
                        green_val++;
                        lcd_goto(0x40);
                        lcd_putch(green_val);
                        __delay_ms(50);                      
                        }                  
                }
                if(RE0 == 0)//Brightness down
                {
                    if(green_val == 0)
                        {                          
                        }
                    else
                        {                      
                        green_val--;
                        lcd_goto(0x40);
                        lcd_putch(green_val);

                        __delay_ms(50);                      
                        }
                }
                CCPR1L = green_val;
        }              
    lcd_clear();  
}
 
I did change the function definition to what you had put and got the errors.

Ive just copied and pasted your code to a new file and get the same errors? I cant see it declared in any of the header files

pic16f84a_config is just pin definitions
C:
#include <stdio.h>
#include <stdlib.h>
#include "pic16f874a.h"
#include "pic16f874a_Config.h"

void Init_Pic(void)
{

    TRISA  = 0xff;
    TRISB  = 0x00;
    TRISC  = 0x00;
    TRISD  = 0x00;
    TRISE  = 0x03;
    PORTB  = 0x00;
    PORTC  = 0x00;
    PORTD  = 0x00;
}
 
Okay Simple.... Just put one at the begining..

C:
#include <stdio.h>
#include <stdlib.h>
#include "pic16f874a_Config.h"
#include "lcd.h"
#include "pic16f874a.h"
#include "delays.h"
#define duty 1

void Set_Green( unsigned int* green_val);

void main(void)
{
   unsigned int green_val; 
    /*
         * configure CCP module as 4000 Hz PWM output
         */
    PIR1 =255;
    T2CON  =0b00000101 ;
    CCP1CON  =0b00001100 ;      //Configure PWM Mode, Set DB bits to 10
    CCPR1L  =0b00000000 ;       // reset to zero

    Init_Pic();
    lcd_init();
    RC0=1;  
 
    for(;;)                       
        {
            CCPR1L = green_val;          
            lcd_goto(0);
            lcd_puts("green_val1");      
            if(RA4 == 0)
                {
                lcd_clear();
                Set_Green(&green_val);;
                }
        }
}

void Set_Green( unsigned int* green_val)
{  
    lcd_goto(0);
    lcd_puts("Green");
    while(RA4 == 0)
        {       // digital adjustment of duty cycle
      
                if(RA5 == 0)//Brightness up
                {
                    if(green_val == 255)
                        {
                        }
                    else
                        {
                        green_val++;
                        lcd_goto(0x40);
                        lcd_putch(green_val);
                        __delay_ms(50);                      
                        }                  
                }
                if(RE0 == 0)//Brightness down
                {
                    if(green_val == 0)
                        {                          
                        }
                    else
                        {                      
                        green_val--;
                        lcd_goto(0x40);
                        lcd_putch(green_val);

                        __delay_ms(50);                      
                        }
                }
                CCPR1L = green_val;
        }              
    lcd_clear();  
}
If this works I have no Idea how your first code compiled correctly...
 
Yep that's true.... I forgot to mention that we are now remote... All operations to "green_val" have to be pointers..
C:
void Set_Green( unsigned int* green_val)
{  
    lcd_goto(0);
    lcd_puts("Green");
    while(RA4 == 0)
        {       // digital adjustment of duty cycle
      
                if(RA5 == 0)//Brightness up
                {
                    if(*green_val == 255)
                        {
                        }
                    else
                        {
                        *green_val++;
                        lcd_goto(0x40);
                        lcd_putch(*green_val);
                        __delay_ms(50);                      
                        }                  
                }
                if(RE0 == 0)//Brightness down
                {
                    if(*green_val == 0)
                        {                          
                        }
                    else
                        {                      
                        *green_val--;
                        lcd_goto(0x40);
                        lcd_putch(*green_val);

                        __delay_ms(50);                      
                        }
                }
                CCPR1L = *green_val;
        }              
    lcd_clear();  
}
 
now the programs not working properly the led goes from on to off with 1 button press rather then the smooth progression I had before and it still reverts to zero once I release the button.
 
It is really a bad idea to use pointers on PIC16 devices, especially that old as yours, unless there's a compelling reason to do so. Just make green_val a global variable instead of passing it to the function as a argument. It's the most efficient and the easiest solution.
 
I would swear that that port in that micro is open collector if set as output. Most, if not all the old ones, had that trap armed for the unwary. Not C conversant myself. In a next life maybe...
 
It is really a bad idea to use pointers on PIC16 devices, especially that old as yours, unless there's a compelling reason to do so. Just make green_val a global variable instead of passing it to the function as a argument. It's the most efficient and the easiest solution.

YES that worked thank you for everyones input.

Im using this pic because I have it and dont want to fork out for a pickit 3 to use the newer chips.
 
It is really a bad idea to use pointers on PIC16 devices, especially that old as yours,
Why?

Passing a pointer is as easy as having a global variable... I try to make newbies learn how to use them... I use structures and pointers to structures even on the little pic12's
 
Just make green_val a global variable instead of passing it to the function as a argument. It's the most efficient and the easiest solution.
I had this return value zero , only fix that worked was this suggestion from NG .
( Note My C sucks ! )
 
Pointers are one of the fundamental parts of C.. They allow parameter lists to be kept to a minimum.. I also agree that using global variable everywhere is favoured by lone programmers..

sprintf( buff, "arg list"); is a classic example.

LCDputs(buff); is another..

I will write the above program using a pointer to show why it works and why it didn't..
 
Okay!!! I've had this before but have forgotten!!!

*green_val++; // <-- Increments the pointer first..
(*green_val)++; // <-- Increments the location..

I was stung by this a couple of years back.... If the operator is out side the brackets it works fine...
*green_val++ is the same as *(green_val++).... Not what I wanted..

Sorry!!! I should have tested the code first!!
 
Status
Not open for further replies.

Latest threads

New Articles From Microcontroller Tips

Back
Top