Interrupt is upsetting my code

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

Help!

 

I have added a third timer interrupt to my code and it's having a strange effect. (Mega 644)

 

I've added Timer 2 in phase correct PWM mode, set tgo interrupt at bottom. It works as expected because dependant on a flag in the ISR it moves a stepper motor and changing the timings produces expected changes in the speed of the stepper.

 

But the new ISR causes another routine that calculates sidereal time from the reading from an RTC to be wildly out every few calculations.

 

Of course I thought it was a variable being changes that needed to be popped on and off the stack or something like that, but no, I've gradually commented out more and more code. Here are two ISRs, the first is OK :

 

TIMER2_OVF:
    reti

 

But this code causes the havoc:

 

TIMER2_OVF:
    in  status,SREG
    out SREG,status
    reti

 

Now as the only real difference is the length of the routine, I wondered if I had got something wrong in the setup so the ISR is being called continuously? Can anyone see anything wrong here, because I can't?

 

;8 bit timer used for controlling the speed of slew movements, so approx 17 Hz for slowest move (1x tracking) up to arournd 3400Hz for fastest moves

	ldi	temp,0<<WGM21|1<<WGM20		;Phase correct PWM (up and down) interrupt at bottom, count in OCR2
	sts	TCCR2A,temp
	ldi	temp,1<<WGM22|0<<CS20|0<<CS21|1<<CS22	;Timer prescaler/64 = 256 KHz basic clock
	sts	TCCR2B,temp
	ldi	temp,36						;chosen by experiment9		
	sts	OCR2A,temp					;divides by previous number*2. then count interrupts to gives slower moves, divide by 215 gives appox 17 hz or slowest move
	;tweak the value loaded to OCR2a to give most relaibe fast move, then tweak divisons to give stepped speeds

 

It's me again...

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

What is "status" ? Presumably this is a register? Is the same register number used for anything else in the code?

TIMER2_OVF:
    in  status,SREG
    out SREG,status
    reti

Obviously that (pointlessly) preserves SREG but in the process it corrupts "status".

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

Yes, status is r4, and b***r me if there isn't an r4 lurking in my code. I've NEVER used r4 for anything else before... what an idiot! Well I won't be using FMUL so I should be able to make Status into r3

 

Thanks - I'm off to run commando through the nettles as punishment...

It's me again...

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

Stub_Mandrel wrote:
I've NEVER used r4 for anything else before... what an idiot! Well I won't be using FMUL so I should be able to make Status into r3

[OT]  OK, I'll bite:  What does use of FMUL have to do with R3 or R4?

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Stub_Mandrel wrote:
and b***r me if there isn't an r4 lurking in my code

This is probably why I stick to C - compilers don't make this kind of error :-)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
TIMER2_OVF:
    in  status,SREG
    out SREG,status

 

It'd really be better to work on ISRs that don't leave the processor state (registers, status) changed when they return.  Reserving certain registers for use by the ISR *only* is not standard practice, and ought not be done unless you really need the extra few cycles of performance.

 

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

I should be able to make Status into r3

I always use R15 for status, well out of the way and the last register before the more useful registers (R16-R31)

John Samperi

Ampertronics Pty. Ltd.

www.ampertronics.com.au

* Electronic Design * Custom Products * Contract Assembly

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

I'm off to run commando through the nettles as punishment...

Suffer for your craft ;-)

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

westfw wrote:

TIMER2_OVF:
    in  status,SREG
    out SREG,status

 

It'd really be better to work on ISRs that don't leave the processor state (registers, status) changed when they return.  Reserving certain registers for use by the ISR *only* is not standard practice, and ought not be done unless you really need the extra few cycles of performance.

 

 

? Surely it's standard practice to save and restore the status register in any ISR other than  the very simplest and not using instructions that change it is very limiting.

 

I miss my dear old 6502 with PHP and PLP that whipped it straight onto the stack and back again.

 

 

It's me again...

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

theusch wrote:

Stub_Mandrel wrote:
I've NEVER used r4 for anything else before... what an idiot! Well I won't be using FMUL so I should be able to make Status into r3

[OT]  OK, I'll bite:  What does use of FMUL have to do with R3 or R4?

 

 

 

Ah ... I was convinced it put the result in R2:R3 for some reason...  its R0:R1 just like MUL.

It's me again...

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

Wouldn't it be a better practice to use PUSH to preserve a register when entering the ISR and POP when leaving? This would avoid hitting registers as the stack would hold the contents. Unless there is a speed of execution requirement.

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

 

"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 user

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

Back when we were using AVRs with limited ram or a hardware stack, using a register for temporary sreg storage was the way to go. I would probably do it just out of habit now days.

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

Wouldn't it be a better practice to use PUSH to preserve a register when entering the ISR and POP when leaving?

Except that SREG cannot be pushed an must be read into another GP register , have you forgotten already? wink

 

This is my basic registers assignment which is never changed, the rest can be utilised for other purposes.

;Registers assignment

;Z register=R30 and R31
;Y register=R28 and R29
;X register=R26 and R27
.def	temp1=r25				;Second Temp register
.def	temp=r24				;Temp register
.
.
.def	save_sreg=r15			;Status reg store during ints.
.
.
.def	hundreds=r2				;Hundreds for HTOD8 and DTOH8
.def	tens=r1					;Tens for HTOD8 and DTOH8
.def	units=r0				;Units for HTOD8, DTOH8 and LPM instruction

ISR_EXT_INT0:
	in		save_sreg,SREG		;Save Status register
;Do something here	
	out		SREG,save_sreg		;Restore Status register
	reti

 

John Samperi

Ampertronics Pty. Ltd.

www.ampertronics.com.au

* Electronic Design * Custom Products * Contract Assembly

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

Except that SREG cannot be pushed an must be read into another GP register , have you forgotten already? wink

       	push	r1
       	push	r0
       	in	r0, 0x3f
       	push	r0
.
.
.
       	pop	r0
       	out	0x3f, r0
       	pop	r0
       	pop	r1
       	reti

Look familiar?

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

js wrote:
Except that SREG cannot be pushed an must be read into another GP register , have you forgotten already? wink

joeymorin wrote:
Look familiar?

 

I guess I am getting to the point of no return surprise

 

Yeah that looks familiar

 

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

 

"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 user

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

? Surely it's standard practice to save and restore the status register in any ISR

Yes, but you need to also save the register in which you save sreg.  Fortunately, PUSH does not affect SREG:

 

TIMER2_OVF:
    push status
    in  status,SREG
     :
    out SREG,status
    pop status
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Assuming your ISR doesn't re-enable interrupts or use r0 this is the simplest error-proof solution:

push    r0
in      r0,sreg

;

out     sreg,r0
pop     r0

Thing is a c-programmer's are almost always held in ram, by force of habit assembly language folk like to keep lots of things they shouldn't in registers :-)

It's me again...

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

But, C guys, you are wasting clock cycles and memory space with unnecessary pushes and pops which can be saved but assigning a dedicated register for status (as in R15). cheeky

John Samperi

Ampertronics Pty. Ltd.

www.ampertronics.com.au

* Electronic Design * Custom Products * Contract Assembly

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

We're all spoilt by having 32 registers instead of 3 ;-)

It's me again...

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

The AVR only really have 16 real registers, r16-r31, the rest have to many restrictions, so they good/best for special use.

 

And to Bob status can't be stored in r15, because that is my zero register :)  

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

sparrow2 wrote:

The AVR only really have 16 real registers, r16-r31, the rest have to many restrictions, so they good/best for special use.

 

You have to wonder how fundamental that limitation is, that it is non-trivial to do away with it. Presumably something in the design of the avr core.

 

It's me again...

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

Stub_Mandrel wrote:
Presumably something in the design of the avr core.

Well it's the difference between squeezing 4 or 5 bits into opcodes such as LDI.

 

LDI is:

It's because there are only 4 'd' bits there that it can only select 16 different registers (R16..R31 in fact). If there had been 5 'd' bits it could have accessed 32 registers. But as the entire 8 bits of 'K' have to be encoded that would mean something in:

has to give. If you used one of those 4 bits (that effectively select the type of operation) you'd only have 3 left so would reduce the number of operations that could be encoded in this way from 16 to 8. Seems to me that sacrificing 16 registers (but still leaving LDI able to access a generous 16) is the lesser choice of two evils!

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

And that is why some of the newer small tiny's only have 16 registers, it was better (and I guess) smaller to get more RAM.

But they forgot to memory map the registers :( (So some code bloat in size) 

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

clawson wrote:

Stub_Mandrel wrote:

Presumably something in the design of the avr core.

 

Well it's the difference between squeezing 4 or 5 bits into opcodes such as LDI.

 

LDI is:

It's because there are only 4 'd' bits there that it can only select 16 different registers (R16..R31 in fact). If there had been 5 'd' bits it could have accessed 32 registers. But as the entire 8 bits of 'K' have to be encoded that would mean something in:

has to give. If you used one of those 4 bits (that effectively select the type of operation) you'd only have 3 left so would reduce the number of operations that could be encoded in this way from 16 to 8. Seems to me that sacrificing 16 registers (but still leaving LDI able to access a generous 16) is the lesser choice of two evils!

 

That makes complete sense, I ought to have figured that out for myself.

It's me again...

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

Of course the other way is to use more than one 16 bit word for the opcodes like this - but that would involve another cycle during the fetch so would halve the AVR speed on a lot of operations. The whole intent of RISC is to do the commonly used stuff as fast as possible.

 

I suppose what they could have done was to have two forms of LDI. One is a quick, 1 cycle one that operates on R16..R31 and then you'd have a two cycle one that is longer for R0..R15. But LDI/MOV already offer this anyway. (which is a typical RISC compromise).

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

32 registers is a lot (but remember that the original low-end AVRs didn't have ANY actual RAM, so these were directly competing with those low-end PICs with 25bytes of "ram")

It's especially annoying when it comes to saving context, and the ratio of RAM to context gets very small :-(