Function not returning a value

Status
Not open for further replies.
J

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...
 

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.
 
NorthGuy said:
It is really a bad idea to use pointers on PIC16 devices, especially that old as yours,
Click to expand...
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
 
NorthGuy said:
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.
Click to expand...
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.

Similar threads

K
  • Question
Better Use of Function Pointers
Replies
11
Views
3K
danadak
  • Question
Assigning a value to a variable in Assembly.
Replies
11
Views
3K
Peet19
  • Locked
  • Question
Transferring the value of a register to a variable.
Replies
1
Views
1K
Nigel Goodwin
  • Question
Can Oshonsoft use 'byref' in a FUNCTION? (Plus examples of CHAR MATCHING CODE)
3 4 5
Replies
92
Views
13K
camerart
M
  • Question
Lcd display does not show any text.
2
Replies
31
Views
2K
mouse22
M
Menu
Log in
Register
Cookies are required to use this site. You must accept them to continue using the site. Learn more…