XC8 - how can I pass bits to and from a function

[Delta]

I am currently reading and debouncing a pushbutton input inside my ISR (a 1ms timer tick).
Here is my code - it works well, although I'm sure it's not the most elegant or efficient method.
SW1_PIN is #defined to a port pin
pb1 and pb1_edge are global bit variables
debounce_timer is a gloabl unsigned char

Code: [Select]

      // read switch inputs
    if (SW1_PIN == 0) // if pin pulled down by button press
      {
      debounce_timer++;
      if (debounce_timer == DEBOUNCE_TIME)
        {
        if (!pb1) // if this is the the transit..
          {
          pb1_edge = 1 ; // then set the edge bit -it will be cleared elsewhere
          }
        pb1=1;
        debounce_timer--;
        }
             
      }
    else
      {
      debounce_timer = 0 ;
      pb1=0 ;
      }
     
Now I would like to wrap this up in a function, so I can call it from the ISR multiple times when I add additional pushbuttons to the project.  I'd like to be able to be able to call it something like:

Code: [Select]

// read and debounce button 1
debounce(*pointer_to_pb1_bit *pointer_to_pb1_edge_bit *pointer_to_pb1_debounce_timer)

However I think XC8 does not allow pointers to bits?  Is there a way to do this?

I know I could just copy/paste the code and change it for each button, but I'm trying to learn good practices...

[Delta]

Hmmmmm, http://www.xargs.com/pic/c-faq.html#pinarray says that you can't use pointers to bit variables, so is there no way to tell a function which bits you want it to modify?

[c4757p]

No, there isn't. How would a pointer to a bit variable even work?

You can, of course, do things like passing in the number of the bit or a bit-mask. For instance instead of trying to pass in a "pointer" to variable.bit, pass &variable and (1<<bit).

[Delta]

No, there isn't. How would a pointer to a bit variable even work?

You can, of course, do things like passing in the number of the bit or a bit-mask. For instance instead of trying to pass in a "pointer" to variable.bit, pass &variable and (1<<bit).

Erm, the same as a pointer to any other variable? 
Surely it just needs to tell the function the address of the bit?

[c4757p]

Bits don't have addresses. At least, not ones that can be passed to instructions.

[Delta]

Bits don't have addresses. At least, not ones that can be passed to instructions.

Now I am confused!  (I am a complete C beginner though!)

unsigned char x ; // this will have an address in the PIC's RAM, 0x0A1 for example
bit y ; // this must also have an address in RAM, shurely? 0xA2.7 for example?

Am I not grasping something basic here? 

[c4757p]

No, not necessarily. Variables often don't have any address in RAM unless they need to - unless you take the address of a variable and give it to something, the compiler's likely to just stuff the data in a register and leave it at that.

If the compiler needs to allocate space for a "bit" variable, it'll allocate a whole byte anyway and use only one bit, because most platforms are not physically capable of addressing bits. A pointer to that "bit" variable will be a pointer to the byte the compiler used to store it.

Relevant: Data structure alignment

If the compiler you're using (I don't remember, for xc8) allows you to have "bit" variables that reference a single bit within a register, that's a special feature they have added, and will be quite limited - essentially, if the actual bit the variable is supposed to reference is not compile-time constant, it won't work. So a bit as a global in a header is fine, the compiler can see what that is, but a bit as a function parameter is not because you could pass anything into there.

[Delta]

OK, cheers for explaining that.

So to read 2 switches, I've just copy/pasted and made a new function.  So in my "functions.c" file I have the following:

Code: [Select]

#include <xc.h>
#include "defines.h"

extern unsigned long int tick_1ms ;
extern bit pb1,pb1_edge,rolloverflag1 ;
extern unsigned char debounce_timer1 ;

extern bit pb2, pb2_edge ;
extern unsigned char debounce_timer2 ;

void read_pb1(void)
  {
  if (SW1_PIN == 0) // if pin pulled down by button press
      {
      debounce_timer1++;
      if (debounce_timer1 == DEBOUNCE_TIME)
        {
        if (!pb1) // if this is the the transit..
          {
          pb1_edge = 1 ; // then set the edge bit -it will be cleared elsewhere
          }
        pb1=1;
        debounce_timer1--;
        }
             
      }
    else
      {
      debounce_timer1 = 0 ;
      pb1=0 ;
      }
  }

void read_pb2(void)
  {
  if (SW2_PIN == 0) // if pin pulled down by button press
      {
      debounce_timer2++;
      if (debounce_timer2 == DEBOUNCE_TIME)
        {
        if (!pb2) // if this is the the transit..
          {
          pb2_edge = 1 ; // then set the edge bit -it will be cleared elsewhere
          }
        pb2=1;
        debounce_timer2--;
        }
             
      }
    else
      {
      debounce_timer2 = 0 ;
      pb2=0 ;
      }
  }
Then in my timer ISR I call them:

Code: [Select]

// read switch inputs
    read_pb1() ;
    read_pb2() ;
This obviously works fine, and I can of course keep doing this as I add switches, but is there not an elegant way were I can have a single "read_pb" function and call it with arguments to determine which switch to process?

[c4757p]

Yeah, your read_pb function should read the whole port, and then AND that with (1<<pin_number) to get the individual bit.

Code: [Select]

void read_pb(uint8_t bitmask)
{
    bool sw_pin = PORTB & bitmask;
    ....
}

read_pb1() becomes read_pb(1 << 1), and read_pb2() becomes read_pb(1 << 2).

(Note: I don't know if the name PORTB is correct, I haven't done PIC in ages. Should be whatever name gets you the inputs from that port.)

[Delta]

But how would the function "know" which flags (ie pb1_edge or pb2_edge) to set?

[macboy]

Another way to reuse source code is by using a macro. It looks similar to calling a function but works differently behind the scenes. One important difference is no restrictions on what you can 'pass into' it because you are basically passing identifiers (like variable names) for use at compile time, not pointers for use at run time. Each use of the macro results in another instance of the code being created by the compiler, but you have only one copy to maintain in your source file. This results in bigger code size but less stack/memory usage and faster execution than using a function.

[Delta]

Another way to reuse source code is by using a macro. It looks similar to calling a function but works differently behind the scenes. One important difference is no restrictions on what you can 'pass into' it because you are basically passing identifiers (like variable names) for use at compile time, not pointers for use at run time. Each use of the macro results in another instance of the code being created by the compiler, but you have only one copy to maintain in your source file. This results in bigger code size but less stack/memory usage and faster execution than using a function.

That sounds ideal!  Heading to the XC8 user manual right now....  Standby for more stupid questions!

[c4757p]

But how would the function "know" which flags (ie pb1_edge or pb2_edge) to set?

Either you can also pass it a pointer to the flag, or you can arrange the flags in a bitfield the same way as they are in the port (it can then just OR them in, or AND them out).

[Delta]

Hmmm, the XC8 manual only talks about assembler macros, could anyone point me to some info about how I can "macro-ise" my read_pb() function please?

PS.  Thanks very much for all the help so far.

[macboy]

Hmmm, the XC8 manual only talks about assembler macros, could anyone point me to some info about how I can "macro-ise" my read_pb() function please?

PS.  Thanks very much for all the help so far.

A C macro is a special kind of #define. In pic.h you may find this:

Code: [Select]

#define __delay_us(x) _delay((unsigned long)((x)*(_XTAL_FREQ/4000000.0)))Note the parameter x
When the compiler sees this in the source code:

Code: [Select]

__delay_us(450);...it replaces it with this:

Code: [Select]

_delay((unsigned long)((450)*(_XTAL_FREQ/4000000.0)))...and the pre-processor will also calculate the math (450)*(_XTAL_FREQ/4000000.0) into a constant before actual compile.

Assuming that XC8 can use "concatenation" in the way the gcc does it, then your two functions:

Code: [Select]

void read_pb1(void)
  {
  if (SW1_PIN == 0) // if pin pulled down by button press
      {
      debounce_timer1++;
      if (debounce_timer1 == DEBOUNCE_TIME)
        {
        if (!pb1) // if this is the the transit..
          {
          pb1_edge = 1 ; // then set the edge bit -it will be cleared elsewhere
          }
        pb1=1;
        debounce_timer1--;
        }
             
      }
    else
      {
      debounce_timer1 = 0 ;
      pb1=0 ;
      }
  }

void read_pb2(void)
  {
  if (SW2_PIN == 0) // if pin pulled down by button press
      {
      debounce_timer2++;
      if (debounce_timer2 == DEBOUNCE_TIME)
        {
        if (!pb2) // if this is the the transit..
          {
          pb2_edge = 1 ; // then set the edge bit -it will be cleared elsewhere
          }
        pb2=1;
        debounce_timer2--;
        }
             
      }
    else
      {
      debounce_timer2 = 0 ;
      pb2=0 ;
      }
  }Could become:

Code: [Select]

#define read_pb(x) { \
  if (SW ## x ## _PIN == 0) // if pin pulled down by button press \
      {\
      debounce_timer ## x++;\
      if (debounce_timer ## x == DEBOUNCE_TIME)\
        {\
        if (!pb ## x) // if this is the the transit..\
          {\
          pb ## x ## _edge = 1 ; // then set the edge bit -it will be cleared elsewhere\
          }\
        pb ## x=1;\
        debounce_timer ## x--;\
        }\
\
      }\
    else\
      {\
      debounce_timer ## x = 0 ;\
      pb ## x=0 ;\
      }\
  }
The backslashes are necessary since the #define ends at end-of-line otherwise.
You might want to tell the compiler to keep intermediate files to check what code it is concocting from the macro.

[Delta]

That's brilliant info, cheers Macboy!  I think I can just about follow what's going on there...

However...

I have shamelessly copy/pasted your code into my project.  And the compiler doesn't complain about the #define, but it doesn't like it when I "call" the macro.

I get:

Code: [Select]

isr.c:45: error: (195) expression syntax
make[2]: *** [build/default/production/isr.p1] Error 1
make[1]: *** [.build-conf] Error 2
make: *** [.build-impl] Error 2
isr.c:45: error: (300) unexpected end of file
(908) exit status = 1
Line 45 is the last line of the ISR.c file, containing the last "}"

Code: [Select]

#include <xc.h>
#include "defines.h"

// Already declared in main.c, but I think I have to do this here...?
extern unsigned long int tick_1ms ;
extern bit rolloverflag1 ;
extern bit pb1,pb1_edge ;
extern unsigned char debounce_timer1 ;
extern unsigned char debounce_timer2 ;

void interrupt high_priority isr_high_priority (void)
  {
  return; // nothing using high priority yins yet...
  }

void interrupt low_priority isr_low_priority (void)
  {
  if (TMR2IF) // if timer 2 has hit the period register value
    {
    TMR2IF=0; // CLEAR THE FLAG!!!!
    tick_1ms++; // increase ticker
    if (!tick_1ms) // if we're at zero
      {
      rolloverflag1 = 0; // clear the rollover flag
      }
   
      // read switch inputs
     // read_pb1() ;
    // read_pb2() ;
   
    read_pb(1) ;
     
    } // end of timer 2 ISR

 
  return;
  } // end of low priority ISR ## THIS IS LINE 45 ##

The editor also shows a red"!" on line 45.  If I comment out the

Code: [Select]

read_pb(1) ; all is well.

I can't see that any braces have been missed or added, so once again I'm rather confused. 

[Delta]

Once again I have successfully blundered my way through and got it working!    I also now know about .pre files - very handy for me to see what this these crazy-ass compilers do behind the scenes.

First I had to remove the outermost braces around Macboy's code, then it compiled OK, but didn't work.

Looking at the .pre file, I could see that is was only placing the first "if" statement and subsequent bit assignment.  But why?   
I then noticed that the last line of the macro that was successfully expanded had a comment in it.  A comment using the "//" method.  So of course the pre-processor (correct term?) treated EVERYTHING after that as a comment, as the lines were concatenated using "\". 
I redid the comments using "/* blah blah /*" and it worked a treat! 

I've also optimised my debouncing code, so the #define:

Code: [Select]

#define read_pb(x)  \
  if (SW ## x ## _PIN == 0) /* IF pin pulled low */ \
      {\
      if (!pb ## x) /* IF the button is NOT already set... */ \
        {\
        if (debounce_timer ## x == DEBOUNCE_TIME)\
            {\
            pb ## x ## _edge = 1 ; /* set the edge bit */ \
            pb ## x = 1 ; /* set the pb bit */ \
            }\
        debounce_timer ## x ++ ; /* if we reach here, the pin is pulled low, but the timer hasn't reached yet */ \
        }\
        \
      }\
    else\
      {\
      debounce_timer ## x = 0 ;\
      pb ## x=0 ;\
      }\

now beautifully expands to:

Code: [Select]

if (PORTAbits.RA7 == 0) { if (!pb1) { if (debounce_timer1 == 250) { pb1_edge = 1 ; pb1 = 1 ; } debounce_timer1 ++ ; } } else { debounce_timer1 = 0 ; pb1=0 ; } ;
if (PORTAbits.RA6 == 0) { if (!pb2) { if (debounce_timer2 == 250) { pb2_edge = 1 ; pb2 = 1 ; } debounce_timer2 ++ ; } } else { debounce_timer2 = 0 ; pb2=0 ; } ;

This seems a very powerful and useful feature, so thanks again Macboy for explaining it to me. 

[DmitryL]

LOL

Good luck with debugging your code 3 month later.
If someone can write horrible macros, this doesn't mean that is a right thing to do.

[Ian.M]

Gack!!! 

Ditch all that and read Jack Ganssle' article: A Guide to Debouncing.  When you get to page 2, pay particular attention to the section near the bottom titled 'Handling Multiple Inputs' which shows how to debounce a whole port's worth of inputs (or even more) in parallel in a computationally efficient fashion.  C source is even provided.

[Delta]

LOL

Good luck with debugging your code 3 month later.
If someone can write horrible macros, this doesn't mean that is a right thing to do.

Glad I could amuse use.

So what's so bad about it?  The code is visible in the #define statement, and I know it works.

Is using macros like that considered bad practice then?

[Delta]

Gack!!! 

Ditch all that and read Jack Ganssle' article: A Guide to Debouncing.  When you get to page 2, pay particular attention to the section near the bottom titled 'Handling Multiple Inputs' which shows how to debounce a whole port's worth of inputs (or even more) in parallel in a computationally efficient fashion.  C source is even provided.

Thanks for the Gack!

I will read through the info you provided, but at the moment I how more interested in how to use a C compiler and the IDE, rather than how to write the best code.  If that makes sense...

[Ian.M]

Its still broken, try passing it a named (#defined) port letter or bit number to see how.
 
Hint: if using ## you *MUST* use two layers of macros to avoid the preprocessor NOT expanding #defined formal parameters.

[Delta]

Its still broken, try passing it a named (#defined) port letter or bit number to see how.
 
Hint: if using ## you *MUST* use two layers of macros to avoid the preprocessor NOT expanding #defined formal parameters.

Sorry, I'm not following you mate.

I pass it a number referring to what switch to read, and the physical input pin for that switch is #defined in a header file.  Do you mean it won't work if I try to pass it PORTAbits.RA0 for example?  I can't do that anyway because I can't then pass it which flag bits to set.

Ideally I did want to be able to do

read_switch(PORTAbits.RA0, flag_bit, edge_bit)

but it's already been explained to me that that is not possible.

[Ian.M]

Before I figured out how to create 'virtual' ports (to avoid the dreaded RMW effect), I've done macro stuff like setting up a 'friendly' name for a pin:

   #define SW1 A,6

Then later on:

   MAKE_INPUT(SW1);
   x=READ_PIN(SW1);

It breaks if you try to use:

   x=READ_PIN(A,6);

because of how the formal parameters get expanded.  You have to call the underlying macro READ__PIN(port,bit) to get it to use pins without friendly names.

In your case, if you pass in a #defined friendly name for the input number rather than a raw number, it breaks.  I see you are putting the abstraction one level deeper by #defining SWnPIN constants, e.g:

   #define SW1PIN PORTAbits.RA

so the ## token pasting doesn't break immediately,  but that's a really *FUGLY* way of doing it, as ideally if you are going to mess with macros for pin handling, you want to be able to translate the same friendly name into the correct reference for the PORT bit, LAT bit (if user accessible), TRIS bit and even the ANSELx bit on newer PICS with one ANSEL register per analog capable port.

[Delta]

OK, I with you now.  But for what I'm trying to achieve, passing the raw pin name is no use anyway, as I can't pass my flags.

So, my method (from Macboy's suggestion) does work, but is fugly.  Understood.

So what is the elegant way to be able to reuse a block of code to read multiple switches?