• 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:

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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...
   }
 

johnl69

Member
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:

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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();  
}
 

johnl69

Member
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;
}
 

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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...
 

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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();  
}
 

johnl69

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

NorthGuy

Well-Known Member
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.
 

atferrari

Well-Known Member
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...
 

johnl69

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

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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
 

granddad

Well-Known Member
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 ! )
 

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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..
 

Ian Rogers

User Extraordinaire
Forum Supporter
Most Helpful Member
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

EE World Online Articles

Loading
Top