Author Topic: Program getting stuck after a function is called. Not sure why.  (Read 711 times)

0 Members and 1 Guest are viewing this topic.

Offline Kalcifer

  • Regular Contributor
  • *
  • Posts: 82
  • Country: ca
So what's happening is after the `display_digits()` function gets called (from the PCINT0 ISR), the MCU (I'm using an ATmega328P) hangs, and doesn't go back to sleep. Furthermore, after the `display_digits()` function is called, the `get_temp()` function stops  working (It only receives 0 even though I can see that the sensor data communication is happening just fine on the scope after the `display_digits()` function has been called).


Something is getting set or disabled that is preventing the MCU from entering back into its sleep loop, and the same thing is also interfering with the get_temp() function. But I have absolutely no Idea what that would be. I'm hoping that another set of eyes would be able to help me out with this.


Here is my code: https://pastebin.com/DXw1jUim


Background: This is for a simple min/max thermometer project. Takes in some simple commands from 4 buttons (display current temp, display min temp, display max temp, reset min and max temps), and displays the temperatures onto seven segment displays.
 

Online DavidAlfa

  • Super Contributor
  • ***
  • Posts: 2875
  • Country: es
Re: Program getting stuck after a function is called. Not sure why.
« Reply #1 on: May 08, 2022, 11:46:22 pm »
Seem crazy overcomplicated, so much pointer thing for just few pins!
Calling display_digits() in a ISR, blocking the system for 5 seconds, is the worst use of interrupts I've ever seen.
But I must admit I have probably done that (and worse) in the early days!
Code: (display_digits() in ISR lol) [Select]
while (1)
    {
        if (TCNT1 > 39062) // Display the digits for 5 seconds.
        {
            *segment_pins.output_register = SS_CLEAR;
            return;
        }
The button ISR should only toggle some flags, save states, it's the main loop who should do the job.
Instead of this:
Code: [Select]
while(1)
    {
        sleep_mode();
    }
Do something like:
Code: [Select]
while(1)
    {
        sleep_mode();        // sleep at the start of the loop.
        process_button();    // system will wake up when a button is pressed
        display_digits();    // and execute the test of the loop
    }                        // After done, goes to sleep again
Also, you keep passing big structures, just use a global array... Same for pins, everything could be done a lot simpler using defines / macros.

My advice is to run, hide somewhere without internet, a programmer might be very angry right now, tracking your IP address carrying a huge stick  :-DD
« Last Edit: May 08, 2022, 11:56:40 pm by DavidAlfa »
Hantek DSO2x1x            Drive        FAQ          DON'T BUY HANTEK! (Aka HALF-MADE)
Stm32 Soldering FW      Forum      Github      Donate
 
The following users thanked this post: Kalcifer

Offline cv007

  • Frequent Contributor
  • **
  • Posts: 665
Re: Program getting stuck after a function is called. Not sure why.
« Reply #2 on: May 09, 2022, 01:57:29 am »
You need to create a display driver that is interrupt based, and once that is done you no longer need to worry about the display. You write data to a 'display buffer' (4 x 8 segments = 4 bytes), the display interrupt code then simply shows what is in the display buffer. The only timing you have to worry about is to get a refresh rate high enough so you cannot see any flicker (as high as required, no need for excessive).

The display driver is also not in charge of putting segment data into the display buffer, that job belongs to other code. Somewhere you will move data into that display buffer based on a value and a lookup table, and you will also have access to all segments so can individually set the dp or any other segment that you want.

Simple 'display driver' example using what looks like code provided to you for pin usage, although it seems you complicated the pin_t a bit-
https://godbolt.org/z/9Yr6jvojz

The pin_t only really needs to store one of the pin registers (like PINx which is the lowest member of the always consecutive PIN/DDR/PORT set). Once you have PINx, you can get at DDR and PORT with no more information needed. To make things easier, use pinFunctions as in the example to handle the pin tasks so you then have no need to manipulate registers on your own. The use of the pin_t like this requires that you keep in mind that if you have an array of them you access by a variable index, or otherwise get to a point where the compiler cannot know any of these values at compile time, the resulting code could possibly lose its atomic properties for dealing with a pin. This is why the example code is not using loops which would normally be used, although in this case the code is inside an interrupt so atomic not as important at that point (but if using loops you then will also get storage used for the pin_t data)-
https://godbolt.org/z/KnK7Yqxae


There are a hundred other ways to deal with pins on the avr which has specific atomic instructions (sbi/cbi), but not atomic specific registers (such as SET/CLR/TGL such as an avr0/1). Always keep in mind you will either get atomic sbi/cbi instructions produced by the compiler to deal with pins, or you don't. You have to look at the asm if there is any doubt, and if manipulating pins both inside and outside of interrupt code it becomes important.

Now that you have a display driver that is set and forget, you can add some more code (functions) to put values into the 'display ram' so they are human readable (a lookup table for segment data, like 0-9 for example, but a 7segment can display quite a lot of things other than numbers). Once you can display numbers as you wish, then start adding more code to get data you wish to see (your temp sensor data). And so on. Each piece you assemble can be tested on its own, and in the end you will not have the tangled (isr) mess you have now.

« Last Edit: May 09, 2022, 09:25:05 am by cv007 »
 

Online DavidAlfa

  • Super Contributor
  • ***
  • Posts: 2875
  • Country: es
Re: Program getting stuck after a function is called. Not sure why.
« Reply #3 on: May 09, 2022, 01:28:17 pm »
Avoid floats, you don't need them!
The DHT22 ouputs fixed point, ex "550" = 55.0ºC, just use uint16_t for everything, applying a decimal point to the 2nd digit (like you're already doing).

Edit:
I think you had some errors dealing with the interrupts, and perhabs and the DHT22 sensor.
Anyways, that approach was terrible. I modified the code for proper interrupt routines, it might have some errors (I'm not an AVR guy, never used them)

www.jdoodle.com/ia/qF0
« Last Edit: May 09, 2022, 11:24:09 pm by DavidAlfa »
Hantek DSO2x1x            Drive        FAQ          DON'T BUY HANTEK! (Aka HALF-MADE)
Stm32 Soldering FW      Forum      Github      Donate
 

Offline Kalcifer

  • Regular Contributor
  • *
  • Posts: 82
  • Country: ca
Re: Program getting stuck after a function is called. Not sure why.
« Reply #4 on: May 10, 2022, 06:58:22 am »
Seem crazy overcomplicated, so much pointer thing for just few pins!
Calling display_digits() in a ISR, blocking the system for 5 seconds, is the worst use of interrupts I've ever seen.
But I must admit I have probably done that (and worse) in the early days!
Code: (display_digits() in ISR lol) [Select]
while (1)
    {
        if (TCNT1 > 39062) // Display the digits for 5 seconds.
        {
            *segment_pins.output_register = SS_CLEAR;
            return;
        }
The button ISR should only toggle some flags, save states, it's the main loop who should do the job.
Instead of this:
Code: [Select]
while(1)
    {
        sleep_mode();
    }
Do something like:
Code: [Select]
while(1)
    {
        sleep_mode();        // sleep at the start of the loop.
        process_button();    // system will wake up when a button is pressed
        display_digits();    // and execute the test of the loop
    }                        // After done, goes to sleep again
Also, you keep passing big structures, just use a global array... Same for pins, everything could be done a lot simpler using defines / macros.

Thank you very much for the suggestions, and advice! They are a massive help!

My advice is to run, hide somewhere without internet, a programmer might be very angry right now, tracking your IP address carrying a huge stick  :-DD

HA yeah I found that out from another forum that I posted this on.

You need to create a display driver that is interrupt based, and once that is done you no longer need to worry about the display. You write data to a 'display buffer' (4 x 8 segments = 4 bytes), the display interrupt code then simply shows what is in the display buffer. The only timing you have to worry about is to get a refresh rate high enough so you cannot see any flicker (as high as required, no need for excessive).

The display driver is also not in charge of putting segment data into the display buffer, that job belongs to other code. Somewhere you will move data into that display buffer based on a value and a lookup table, and you will also have access to all segments so can individually set the dp or any other segment that you want.

Simple 'display driver' example using what looks like code provided to you for pin usage, although it seems you complicated the pin_t a bit-
https://godbolt.org/z/9Yr6jvojz

Exelent suggestion, and thank you very much for the example code!

You need to create a display driver that is interrupt based, and once that is done you no longer need to worry about the display. You write data to a 'display buffer' (4 x 8 segments = 4 bytes), the display interrupt code then simply shows what is in the display buffer. The only timing you have to worry about is to get a refresh rate high enough so you cannot see any flicker (as high as required, no need for excessive).

The display driver is also not in charge of putting segment data into the display buffer, that job belongs to other code. Somewhere you will move data into that display buffer based on a value and a lookup table, and you will also have access to all segments so can individually set the dp or any other segment that you want.

Simple 'display driver' example using what looks like code provided to you for pin usage, although it seems you complicated the pin_t a bit-
https://godbolt.org/z/9Yr6jvojz

The pin_t only really needs to store one of the pin registers (like PINx which is the lowest member of the always consecutive PIN/DDR/PORT set). Once you have PINx, you can get at DDR and PORT with no more information needed. To make things easier, use pinFunctions as in the example to handle the pin tasks so you then have no need to manipulate registers on your own. The use of the pin_t like this requires that you keep in mind that if you have an array of them you access by a variable index, or otherwise get to a point where the compiler cannot know any of these values at compile time, the resulting code could possibly lose its atomic properties for dealing with a pin. This is why the example code is not using loops which would normally be used, although in this case the code is inside an interrupt so atomic not as important at that point (but if using loops you then will also get storage used for the pin_t data)-
https://godbolt.org/z/KnK7Yqxae

The reason I added the direction, output, and input registers to the struct was mostly for readability. To me, I felt that it wasn't obvious, by just looking at the code, what it was doing by using an array index to get the other registers. I felt that it was possibly more readable to explicitly state the registers, but I completely agree that it is completely unecessary, and verbose.

From your code snippet, I'm not sure I understand what this line does.
Code: [Select]
#define SAI     __attribute(( always_inline )) inline static //to get sbi/cbi as much as possible

Avoid floats, you don't need them!
The DHT22 ouputs fixed point, ex "550" = 55.0ºC, just use uint16_t for everything, applying a decimal point to the 2nd digit (like you're already doing).
You are absolutely correct! I'm truly not sure why I am doing it the way that I currently am. It is just overcomplicating it to no end.

Edit:
I think you had some errors dealing with the interrupts, and perhabs and the DHT22 sensor.
Anyways, that approach was terrible. I modified the code for proper interrupt routines, it might have some errors (I'm not an AVR guy, never used them)

www.jdoodle.com/ia/qF0


The sensor is definitely working fine (I can see its output on the scope), but I defintiely agree that the interrupts are most likely the culprit. And thank you very much for the code example!



 

Offline cv007

  • Frequent Contributor
  • **
  • Posts: 665
Re: Program getting stuck after a function is called. Not sure why.
« Reply #5 on: May 10, 2022, 07:58:50 am »
Quote
From your code snippet, I'm not sure I understand what this line does.
See your avrfreaks thread about why these instructions are wanted for the avr in use. These attributes will get the compiler to produce the sbi/cbi instructions as much as possible- static does the work, always_inline is to prevent the compiler from doing what it thinks is clever and producing something other than sbi/cbi, and the inline prevents the toolchain from complaining about unused functions (allows you to easily put this code in a header where you have access to any of the functions, get no complaints and no code for anything unused).
 

Online ledtester

  • Super Contributor
  • ***
  • Posts: 2506
  • Country: us
Re: Program getting stuck after a function is called. Not sure why.
« Reply #6 on: May 10, 2022, 08:08:55 am »
Re: "Display for 5 seconds"

It can be very useful to maintain an application clock which calls an interrupt every, say, millisecond. Then in the ISR you can decrement (software implemented) countdown timers and set flags for your main loop to pick up when the timers hit 0. This ISR is also where you could handle multiplexing the LED digits.

Then your code could look something like:

Code: [Select]
main() {
  disable all software timers;
  while (1) {
    blank display;
    sleep_mode();
    do {
      process_buttons();
      if (button pressed) {
        read and display temperature();
        reset and start the five second timer;
      }
      delay a bit;
    } while (five second timer still running);
  }
}

Note that blanking/setting the display will be as simple as populating a buffer reserved for the digits of the LED display.
 

Online DavidAlfa

  • Super Contributor
  • ***
  • Posts: 2875
  • Country: es
Re: Program getting stuck after a function is called. Not sure why.
« Reply #7 on: May 10, 2022, 10:21:46 am »
Yes, the sensor might be working, I think the code was stalling when receiving the data.
Also you were suming all the 8 bytes from the uint64, including the checksum itself, while the data is only 4bytes, so your sum was always wrong.
« Last Edit: May 10, 2022, 10:33:19 am by DavidAlfa »
Hantek DSO2x1x            Drive        FAQ          DON'T BUY HANTEK! (Aka HALF-MADE)
Stm32 Soldering FW      Forum      Github      Donate
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf