Program getting stuck after a function is called. Not sure why.

Go To Last Post
23 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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.

Last Edited: Sun. May 8, 2022 - 11:54 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
  1. // ISR for buttons:

Big mistake---why on Earth have an ISR for buttons?  Do you need to touch a button and have something happen in 2 microseconds?

Additionally, you open the doors to responding (interrupting) to all kinds of noise and also bouncing....a mess (though you can negate somewhat by only turning on this interrupt occasionally)...but then why have it at all.

Make life easy, get rid of it.

  1. ISR (PCINT0_vect)

  2.     _delay_ms(1);

Ok, now that really takes the cake ...tops it off with a 1ms icing delay ...the LAST thing you would ever want in an IRQ..... IRQ get in, get out, fast as possible, few us or 10's of microseconds is nice. 

Have your trashcan ready and use it.  Old code brings $5 a pound at the scrapyard.  I made $50 easy last week from pickin out scraps.

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:
why on Earth have an ISR for buttons?

So that hitting the button will wake the MCU up from sleep.

avrcandies wrote:
Do you need to touch a button and have something happen in 2 microseconds?

If I'm not using it for anything else, I don't see any reason that it's  bad to use it; it's not like it hurts to use interrupts.

avrcandies wrote:
Ok, now that really takes the cake ...tops it off with a 1ms icing delay

Initially that was intended to be temporary, but it seems to work well for debouncing.

 

EIther way, none of that really explains why the program is hanging where it is.

Last Edited: Sun. May 8, 2022 - 11:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

So that hitting the button will wake the MCU up from sleep.

Ok, that is one decent reason.  Depending on the need you can also periodically wake up using a timer and check your I/Os' and go back to sleep.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

An ICE debugger would tell you WHERE it is hanging.

John Samperi

Ampertronics Pty. Ltd.

https://www.ampertronics.com.au

* Electronic Design * Custom Products * Contract Assembly

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

js wrote:

An ICE debugger would tell you WHERE it is hanging.

 

You could find out in a S.N.A.P. too....winkcheeky

 

Jim

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I took a quick look at your code. I'll pull no punches; It's an absolute structural nightmare.

 

Your main() is essentially init() followed by sleep forever. This is your first WTF.

ALL the work is done by chaining off various ISRs. This is your 2nd WTF.

In your PIN_CHANGE ISR you call get_temp() which in turn calls sensor_rx which relies on TIMER1_CAPT ISR to increment a global number_of_captures .

 

This is just such a big mess that I must force myself to stop or you'll get upset.

My advice: Delete your code and start again.

 

Last Edited: Mon. May 9, 2022 - 08:31 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

N.Winterbottom wrote:
I took a quick look at your code. I'll pull no punches; It's an absolute structural nightmare.
That was my impression too.

 

One of the key things about AVR interrupts (because they globally disable interrupts during handling and, on basic AVR, there's no priority system) is that you need to "get in and get out" as fast as you possibly can. An ISR() should only operate for microseconds. Do only what MUST be done at the exact instant of the interrupt and leave the heavy lifting stuff to be done elsewhere.

 

The idea of sei()s in ISRs is a recipe for disaster - what happens if the same interrupt even t occurs while that event is already part way through handling (and repeat a few hundred times!)

 

Also look at the generated code and see what the avr-gcc penalty is if you call a function from an ISR ! ;-)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Button wake up using an interrupt is fine, but the ISR() does not need to do anything, it can be left blank or at most disable this source of interrupts, once the mpu is awake it can poll the buttons and do what is needed, then enable wake up interrupts and go back to sleep.   Do all the work in your main(), seek out state machine for guidance on how that is done.  Sorry but I have to agree with the others here, time to trash this code, and get out a pencil and paper and do a new design.

Your program should work without the sleep/wake up feature, once it does, then you can add it to reduce power consumption if that is needed.

Good luck with your project.

Jim

 

 

FF = PI > S.E.T

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Also posted here...

 

https://www.eevblog.com/forum/mi...

 

 

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 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.

 Forget about sleep mode for the minute---do the basic measurements work well?   A simple state machine would serve you well to select the different modes and eventually go to sleep.

 

Using a port is extremely easy and straightforward...what is all this worthless crap for? 

  1. {

  2.         .direction_register = &DDRC,

  3.         .output_register = &PORTC,

  4.         .pin_bitmask = 1 << 0,

  5.     },

 

The ports are ready to use, just configure & use them...spend your time working on the program not inventing foolishness.  

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Using a port is extremely easy and straightforward...what is all this worthless crap for?

Partly my fault. I showed some example in another thread, and the pin struct was used. What is left resembles what I showed, but was made worse.

 

You can use that idea, but no need to create the extra info (only need &PINx, not DDR and PORT). Also, as soon as you start creating an array of them (which is nice), you have to remember as soon as you start using array indexes you will lose the atomic properties as cbi/sbi is no longer being produced. So it needs to be treated just as you would treat a macro if you want to maintain the use of sbi/cbi (no variables for array index).

 

If these are going to be used, then also use the pinFunctions so you do not have to do bit manipulations yourself (and you then don't have to look at a bunch of bit manipulations in your app code), and can easily take advantage of the PIN/DDR/PORT relationship. This idea works fine and is better than macros (imo), but you have to knows the 'rules' to maintain atomic pin manipulation (static inline always_inline also needed). Use an avr0/1 and the atomic restrictions go away since there are registers for atomic with no reliance on getting a specific instruction.

 

This would be a good start to the project, where you get a display working that depends on no other code, works in the background, can easily be tested/verified (also notice that any pin can be used for any segment/common)-

https://godbolt.org/z/aT1o5Txbr

and once this is done, move on to the next problem. You will have a display you can use at that point, which will not be tangled up in or depend on other code. Get smaller pieces of code doing specific things, work on them one at a time, eventually ending up in a completed app.

 

 

Also posted here...

These questions are getting posted to this forum, eevblog forum, and I guess possibly others. A shotgun approach. Makes one less interested in answering when its known you are missing 1/2 (or more) of the conversation.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I showed some example in another thread, and the pin struct was used. 

Well that's wonderful for something complicated, like controlling an elevator with banks and banks of I/O, but this is about as simple as it gets at the opposite end of the spectrum.  There seems to be a tendency to forget about simplicity when folks start out & all of a sudden they are creating battlefronts and uphill climbs where none were needed.  Instead of concentrating on their main app issues, they end up creating more unnecessary side issues.  Just work the I/O simply and effectively & that's one timesaving that can be used elsewhere.

 

I noted someone else had a similar reaction in the EEVblog:

Seem crazy overcomplicated, so much pointer thing for just few pins!

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

Last Edited: Mon. May 9, 2022 - 07:48 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I sniffed a little, to see how it can be done in C, for all my 7S and Temp projects were in ASM.

 

Only one remark on PWM for light level: Do it! If you can, do adjust display refresh to be 16ms, and add another timer for up to 1ms, which will produce either Overflow, or Compare match (I prefer Overflow) and which will stop the light and engage another digit. Choose light power in steps not less than 1:3, for example 1:4:16:64.

 

Thus, 16ms and 1ms, two timers... if you run out of them, you can use WDT 16ms.

Once in 16ms you can read all keys.

So, start with program-keeping, good luck.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'll be honest, these comments were actually quite entertaining for me to read! I find it hillarious how bad my code must have been to illicit such a resonse from so many people. I really want to improve, so all of your tips and suggestions are very much appreciated!

 

N.Winterbottom wrote:
ALL the work is done by chaining off various ISRs. This is your 2nd WTF.

I completely agree. After further thought, I'm farily certain that the problem that I encountered, which prompted me to make this post, is due to that spaghetti mess.

 

N.Winterbottom wrote:
This is just such a big mess that I must force myself to stop or you'll get upset.

Please do not hold back any suggestions! I am open to any and all criticism. I want to improve, and the only way to do so is for people to point out where I am wrong.

 

clawson wrote:

One of the key things about AVR interrupts (because they globally disable interrupts during handling and, on basic AVR, there's no priority system) is that you need to "get in and get out" as fast as you possibly can. An ISR() should only operate for microseconds. Do only what MUST be done at the exact instant of the interrupt and leave the heavy lifting stuff to be done elsewhere.

 

The idea of sei()s in ISRs is a recipe for disaster

 

Noted, and I completely aggree!

 

clawson wrote:
Also look at the generated code and see what the avr-gcc penalty is if you call a function from an ISR ! ;-)

What do you mean by this? Do you mean look at the generated assembly?

 

ki0bk wrote:
Button wake up using an interrupt is fine, but the ISR() does not need to do anything, it can be left blank or at most disable this source of interrupts, once the mpu is awake it can poll the buttons and do what is needed, then enable wake up interrupts and go back to sleep.   Do all the work in your main()

Perfectly understood, thank you!

 

Brian Fairchild wrote:

Also posted here...

 

https://www.eevblog.com/forum/mi...

as @curtvm guessed, the shotgun approach is exactly what I intended. It can be hit or miss at times on forums wether or not you get responses, so I personally try to maximise the "quesiton surface area".

 

curtvm wrote:

Using a port is extremely easy and straightforward...what is all this worthless crap for?

Partly my fault. I showed some example in another thread, and the pin struct was used. What is left resembles what I showed, but was made worse.

 

This one really gave me a chuckle. I essentially was like "Oh thats an interesting way to abstract the register bit-operations a bit", so I copied what you had, and then I thought I would do what I thought was an improvement at the time by explicitly stating what each register pointer was for in then code, instead of just dereferencing the pointer at various bytes with respect to the PIN register to get the other registers. It's definitely super verbose, but I thought that it was at least somewhat well thought out LOL

 

curtvm wrote:
Also, as soon as you start creating an array of them (which is nice), you have to remember as soon as you start using array indexes you will lose the atomic properties as cbi/sbi is no longer being produced

 

What do you mean by this exactly? What is cbi/sbi?

 

curtvm wrote:
If these are going to be used, then also use the pinFunctions so you do not have to do bit manipulations yourself

 

Fantastic suggestion. I will do exactly that.

 

curtvm wrote:
but you have to knows the 'rules' to maintain atomic pin manipulation

 

Would you have a reference for me to read into what you mean by this? I vaguely have heard some stuff about atomic operators (and I think something was added to C with C11 regarding it), but I know very little.

 

curtvm wrote:
where you get a display working that depends on no other code,

 

This is awesome. I've been trying to think of a way to get things more independent, and this is exactly what I need, so thank you!

 

avrcandies wrote:
Well that's wonderful for something complicated,

 

Personally, I thought that the idea was quite elegant.

 

avrcandies wrote:
Just work the I/O simply and effectively & that's one timesaving that can be used elsewhere.

 

Good advice!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

What do you mean by this exactly? What is cbi/sbi?

When you change a pin you typically want to only change a single pin, preserving the state of the other pins on the same port. This is usually in the form of port |= bitmask or port &= ~bitmask, which means the code will read the port register, modify the value (set/clr 1 bit), then write the new value back to the register. Since this is several instructions, there is an opportunity for an interrupt to 'interrupt' that sequence, and if you also change a pin in the interrupt code its possible that you are setting/clearing a pin on the same port and now when you get back to the main code it will finish its sequence. The problem is the main code does not know about the changes that took place, and will write an 'old' value back to the register, 'undoing' the pin work the interrupt just did. Not an easy thing to figure out that once in a while the pins act strange, so avoidance is the answer. This is not a unique thing only to pins, its just pins are a high use item in both interrupt and non-interrupt code, and there are certainly many more things that should not be interrupted.

 

For an avr, these pin registers are in an address range where the sbi/cbi instruction can be used. This single instruction clears/sets a single bit in a register, and since it cannot be interrupted (its a single instruction) you can know your pin manipulation will always be valid (atomic). These instructions encode the address and bit position so can only be produced at compile time. If you start creating code where the compiler cannot know the address or bit position at compile time (like using a var for an index into an array of pin_t), it will revert to using the

'regular' instructions which are no longer atomic. There are other ways to deal with this, and if you look at arduino code they surround their pin code with disable/re-enable interrupts because of the way they deal with pins. Not a pleasant thing to do, surrounding all pin manipulation with interrupt protection, but if you never have to see it then probably not so bad.

 

Newer avr's like the avr0/1 series deal with atomic pin manipulation by having a special set of port registers that set/clr/tgl a pin. It no longer matters what instructions are produced by the compiler, you will always get atomic pin manipulation if you use these registers. So for example an array of pin_t can be used without any thought of atomic, assuming you are using the set/clr/tgl registers.

 

And this is what a similar translation would look like for a mega4809-

https://godbolt.org/z/vT1v119Ed

notice the use of OUTSET/OUTCLR, so atomic is taken care of, which also means accessing the arrays by index is fine, too. And a little nicer to use. The pin code for a avr0/1 would normally look a little different, but just kept it similar to the previous 328p example.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

where the sbi/cbi instruction can be used

Note that we also typically trust that our compiler will use them when possible to help us out.  Compilers have many setting, configs , adjustments, etc, and accidentally disabling helpful modes (or enabling them unexpectedly) can cause some odd issues. Of course, knowing the compiler inside out & upside down is helpful...and a long road.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you Curt, very nice description of atomicity(?). yes

Wayne

East London
South Africa

 

  • No, I am not an Electronics Engineer, just a 54 year old hobbyist/enthusiast
  • Yes, I am using Proteus to learn more about circuit design and electronics
  • No, I do not own a licensed copy of Proteus, I am evaluating it legitimately
  • No, I do not believe in software or intellectual property piracy or theft
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


Izerpizer wrote:
What do you mean by this? Do you mean look at the generated assembly?
Yes.

 

Here is a contrived example (I had to tell the compiler not to inline the called function as it is so trivial it would probably have inlined it):

 

So anyway here is the ISR:

 

 

Considering that the called function is one line and just writes PORTB:

 

 

And it only uses R24. I am kind of thinking you were not expecting all those PUSH/POPs in the ISR ?

 

The issue is that at the time of compiling the ISR the compiler does not know what "foo()" is or how complex it might be or what registers it may use. So it takes no chances and it preserves ALL the registers!

 

If I just do:

 

which has the same effect. Then...

 

 

Sure there's some needless PUSH/POP in there still (the 5.4.0 compiler I am using still made an assumption that SREG would be changed so it needs to preserve that. It also thinks it needs to ensure R1 contains the usual 0x00 even though the ISR code I wrote does not ever use it. A later compiler removes this inefficiency.

 

HOWEVER not calling functions from ISR() is a lot more efficient than doing what you are doing.

 

This is kind of the point - an ISR should be a handful of instructions that just achieves the bare minimum in the shortest time possible then exits so that I in SREG is left disabled for the absolute shortest time possible. There should be nothing so complex in the ISR that requires to be broken out into called functions in the first place. Do the majority of the work in the interrupt enabled main() environment that is just fed with small inputs from the ISRs

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
I am kind of thinking you were not expecting all those PUSH/POPs in the ISR ?

 

I was thinking about this today. Would you say that all these operations are just an inneficiency of a compiler? If you were to write the assembly yourself, there would be no need to manually add in all of those PUSH and POP instructions right?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Izerpizer wrote:
no need to manually add in all of those PUSH and POP instructions

 

True.

 

My assembler ISR routines can reach n00 lines of code, and without any push/pop. It is because there are some registers specially assigned to work in interrupts, but not on main level.

 

Only if there are buffer actions where Y or Z registers are involved, they will be protected from corruption.

Last Edited: Thu. Jun 2, 2022 - 08:29 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 It is because there are some registers specially assigned to work in interrupts, but not on main level.

It's amazing how much more efficient you can be in some cases.  Compilers are getting smarter however....they need to be like a chess strategy program.

 

If you have a CNC machine, the motor RPM is used in many places--this should be assigned by the compiler to 1 or 2 registers and not to RAM.  Some compilers are smart enough to "see this"

In fact, they should use up all registers where possible (why use ram when a reg goes completely unused)...of course the user likely won't notice any difference.  

I've written programs, some sophisticated, that used no ram at all (other than stack).  You can do a lot with 31 regs.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you want the most efficient ISRs then, yes, follow that example in the user manual that shows how to do Asm only ISRs. 

 

However reread this thread. It seems like your overall design of loading up all the work in the ISRs is sub-optimal so you maybe focusing on the wrong area anyway. C implemented ISRs will be fine if you reduce the actual ISR work to the bare minimum.