1

I'm working on a 433MHz RF reciver code for a PIC16F628A using a cheap 433MHz receiver. The catch here is that I'm only able to use RA4 for the RX, and because of that i had to use timer2 (because the other timers are already i use). I'm almost done with having it completely functional, but there is something that is breaking all my code, and I cant figure out why. The issue is on the ISR:

typedef union {
    uint8_t REG;

struct {
    unsigned RX : 1;    // Bit de estado de pin Rx
    unsigned RXp : 1;   // Bit de estado previo de pin Rx
    unsigned DONE : 1;  // Bit de fin de recepcion
    unsigned MTCH : 1;  // Bit de control conocido
    unsigned CH1 : 1;   // Bit de canal 1
    unsigned CH2 : 1;   // Bit de canal 2
    unsigned GRB : 1;
} REGbits;



} customRFREG;


volatile customRFREG RFREGu; // Registro de modulo RF 433MHz
#define RFREG RFREGu.REG
#define RFREGbits RFREGu.REGbits
int8_t RXc = 0;             // Cuenta de pulsos
uint32_t rcvd_key = 0;     // Registro de recepcion


uint32_t key_db[10] = {0b10010110100101101010, 0b10011000010110001000, 0x0AEAEA, 0x0AEAEA, 0x0AEAEA, 0x0AEAEA, 0x0AEAEA, 0x0AEAEA, 0x0AEAEA, 0x0AEAEA};


void main(void) {
    TRISA = 0x10;
    TRISB = 0x00;
    PORTA = 0x00;
    PORTB = 0x00;


RFREG = 0x00;

INTCON = 0xC0;
PIE1 = 0x02;
T2CON = 0x00;
PR2 = 111;
while (1) {
    if (PORTAbits.RA4 && !T2CONbits.TMR2ON && !RFREGbits.MTCH) {
        T2CONbits.TMR2ON = 1;
    }
    if (RFREGbits.DONE) {
        RFREGbits.RX = RXc = 0;
        RFREGbits.DONE = T2CONbits.TMR2ON = 0;
        RFREGbits.RXp = 0;
        for (int i = 0; i < 10; i++){
            if(key_db[i] == (rcvd_key >> 8)){
                rcvd_key = (rcvd_key - (key_db[i] << 8)) >> 4;
                RFREGbits.MTCH = 1;
                break;
            }
        }
        if (!RFREGbits.MTCH){
            rcvd_key = 0;
        }
    }
    if (RFREGbits.MTCH){
        switch (rcvd_key){
            case 13:
                RFREGbits.CH1 = !RFREGbits.CH1;
                break;
            case 14:
                RFREGbits.CH2 = !RFREGbits.CH2;
                break;
        }
        PORTAbits.RA1 = RFREGbits.CH1;
        PORTAbits.RA2 = RFREGbits.CH2;
        RFREGbits.MTCH = rcvd_key = 0;
        __delay_ms(1000);
    }
}
return;



}


void __interrupt() isr() {
    if (PIR1bits.TMR2IF) {
        RFREGbits.RX = PORTAbits.RA4;
        if (!RFREGbits.RX) {
            if (RFREGbits.RXp) {
                rcvd_key <<= 1;
                if (RXc > 0) {
                    rcvd_key |= 1;
                    PORTBbits.RB4 = 1;
                } else {
                    PORTBbits.RB4 = 0;
                RXc = 0;
            }
            RXc--;
            PORTBbits.RB3 = 0;
        } else {
            RXc++;
            PORTBbits.RB3 = 1;
        }
        if (RXc < -20){
            RFREGbits.DONE = 1;
            T2CONbits.TMR2ON = 0;
        }
        RFREGbits.RXp = RFREGbits.RX;
        PORTBbits.RB5 = !PORTBbits.RB5;
        //RFREGbits.GRB = !RFREGbits.GRB;
        PIR1bits.TMR2IF = 0;
    }
}

As is, the code works perfec (except for the PORTAbits.RA1 = RFREGbits.CH1;, it turns on for a split second and then gets turned off). The thing is that I need RB3, RB4 and RB5 for other things. Removing RB3 and RB4 produces no changes on the functionality of the code, and it still works perfectly fine, but when I remove RB5, everything stops working.

Here is a graph of the outputs with RB5: with RB5 And here is one without RB5: without RB5 As you can see, the decodification when ther is no RB5 is ok up until it stops (on the wrong place).

I have found no explanation on why this happens. I've tried to replace the PORTBbits.RB5 = !PORTBbits.RB5; with RFREGbits.GRB = !RFREGbits.GRB;, but still, the decodification was totally off. I've also tried to take a look at the (of what I think is) generated assembly code to see if I might be able to find a bug (with no knoweldge on assembly):

   983                           ;newmain.c: 126:         PORTBbits.RB5 = !PORTBbits.RB5;
   984     019E  1003                   clrc
   985     019F  1283                   bcf 3,5 ;RP0=0, select bank0
   986     01A0  1303                   bcf 3,6 ;RP1=0, select bank0
   987     01A1  1E86                   btfss   6,5 ;volatile
   988     01A2  1403                   setc
   989     01A3  1803                   btfsc   3,0
   990     01A4  29A6                   goto    u33_21
   991     01A5  29AA                   goto    u33_20
   992     01A6                     u33_21:
   993     01A6  1283                   bcf 3,5 ;RP0=0, select bank0
   994     01A7  1303                   bcf 3,6 ;RP1=0, select bank0
   995     01A8  1686                   bsf 6,5 ;volatile
   996     01A9  29AD                   goto    u34_24
   997     01AA                     u33_20:
   998     01AA  1283                   bcf 3,5 ;RP0=0, select bank0
   999     01AB  1303                   bcf 3,6 ;RP1=0, select bank0
  1000     01AC  1286                   bcf 6,5 ;volatile
  1001     01AD                     u34_24:

982                           ;newmain.c: 126:         RFREGu.REGbits.GRB = !RFREGu.REGbits.GRB;
   983     019E  1003                   clrc
   984     019F  1F75                   btfss   _RFREGu,6   ;volatile
   985     01A0  1403                   setc
   986     01A1  1803                   btfsc   3,0
   987     01A2  29A4                   goto    u33_21
   988     01A3  29A6                   goto    u33_20
   989     01A4                     u33_21:
   990     01A4  1775                   bsf _RFREGu,6   ;volatile
   991     01A5  29A7                   goto    u34_24
   992     01A6                     u33_20:
   993     01A6  1375                   bcf _RFREGu,6   ;volatile
   994     01A7                     u34_24:
   995     01A7                     i1l757:

As you can see, the only difference (appart from the variables) are those extra bcf 3,5 and bcf 3,6, wich I think they select the PORTB register and do something???

One of the weird things is that the PORTBbits.RB5 = !PORTBbits.RB5; shouldn't affect anything related to time or the ISR execution itself. Another thing, that i just did for testing, is replace the RB5 with another pin. I even changed port, but even that broke the code. What didnt broke the code was changing where the pin state is altered. I tried inside the ISR and in the while(1), and that did not affect the functionality of the code. I'm starting to think that it could be some kind of bug with the uC itself, but I don't really know

fpp
  • 171
  • 9
  • 1
    PIC has some weird pipeline behavior. The instruction right before/right after a branch is affected. Try moving your PIR1bits.TMR2IF = 0; higher in the interrupt (perhaps as the first thing after if (PIR1bits.TMR2IF). – Ben Voigt Jul 25 '23 at 19:44

2 Answers2

2

This is almost certainly a classic race condition bug.

Whenever you have a GPIO register and using the pins for unrelated purposes, you can never write to that register from both an ISR and the main program. You need protection from race conditions.

What happens is that while executing the background code, the GPIO register has some value. Then the ISR gets launched and that value gets changed. Now if the interrupt happens just before a write to the GPIO register in the background program, the ISR will write its value to the register and then the background program will immediately overwrite whatever changes you did inside the ISR.

Please note that no variable access in C can be regarded as atomic despite the size of the variable and CPU data word, because any variable access in the higher level C language can translate into several assembler instructions.

As for how to solve it, there are some options:

  • Temporarily disable the specific interrupt before you write to the GPIO register in main() and enable it once the write is done.
  • Invent a "poor man's semaphore", essentially just a bool variable. Pseudo code:
static volatile bool busy = false;

main()
{
  ...
  while(busy)
  {}


busy = true;
  GPIO_REG = something;
  busy = false;
}


ISR()
{
  while(busy)
  {
    enable_global_interrupts();
    // run loop until main() is finished
  }


busy = true;
  GPIO_REG = something;
  busy = false;
}

This synchronizes the main() and ISR so that each access to the GPIO register works like a "critical section" (OS programming term).

Now this may or may not be a sensible option depending on real-time spec. If the ISR is doing something timing-sensitive, the only option might be to switch to another register which is exclusively accessed by the ISR only.

Lundin
  • 20,162
  • 1
  • 26
  • 76
  • 1
    The PIC in question is a single core single thread CPU. The ISR and main program will never be running simultaneously, so your code example is not applicable here. It will just block forever if the interrupt occurs when busy is true. The only potential issue remaining is one of atomic access where the interrupt occurs part way through a multi cycle operation. To prevent this you use critical sections which temporarily disable interrupts just before the operation and reenable them immediately after as per your first bullet point. – Tom Carpenter Aug 08 '23 at 08:28
  • @TomCarpenter Race condition bugs existed long before multi-core CPUs were invented. On microcontrollers, ISRs interrupts the execution to execute some different code, hence the name interrupt. It works exactly like threads but on a much lower level (in fact threads are implemented by interrupt/exception mechanisms in the OS). And the problems of re-entrancy are the very same, whenever a register or variable is shared between the ISR and the main program. – Lundin Aug 08 '23 at 08:34
  • @TomCarpenter "It will just block forever if the interrupt occurs when busy is true" In the pseudo code it is implied that enable_global_interrupts clears the global interrupt mask in some condition code register. I don't remember what this one is called on PIC, but I assume that the reader of this answer knows of basic MCU programming and maskable interrupts. – Lundin Aug 08 '23 at 08:35
  • More study material can be found here and here. – Lundin Aug 08 '23 at 08:38
  • As Tom said, this is a single thread CPU, so I don't think that "race condition bugs" exist here. That said, in the code I attached there is no pin that is modified in both the ISR or main() – fpp Aug 09 '23 at 20:37
  • @fpp Call it what you will - bugs caused by executing multiple independent functions at the same time. This is a bug present in your code; I've worked over 20 years with troubleshooting microcontroller programs, so I kind of recognize this classic bug when I see it... I've managed to write it myself at several occasions too. – Lundin Aug 10 '23 at 06:27
  • 1
    I wouldn't be at all surprised that @fpp's comparators-being-on condition is masking this bug. Or something else obscure will change and then this bug manifests. But R-M-W and "unexpected alteration of PORTx" have plagued PIC users for many decades now. I for one, learned a motto of "Always use LATx or a Shadow Reg in place of PORTx." – rdtsc Aug 10 '23 at 11:38
1

The issue was that I forgot to disable the comparators, thus, when I read RA4, that resulted in the uC clearing the whole PORTA register. What I don't know is why altering the state of RB5 "fixed" that. So, adding CMCON = 0x07; fixes the issue.

A more detailed answer can be found here

fpp
  • 171
  • 9
  • 1
    ...and I bet you will still get the very same problem of PORTA mysteriously getting cleared even after this fix, just more rarely and intermittently. Just don't come say that there's no explanation for it when it happens. – Lundin Aug 10 '23 at 06:29