switch case with enum (LED state machine)

BobTPH

Joined Jun 5, 2013
11,524
@nsaspook,

From the first paragraph of your quoted description:
A State Machine relies on user input or in-state calculation to determine which state to go to next.
So he has not conceived or implemented a state machine. If this were an assignment I had given him, to invent a problem and solve it with a state machine, it would get an F. He is missing the entire point.
 

nsaspook

Joined Aug 27, 2009
16,330
@nsaspook,

From the first paragraph of your quoted description:

So he has not conceived or implemented a state machine. If this were an assignment I had given him, to invent a problem and solve it with a state machine, it would get an F. He is missing the entire point.
I think you're being unnecessarily harsh on the guy.

Sure, he get's an F, today, but that's what learning is, an F today on day one and later, hopefully better grades. IMO part of the problem with software today on the embedded level is people just coding in X language instead of designing in structures using software engineering with things like state-machines, when appropriate. It's a skill set that needs lots of hard work, asking questions and at times, screwing up.

Yes, it's really obvious the OP doesn't understand a lot on the subjects the OP asks questions on but the OP get a A+ for actually trying.
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
@BobTPH

I appreciate your response and feedback. I've taken your advice and have been delving deeper into the concept of state machines and how they can be effectively applied. I'd like to share my refined understanding and the improvements I've made to my LED state machine demonstration.

I've restructured my code to better align with the principles of a state machine. Here's the updated version of my code:

C:
#include <xc.h>
#include <stdio.h>
#include "config.h"

#define RED_PIN LATB0
#define YELLOW_PIN LATB2
#define GREEN_PIN LATB1

// Enumeration for different light states
enum LightState {
    RED_STATE,
    YELLOW_STATE,
    GREEN_STATE
};

// Global variables for light state and timing
enum LightState currentLightState = RED_STATE;
unsigned int elapsedTime = 0;

// Function to initialize hardware setup
void initializeHardware() {
    // Set RB0, RB1, RB2 as Output Ports
    TRISB0 = 0;
    TRISB1 = 0;
    TRISB2 = 0;
  
    // Initialize LEDs to low
    RED_PIN = 0;
    YELLOW_PIN = 0;
    GREEN_PIN = 0;
}

// Function to configure Timer0
void configureTimer0() {
         TMR0L = 250;    //Timer0 Register Low Byte

    //T0CON: TIMER0 CONTROL REGISTER
     TMR0ON = 1; //Timer0 On
     T08BIT = 1; // Timer0 is configured as an 8-bit timer/counter0
     T0CS = 0;   // Internal instruction cycle clock (CLKO)
     T0SE = 1;    //Increments on high-to-low transition on T0CKI pin0
     PSA = 0; //Timer0 prescaler is assigned; Timer0 clock input comes from prescaler output
     //1:8 Prescale value
     T0PS2 = 1;
     T0PS1 = 1;
     T0PS0 = 1;
     //INTCON: INTERRUPT CONTROL

     PEIE = 0;   //Disables all peripheral interrupts
     TMR0IE = 1; //Enables the TMR0 overflow interrupt
     INT0IE = 0; //Disables the INT0 external interrupt
     RBIE = 0; //Disables the RB port change interrupt
     TMR0IF = 0;// cleared timer overflow flag
     INT0IF = 0; // disabled external interrupt
     RBIF = 0; //disabled Port Change Interrupt Flag bit
     GIE = 1;    // Enable Global Interrupt Enable bit
}

// Main function
void main(void) {
    initializeHardware();   // Initialize hardware setup
    configureTimer0();      // Configure Timer0
  
    while (1) {
      
        // Check light state based on elapsed time
        switch (currentLightState) {
            case RED_STATE:
                if (elapsedTime >= 20000) { // 20 seconds
                    currentLightState = GREEN_STATE;
                    elapsedTime = 0; // Reset elapsed time
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 1;
                }
                break;
            case GREEN_STATE:
                if (elapsedTime >= 3000) { // 3 seconds
                    currentLightState = YELLOW_STATE;
                    elapsedTime = 0; // Reset elapsed time
                 
                    RED_PIN = 0;
                    YELLOW_PIN = 1;
                    GREEN_PIN = 0;
                }
                break;
            case YELLOW_STATE:
                if (elapsedTime >= 20000) { // 20 seconds
                    currentLightState = RED_STATE;
                    elapsedTime = 0; // Reset elapsed time
                  
                    RED_PIN = 1;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 0;
                }
                break;
        }
    }
}

// Timer0 Interrupt Service Routine
void __interrupt() isr() {
    if (TMR0IF) {
        elapsedTime++; // Increment elapsed time
        TMR0L = 250;   // Reload Timer0
        TMR0IF = 0;    // Clear Timer0 interrupt flag
    }
}
 
Last edited:

Thread Starter

Kittu20

Joined Oct 12, 2022
511
That is more like it. Now it is actually behaving as a state machine, with the input being elapsed time. Good work!
Thank you for the feedback. I hope now we have one practical example to choose option polling or interrupt. I used interrupt because I thought it should be used but I don't have any specific reason.

So can anyone clarify which option would be best suitable for this example? In my place if you ware doing this what option would you choose and please explain why would you choose?

Added link for previous discussion https://forum.allaboutcircuits.com/...n-polling-and-interrupts-when-and-why.195481/
 

BobTPH

Joined Jun 5, 2013
11,524
I used interrupt because I thought it should be used but I don't have any specific reason
For keeping an accurate timer, interrupts is the only option. Otherwise you would have to check for the timer completing its period all over your code, ensuring that it was always checked before it could could complete more than once. With a complex program, that would be a nightmare.

All of my microcontroller programs use a timer interrupt if they need timing.
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
I've reached a point where I'm about to move on to the next step. Before I do, I wanted to reach out to this awesome community and see if anyone has any suggestions or advice regarding C language concepts for program #23. I'm pretty confident in my code so far, but a fresh set of eyes and some expert insights would be greatly appreciated.

Next I'm planning to create a two-way traffic lights control system using two sets of RED, Yellow, and Green LEDs. I believe this would be good idea
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
I'm trying to decide whether it's a good idea to use inline functions in my program to improve performance. Any thoughts on whether this is a good approach?
 

nsaspook

Joined Aug 27, 2009
16,330
I'm trying to decide whether it's a good idea to use inline functions in my program to improve performance. Any thoughts on whether this is a good approach?
It's unlikely to make any difference with most programs with a modern C compiler and it's only a suggestion to the compiler and cannot always be obeyed as usually the optimizer will decide whether to inline or not better than you can.
 

JohnInTX

Joined Jun 26, 2012
4,787
I wanted to reach out to this awesome community and see if anyone has any suggestions or advice regarding C language concepts for program #23
Here are a few things that I noticed:
1) It is not a good idea to use single bit initialization on TRIS on a PIC. It's not a problem here but it can cause problems on ports that use hardware peripherals, especially PSP and I2C. Initialize TRIS using byte writes i.e. TRISB = 0b00000000;
2) You are only initializing the IO that you are using. What about the other IO? It's not good to leave them floating. Always nitialize ALL unused IO to output 0 i.e. do a full initialization of the machine after reset, even if you are just trying things out.
3) If your program gets busy, the 'elapsedTime' counter can overflow. You need to guard against that possibility. Never assume that the program timing will take care of that.
4) In the 'case' statements, you read the two byte 'elapsedTime' variable as if it is 'atomic' i.e. always valid. Keep in mind that reading a 16bit variable on an 8 bit machine requires more than one machine cycle (several in fact). If the interrupt occurs between the reading of the individual bytes of the timer, it can corrupt the value read while reading it - not a good thing. The remedy is to disable the interrupt driving 'elapsedTime', reading it into a temporary variable while it is stable, re-enabling the interrupt then using the value in the temporary variable.

Use a function to manage the interrupt and return a stable timer value:

unsigned int read_elapsedTime(void)
{
unsigned int temp_value;
disable timer interrupt;
temp_value = elapsedTime;
enable timer interrupt;
return (elapsedTime);
}

Your test becomes:
if (read_elapsedTime() > 20000) { // for example
do something;
}

You'll also need a function to clear the timer using the same approach.

Have fun!
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
Here are a few things that I noticed:
1) It is not a good idea to use single bit initialization on TRIS on a PIC. It's not a problem here but it can cause problems on ports that use hardware peripherals, especially PSP and I2C. Initialize TRIS using byte writes i.e. TRISB = 0b00000000;
2) You are only initializing the IO that you are using. What about the other IO? It's not good to leave them floating. Always nitialize ALL unused IO to output 0 i.e. do a full initialization of the machine after reset, even if you are just trying things out.
3) If your program gets busy, the 'elapsedTime' counter can overflow. You need to guard against that possibility. Never assume that the program timing will take care of that.
4) In the 'case' statements, you read the two byte 'elapsedTime' variable as if it is 'atomic' i.e. always valid. Keep in mind that reading a 16bit variable on an 8 bit machine requires more than one machine cycle (several in fact). If the interrupt occurs between the reading of the individual bytes of the timer, it can corrupt the value read while reading it - not a good thing. The remedy is to disable the interrupt driving 'elapsedTime', reading it into a temporary variable while it is stable, re-enabling the interrupt then using the value in the temporary variable.
Have fun!
I want to share some recent improvements I've made in my code, specifically in the initialization of hardware and managing a counter variable called elapsedTime and I thought it might be beneficial to discuss these enhancements. I introduced a new function called incrementElapsedTime, which ensures that the elapsedTime counter doesn't overflow. This function first checks whether the elapsedTime counter is below the maximum value of 65535 before incrementing it.

C:
#include <xc.h>
#include <stdio.h>
#include "config.h"

#define RED_PIN LATB0
#define YELLOW_PIN LATB2
#define GREEN_PIN LATB1


#define RED_PIN LATB0
#define YELLOW_PIN LATB2
#define GREEN_PIN LATB1

// Enumeration for different light states
enum LightState {
    RED_STATE,
    YELLOW_STATE,
    GREEN_STATE
};

// Global variables for light state and timing
enum LightState currentLightState = RED_STATE;
unsigned int elapsedTime = 0;

void initializeHardware() {
    // Set LATx registers to initial states (all low)
    LATA = 0x00;
    LATB = 0x00;
    LATC = 0x00;
    LATD = 0x00;
    LATE = 0x00;

    // Set TRISx registers to configure pins as outputs (all output)
    TRISA = 0x00;      // All pins are configured as output (Unused)
    TRISB = 0x00;      // All pins are configured as output
    TRISC = 0x00;      // All pins are configured as output (Unused)
    TRISD = 0x00;      // All pins are configured as output (Unused)
    TRISE = 0x00;      // All pins are configured as output (Unused)

    // Additional configuration for unused peripherals
    ANCON0 = 0x00;     // Set all analog pins to digital mode
    ANCON1 = 0x00;     // Set all analog pins to digital mode
    CM1CON = 0x00;     // Turn off Comparator 1
    CM2CON = 0x00;     // Turn off Comparator 2
    ADCON0 = 0x00;     // Disable A/D conversion
    ADCON1 = 0x00;     // Disable A/D conversion
    ADCON2 = 0x00;     // Disable A/D conversion

    // Initialize LEDs to off state
    RED_PIN = 0;       // Set RB0 (RED_PIN) to low
    YELLOW_PIN = 0;    // Set RB1 (YELLOW_PIN) to low
    GREEN_PIN = 0;     // Set RB2 (GREEN_PIN) to low
}


// Function to safely increment elapsed time considering overflow
void incrementElapsedTime() {
    if (elapsedTime < 65535) {
        elapsedTime++;
    }
}

// Function to read elapsed time with interrupt disabled
unsigned int readElapsedTime() {
    unsigned int temp_value;
    INTCONbits.GIE = 0; // Disable global interrupts
    temp_value = elapsedTime;
    INTCONbits.GIE = 1; // Enable global interrupts
    return temp_value;
}

void configureTimer0() {
    // Configure Timer0 Control Register (T0CON)
    T0CON = 0b11000101; // Binary: 11000101

    // Initialize Timer0 counter value
    TMR0L = 250; // Timer0 Register Low Byte

    // Configure Interrupt Control (INTCON)
    INTCON = 0b10100000; // Binary: 10100000
}


// Main function
void main(void) {
    initializeHardware();   // Initialize hardware setup
    configureTimer0();      // Configure Timer0

    while (1) {
     
        // Check light state based on elapsed time
        switch (currentLightState) {
            case RED_STATE:
                if (readElapsedTime() > 20000) { // 20 seconds
                    currentLightState = GREEN_STATE;
                    elapsedTime = 0; // Reset elapsed time
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 1;
                }
                break;
            case GREEN_STATE:
                if (readElapsedTime() > 3000) { // 3 seconds
                    currentLightState = YELLOW_STATE;
                    elapsedTime = 0; // Reset elapsed time
               
                    RED_PIN = 0;
                    YELLOW_PIN = 1;
                    GREEN_PIN = 0;
                }
                break;
            case YELLOW_STATE:
                if (readElapsedTime() > 20000) { // 20 seconds
                    currentLightState = RED_STATE;
                    elapsedTime = 0; // Reset elapsed time
                 
                    RED_PIN = 1;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 0;
                }
                break;
        }
    }
}

// Timer0 Interrupt Service Routine
void __interrupt() isr() {
    if (TMR0IF) {
        incrementElapsedTime(); // Safely increment elapsed time
        TMR0L = 250;   // Reload Timer0
        TMR0IF = 0;    // Clear Timer0 interrupt flag
    }
}
Feel free to share your thoughts and experiences, and let's discuss ways to improve code reliability and maintainability together!
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
Interrupt latency is the delay between an interrupt request and the execution of the corresponding interrupt service routine (ISR).

My program involves handling interrupts I'd greatly appreciate your expertise in reviewing my code and letting me know if my efforts are on the right track to reduce interrupt latency .
 

nsaspook

Joined Aug 27, 2009
16,330
Interrupt latency is the delay between an interrupt request and the execution of the corresponding interrupt service routine (ISR).

My program involves handling interrupts I'd greatly appreciate your expertise in reviewing my code and letting me know if my efforts are on the right track to reduce interrupt latency .
That's one of those things that will need to be instrumented and measured to get a baseline. For the response time (taking into consideration the timing signal code) you can use a measurable (a pulsed signal) interrupt trigger going to something like a external interrupt pin and a measurable signal (like a gpio toggle) to detect when the interrupt routine starts. Because you're using C, the default run-time code and routines will be handled by the C compilers generalized interrupt handlers unless you want to rewrite them in hard-coded to function C or in ASM.

With the K or Q series PIC18 chips most of the interrupt context switching is done automatically so it's unlikely you will improve the basic Interrupt latency on advanced 8-bit chips.
https://onlinedocs.microchip.com/pr...tml?GUID-2AC0BB59-9083-4213-A961-F40BE6B91AF6

What xc8 does with a external interrupt 0 using the MCC to write the code.
C:
// interrupt vector handler
void __interrupt(irq(INT0),base(8)) INT0_ISR()
{
    EXT_INT0_InterruptFlagClear();

    // Callback function gets called everytime this ISR executes
    INT0_CallBack();
}

// ISR code handler
void INT0_CallBack(void)
{
    // Add your custom callback code here
    if(INT0_InterruptHandler)
    {
        INT0_InterruptHandler();
    }
}

// your code starts here
// the ISR code with GPIO signals for the code signals and execution time
void ext0_isr(void)
{
    IO_RD7_SetHigh(); // ISR start
    IO_RD7_SetLow(); // ISR execution code ends
// there is still the restore context time when it returns to main.
}

// in main link the ISR code to the external interrupt and enable the interrupt
INT0_SetInterruptHandler(ext0_isr);
EXT_INT0_InterruptEnable();


void onesec_io(void)
{
    RLED_Toggle();
    DLED_Toggle();
    MLED_SetLow();
    B.one_sec_flag = true;
}
The external interrupt 0 falling trigger signal purple trace is a GPIO output pin DLED to the ext0 input pin from a one second timer interrupt. The yellow trace is the ISR GPIO signal response.
1692720934886.png
Response time.
1692721276603.png
ISR completion time.
 

JohnInTX

Joined Jun 26, 2012
4,787
Feel free to share your thoughts and experiences,
elapsedTime = 0; // Reset elapsed time
This is still a problem.

C:
// Timer0 Interrupt Service Routine
void __interrupt() isr() {
    if (TMR0IF) {
        incrementElapsedTime(); // Safely increment elapsed time
        TMR0L = 250;   // Reload Timer0
        TMR0IF = 0;    // Clear Timer0 interrupt flag
    }
}
You don't really need a function to increment elapsedTimer here. The timer is incremented in one place and only in the interrupt routine. Do in in line and save the call and return overhead.

The bounds checking on elapsedTimer is good.

@nsaspook has you covered regarding interrupt latency. I will add that it is a good idea to always do the absolute minimum required in the interrupt routine and defer as much processing as possible to the main routines. You want the interrupt routine as short as possible.

Nice work.
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
This is still a problem.
You don't really need a function to increment elapsedTimer here. The timer is incremented in one place and only in the interrupt routine. Do in in line and save the call and return overhead.

The bounds checking on elapsedTimer is good.
Your analysis of not needing a separate function to increment elapsedTime in the interrupt routine is correct.

Here's the optimized version of the ISR without the function call:

C:
#include <xc.h>
#include <stdio.h>
#include "config.h"

#define RED_PIN LATB0
#define YELLOW_PIN LATB2
#define GREEN_PIN LATB1

#define RED_PIN LATB0
#define YELLOW_PIN LATB2
#define GREEN_PIN LATB1

// Enumeration for different light states
enum LightState {
    RED_STATE,
    YELLOW_STATE,
    GREEN_STATE
};

// Global variables for light state and timing
enum LightState currentLightState = RED_STATE;
unsigned int elapsedTime = 0;

void initializeHardware() {
    // Set LATx registers to initial states (all low)
    LATA = 0x00;
    LATB = 0x00;
    LATC = 0x00;
    LATD = 0x00;
    LATE = 0x00;

    // Set TRISx registers to configure pins as outputs (all output)
    TRISA = 0x00;      // All pins are configured as output (Unused)
    TRISB = 0x00;      // All pins are configured as output
    TRISC = 0x00;      // All pins are configured as output (Unused)
    TRISD = 0x00;      // All pins are configured as output (Unused)
    TRISE = 0x00;      // All pins are configured as output (Unused)

    // Additional configuration for unused peripherals
    ANCON0 = 0x00;     // Set all analog pins to digital mode
    ANCON1 = 0x00;     // Set all analog pins to digital mode
    CM1CON = 0x00;     // Turn off Comparator 1
    CM2CON = 0x00;     // Turn off Comparator 2
    ADCON0 = 0x00;     // Disable A/D conversion
    ADCON1 = 0x00;     // Disable A/D conversion
    ADCON2 = 0x00;     // Disable A/D conversion

    // Initialize LEDs to off state
    RED_PIN = 0;       // Set RB0 (RED_PIN) to low
    YELLOW_PIN = 0;    // Set RB1 (YELLOW_PIN) to low
    GREEN_PIN = 0;     // Set RB2 (GREEN_PIN) to low
}


// Function to read elapsed time with interrupt disabled
unsigned int readElapsedTime() {
    unsigned int temp_value;
    INTCONbits.GIE = 0; // Disable global interrupts
    temp_value = elapsedTime;
    INTCONbits.GIE = 1; // Enable global interrupts
    return temp_value;
}

void configureTimer0() {
    // Configure Timer0 Control Register (T0CON)
    T0CON = 0b11000101; // Binary: 11000101

    // Initialize Timer0 counter value
    TMR0L = 250; // Timer0 Register Low Byte

    // Configure Interrupt Control (INTCON)
    INTCON = 0b10100000; // Binary: 10100000
}


// Main function
void main(void) {
    initializeHardware();   // Initialize hardware setup
    configureTimer0();      // Configure Timer0

    while (1) {
     
        // Check light state based on elapsed time
        switch (currentLightState) {
            case RED_STATE:
                if (readElapsedTime() > 20000) { // 20 seconds
                    currentLightState = GREEN_STATE;
                    elapsedTime = 0; // Reset elapsed time
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 1;
                }
                break;
            case GREEN_STATE:
                if (readElapsedTime() > 3000) { // 3 seconds
                    currentLightState = YELLOW_STATE;
                    elapsedTime = 0; // Reset elapsed time
               
                    RED_PIN = 0;
                    YELLOW_PIN = 1;
                    GREEN_PIN = 0;
                }
                break;
            case YELLOW_STATE:
                if (readElapsedTime() > 20000) { // 20 seconds
                    currentLightState = RED_STATE;
                    elapsedTime = 0; // Reset elapsed time
                 
                    RED_PIN = 1;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 0;
                }
                break;
        }
    }
}

// Timer0 Interrupt Service Routine
void __interrupt() isr() {
    if (TMR0IF) {
        if (elapsedTime < 65535) {
            elapsedTime++;
        }
        TMR0L = 250;   // Reload Timer0
        TMR0IF = 0;    // Clear Timer0 interrupt flag
    }
}
This version directly increments elapsedTime within the ISR, which can indeed save the call and return overhead associated with function calls
 

Thread Starter

Kittu20

Joined Oct 12, 2022
511
This is still a problem.
In this version, the elapsedTime is reset within the main loop when a state transition is about to occur.
C:
// Timer0 Interrupt Service Routine
void __interrupt() isr() {
    if (TMR0IF) {
        if (elapsedTime < 65535) {
            elapsedTime++;
        }
        TMR0L = 250;   // Reload Timer0
        TMR0IF = 0;    // Clear Timer0 interrupt flag
    }
}

void main(void) {
    initializeHardware();
    configureTimer0();

    while (1) {
        // Reset elapsed time when appropriate
        if (currentLightState == RED_STATE && readElapsedTime() > 20000) {
            elapsedTime = 0;
        } else if (currentLightState == GREEN_STATE && readElapsedTime() > 3000) {
            elapsedTime = 0;
        } else if (currentLightState == YELLOW_STATE && readElapsedTime() > 20000) {
            elapsedTime = 0;
        }

        // Check light state based on elapsed time
        switch (currentLightState) {
            case RED_STATE:
                if (readElapsedTime() > 20000) { // 20 seconds
                    currentLightState = GREEN_STATE;
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 1;
                }
                break;
            case GREEN_STATE:
                if (readElapsedTime() > 3000) { // 3 seconds
                    currentLightState = YELLOW_STATE;
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 1;
                    GREEN_PIN = 0;
                }
                break;
            case YELLOW_STATE:
                if (readElapsedTime() > 20000) { // 20 seconds
                    currentLightState = RED_STATE;
                    // LED control logic
                    RED_PIN = 1;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 0;
                }
                break;
        }
    }
}
 

JohnInTX

Joined Jun 26, 2012
4,787
In this version, the elapsedTime is reset within the main loop when a state transition is about to occur.
Well, no. That does not solve the original problem and it introduces a new problem. To understand what I mean consider this scenario:

1) At line 27, switch (currentLightState), assume elapsedTime has a value of 3000 and the state = GREEN_STATE.
2) On this pass, the timer is not greater than 3000 so no changes are made. You would expect a change to YELLOW_STATE on the next increment of elapsedTime BUT before testing elapsedTime at the top of the while loop something happens..
3) TIMER0 triggers its interrupt and elapsedTime is incremented from 3000 to 3001. The interrupt routine completes and main resumes its flow to the top of the while loop.
4) The code continues and at line 20 decides to clear elapsedTimer.
5) Now instead of expecting a transition from GREEN_STATE to YELLOW_STATE, elapsedTime is 0 instead of the expected 3001 and you wait longer than desired.

You had the state machine right the first time. The state transition and all other processing is made based on the value of state and elapsedTime sampled ONCE per pass. Your new code splits the processing into two sections and requires that elapsedTime does not change between sections. You can't make that assumption.

Now back to the original problem:
elapsedTime = 0;

Consider the code that the compiler has to generate to clear the 16 bit integer. Compilers vary but one way to do it is:
Code:
clrf elapsedTime_High_Byte  ; clear high 8 bits
clrf elapsedTime_Low_Byte  ; clear low 8 bits
What could go wrong?

To understand, assume the value of elapsedTime is 1791 (06 FF). Trace the code:
Code:
clrf elapsedTime_High_Byte  ; clears 06 to 00, no problem there but the current value of elapsedTime is 00 FF until the second instruction BUT..

--> Timer0 interrupt right here -->
        elapsedTime++; increments the CURRENT value of elapsedTime (00 FF) to (01 00).
        ; Note how the increment of the low byte flows into the high byte as it should.

        ; Now the interrupt is done and processing resumes by clearing the low byte:
clrf elapsedTime_Low_Byte ; clear the lower 8 bits
What is the new value of elapsedTime? Is it 00 00? No. How well does the code perform when clearing a value of 1791 results in a value of 256?

That's the problem I was talking about. You fixed it to read elapsedTime. I noted in #30 above that you also had to do the same to clear the integer.

Carry on!
 
Last edited:

Thread Starter

Kittu20

Joined Oct 12, 2022
511
Well, no. That does not solve the original problem and it introduces a new problem. To understand what I mean consider this scenario:

That's the problem I was talking about. You fixed it to read elapsedTime. I noted in #30 above that you also had to do the same to clear the integer.

Carry on!
I've introduced the currentTime and elapsedTimeSinceLastLoop variables to ensure that the elapsed time is consistently calculated and used throughout the loop. This should resolve the issues we were facing with the timing and state transitions

Here's the corrected version of code, with the issue you identified

C:
#define RED_PIN LATB0
#define YELLOW_PIN LATB2
#define GREEN_PIN LATB1

// Enumeration for different light states
enum LightState {
    RED_STATE,
    YELLOW_STATE,
    GREEN_STATE
};

// Global variables for light state and timing
enum LightState currentLightState = RED_STATE;
unsigned int elapsedTime = 0;

// ... (initializeHardware(), configureTimer0(), and ISR definitions remain the same)

void main(void) {
    initializeHardware();
    configureTimer0();

    unsigned int previousTime = 0;

    while (1) {
        unsigned int currentTime = readElapsedTime(); // Read elapsedTime once per loop iteration
        unsigned int elapsedTimeSinceLastLoop = currentTime - previousTime;
        previousTime = currentTime;

        // Check light state based on elapsed time
        switch (currentLightState) {
            case RED_STATE:
                if (elapsedTimeSinceLastLoop > 20000) { // 20 seconds
                    elapsedTime = 0;
                    currentLightState = GREEN_STATE;
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 1;
                }
                break;
            case GREEN_STATE:
                if (elapsedTimeSinceLastLoop > 3000) { // 3 seconds
                    elapsedTime = 0;
                    currentLightState = YELLOW_STATE;
                    // LED control logic
                    RED_PIN = 0;
                    YELLOW_PIN = 1;
                    GREEN_PIN = 0;
                }
                break;
            case YELLOW_STATE:
                if (elapsedTimeSinceLastLoop > 20000) { // 20 seconds
                    elapsedTime = 0;
                    currentLightState = RED_STATE;
                    // LED control logic
                    RED_PIN = 1;
                    YELLOW_PIN = 0;
                    GREEN_PIN = 0;
                }
                break;
        }
    }
}

  1. Introducing currentTime: I added a variable named currentTime to store the current value of elapsedTime at the beginning of each iteration of the loop.
  2. Calculating elapsedTimeSinceLastLoop: The elapsedTimeSinceLastLoop is calculated by subtracting the value of previousTime from currentTime. This gives the time that has passed since the last iteration of the loop.
  3. Updating previousTime: After calculating elapsedTimeSinceLastLoop, I updated the value of previousTime to currentTime. This ensures that we have the correct starting point for the next iteration of the loop.
  4. Using elapsedTimeSinceLastLoop in Switch Cases: I replaced the direct use of readElapsedTime() in switch cases with elapsedTimeSinceLastLoop. This way, we're working with a consistent value of time that has passed since the last loop iteration, regardless of any changes made to elapsedTime.
  5. This "while (1)" loop continuously checks the elapsed time and makes state transitions based on the predefined time intervals
 

joeyd999

Joined Jun 6, 2011
6,309
You guys realize the pin output buffer reflects the current state? No need for the superfluous state variable.

Just sayin'.
 
Top