cli very late in gcc generated ISR code

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

Hi all,

I have a standard problem of repeated INT0 executions due to a 'not optimal' design: I am currently using the rising-edge to trigger an interrupt upon a reed-switch event.

I think I'llhave to use hardware-debounced input in the final solution.

However, besides design questions I wonder whether a different gcc code-generation could mitigate the problem.
The gcc generated INT0 assembler-code looks like this:

00001172 <__vector_1>:
    1172:	1f 92       	push	r1
    1174:	0f 92       	push	r0
    1176:	0f b6       	in	r0, 0x3f	; 63
    1178:	0f 92       	push	r0
    117a:	11 24       	eor	r1, r1
    117c:	2f 93       	push	r18
    117e:	3f 93       	push	r19
    1180:	4f 93       	push	r20
    1182:	5f 93       	push	r21
    1184:	6f 93       	push	r22
    1186:	7f 93       	push	r23
    1188:	8f 93       	push	r24
    118a:	9f 93       	push	r25
    118c:	af 93       	push	r26
    118e:	bf 93       	push	r27
    1190:	2f b7       	in	r18, 0x3f	; 63
    1192:	f8 94       	cli
    1194:	80 91 56 01 	lds	r24, 0x0156
    1198:	90 91 57 01 	lds	r25, 0x0157
    119c:	a0 91 58 01 	lds	r26, 0x0158
    ...

and I ask myself, why the cli does appear so late in the code. With all the push-operations being placed before the cli, it takes more than two dozen cycles before further interrupts are blocked.

Why is this so?
Wouldn't it make sense to place cli closer to the beginning of the ISR?
Are there any gcc-attributes/pragmas to control code-generation?

You opinion is very much appreciated.

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

Quote:

and I ask myself, why the cli does appear so late in the code. With all the push-operations being placed before the cli,

The push's are the "prologue". YOu are getting that many pushes either because (a) the ISR() is too complex or (b) more likely it makes a CALL to some function.

The big question for me here though is WHY do you want a CLI in an ISR() anyway? As the AVR CPU vectors to an interrupt it starts by clearing the I bit in SREG anyway so a cli() in the ISR() is completely pointless.

(I'm not sure whether to leave this in AVR Forum or move to GCC - the bit about prologue is GCC specific but the more general point about I behaviour in ISRs applies to all AVRs and all C compilers - I'll leave it for now and see how it goes).

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

Quote:

I think I'llhave to use hardware-debounced input in the final solution.

"Have to"? What is wrong with a simple software debounce? No added hardware; reliable; ...

That said, we have some low-power, low-speed encoder apps that use reed switches. And then indeed we have an RC and a Schmitt-trigger stage before the AVR pin.

IME raw reed switch signals show little bounce. But those are the encapsulated reed switch "units". Maybe because they are magnet-actuated and when the magnet pulls them it is too strong a field to have bounce?

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

Oh, yes--is your code fragment really generated by the GCC compiler? Please show the source code. And please tell optimization level. (I'll guess if there is a cli in there it is -O0)

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

Quote:

(I'll guess if there is a cli in there it is -O0)


Nope the compiler has no reason to ever add CLIs (it knows how the AVR interrupts work! ;-)). This cli() is because OP has written:

ISR(SOME_vect) {
 cli();
 //overlong and tortuous code follows
}

he's askling why his (pointless as we know) cli() has appeared so late in what was generated. In fact I'm going to guess what he actually has for:

 //overlong and tortuous code follows

is really:

  some_external_function();

and that is why the compiler (at any optimisation level) precedes his cli() with a load of PUSH's he wasn't expecting.

(is this getting GCC specific enough yet to be moved I wonder?).

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

My guess is that the OP also has a sei() at the end of the ISR. While the cli() is pointless, the sei() is harmful and must be removed to avoid incorrect behavior.

Regards,
Steve A.

The Board helps those that help themselves.

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

Quote:

My guess is that ...

Quote:

Please show the source code. And please tell optimization level.

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

There is no cli, no function call, no sei. At least not intentionally ;-)

The -O parameter is 's': -Os

But the discussion has made me aware of a code-snippet which I moved from another location to the ISR. It's a macro and thus no 'cli' is visible:

ISR(INT0_vect)
{
    uint32_t curTime;
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { 
       curTime = g_sysTime; }

    uint16_t revTime = curTime - g_lastTime;
    
    if (revTime < 200) // 200 * 0.1ms = 20ms
        return;           // everything below is invalid

    g_revTime = revTime;
}

Do you see it at the first glance? Well, I didn't - but now I know: I don't need the atomic block above as it all happens within an ISR: the operation will be atomic in any case. My bad.

Sometimes you look at the code but simply don't see the culprit. Your 'why use cli in an ISR' statement gave me the pointer.

Thank you for your input.

PS: as a c-programmer (i.e. not a real assembler-adept) I had the (wrong) idea, that interrupt-blocking in an ISR is done 'automatically' in that means, that the c-compiler 'automatically' generates a 'CLI' statement and thus no extra 'cli();' in the c-source is needed.

This is of course wrong, even in assembler it doesn't need any CLI at all. I in fact knew this but when I was looking at the assembler-code I was simply thinking in the wrong direction.

Last Edited: Wed. Jul 23, 2014 - 05:23 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

And I've just detected another error, it's visible in the code above ... :shock: :wink:

(the compiler seems to 'repair' this, but it's not good code)

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

Quote:
And I've just detected another error, it's visible in the code above ...
If you mean the return statement, there's nothing wrong with that.

Regards,
Steve A.

The Board helps those that help themselves.

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

Well, I would have written:
if (revTime >= 200) {g_revTime = revTime;}
(is more concise: one return point, which is easier to read)

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

But that is style, not an error.

Regards,
Steve A.

The Board helps those that help themselves.

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

Quote:

There is no cli, no function call, no sei. At least not intentinally

Then what WAS your intention of using ATOMIC_BLOCK() then?

You don't need ATOMIC_BLOCK() in an ISR - because ISRs run with interrupts disabled the entire thing is atomic. It's the access code in main() that is sharing the same thing that needs atomic protection. That's your fault here.

Without ATOMIC_BLOCK the CLI goes.

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

clawson wrote:

Then what WAS your intention of using ATOMIC_BLOCK() then?

There was none, as I've written above: A cut and paste mistake.

clawson wrote:

You don't need ATOMIC_BLOCK() in an ISR - because ISRs run with interrupts disabled the entire thing is atomic. It's the access code in main() that is sharing the same thing that needs atomic protection. That's your fault here.

Agreed. That was where I copied the snippet from: main().

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

Koshchi wrote:
Quote:
And I've just detected another error, it's visible in the code above ...
If you mean the return statement, there's nothing wrong with that.

Yes it seems so, the resulting assembler code looks ok.

But it isn't good style, and I basically consider this a mistake since I was not aware of the fact that a 'return' is compiled correctly within an ISR.

Good luck, so to say errorfree by accident ;-)

Things work fine now, I've streamlined the code.

Thanks to everybody for the replies.

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

Trouble is your atomic block. An ISR is an atomic block already, and your extra one is generating the unneeded and harmful cli - sei pair.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

Torby wrote:
Trouble is your atomic block. An ISR is an atomic block already, and your extra one is generating the unneeded and harmful cli - sei pair.

Is it not true that one may set the I bit in an ISR to allow it to be pre-empted by another interrupt? In that event an "atomic block" may be appropriate to encapsulate a small critical section. Others have said that more recent AVRs have more complex interrupt priority schemes, so this is not entirely academic.

Koshchi suggested that the exit from an ISR must occur with the I bit clear, but I do not see why that is necessarily so. RETI should be dumb enough to blindly set the I bit without regard to its current state without ill effect, but that assumes the functional unit associated with that operation is sane and I'm not yet at a point where I can quickly verify this assumption.

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

Capt'n - what you say is true. Generally you would try to avoid nested interrupts in an AVR as you have little ram and potentially sizeable stack frames.

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

Kartman wrote:
Capt'n - what you say is true. Generally you would try to avoid nested interrupts in an AVR as you have little ram and potentially sizeable stack frames.

Hmm. So if I used SEI in a pin-change interrupt context, and those interrupts occurred too frequently, the stack could smash all my program variables and invoke undefined behaviour...

More commonly however I would use SEI in an interrupt context mainly to reduce interrupt latency among different interrupts, which should not be a problem as in most cases different ISRs shouldn't interfere in a way that would cause deeply nested stack frames. But the real answer is of course "it depends" on the specifics of the program.

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

He doesn't say what processor he's using, but he didn't ask in the xmega section, so I assumed he wasn't using an xmega with its interrupt priority system.

If he is, and higher priority interrupts might disrupt his atomic operation, he may need to use the interrupt priority system to protect his critical section rather than the cli - sei pair that the ATOMICBLOCK macro builds.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

Quote:
and your extra one is generating the unneeded and harmful cli - sei pair.
Untrue, ATOMIC_BLOCK does not generate a sei(), it restores the original state of SREG. If it did a sei() then if the global interrupt flag was not set before the atomic block then it would produce undesired behavior.

Regards,
Steve A.

The Board helps those that help themselves.

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

Quote:

More commonly however I would use SEI in an interrupt context mainly to reduce interrupt latency among different interrupts, which should not be a problem as in most cases different ISRs shouldn't interfere in a way that would cause deeply nested stack frames

A very dangerous strategy: "suck it and see". It is extremely unwise to SEI within an ISR on the AVR. The only real solution to your interrupt latency is to remove code from the ISRs. Usually you need do nothing more than buffer a character or set a flag. Then look for those things in main() and do the rest. In fact if you are only setting a flag you might as well
forget the ISR all together and simply poll the ??IF flag for the peripheral.

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

clawson wrote:
Quote:

[reducing interupt latency]

A very dangerous strategy: "suck it and see". It is extremely unwise to SEI within an ISR on the AVR. The only real solution to your interrupt latency is to remove code from the ISRs. Usually you need do nothing more than buffer a character or set a flag. Then look for those things in main() and do the rest. In fact if you are only setting a flag you might as well
forget the ISR all together and simply poll the ??IF flag for the peripheral.

I agree in principle that there is rarely a good reason to have large code blocks executing in an ISR. But recall that I am still working on an assembler software serial module, and the code takes anywhere from 16 to 19 cycles just to get to the current state machine phase. At 50-odd cycles per interrupt, that's fairly heavy and the math shows full-duplex at high rates can interact in such a way as to cause bit errors on account of the ISR latencies. Using SEI can reduce the latencies by 1/2 or more and since these ISRs are running at the same basic frequency there shouldn't be any showstoppers. The problem sort of goes away in the half-duplex scenario, but where's the fun in that?

I agree in most cases an ISR shouldn't need to do much more than set a flag but that doesn't work in all cases. I suppose bit-banged protocols in particular suffer from the latency problem the most, and I don't see that it is reasonable to expect to be able to move many instructions out of the ISR in those cases.

At any rate, I don't see why SEI in the interrupt context is any more difficult (or dangerous) than using atomic operations to synchronize code on SMP machines.

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

Quote:

At any rate, I don't see why SEI in the interrupt context is any more difficult (or dangerous) than using atomic operations to synchronize code on SMP machines.

-- You put more burden on stack space. As is readily seen in your example... In a general case, it makes stack usage much less deterministic than without nested interrupts.
-- You have to guard against handling "yourself". Cascading/"recursive".
-- Re the latter, by the time you do the housekeeping to guard you could be in-and-done.
-- YMMV but I dedicate register [variables] to critical interrupt(s). With only one ISR at a time, I can re-use them in multiple ISRs.

I recall one or more past threads on this; perhaps with more points.

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

Quote:

At any rate, I don't see why SEI in the interrupt context is any more difficult (or dangerous) ...

tpappano is in your camp:
https://www.avrfreaks.net/index.p...

Others aren't, including in the linked thread.

Another lively thread:
https://www.avrfreaks.net/index.p...

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

Processors like the 8051 and Z80 have interrupt priority so a given isr can only be interrupted by one of higher priority. This gives a level of 'protection'. Since the AVR doesn't have this, any interrupt source can interrupt. Thus needing an extra level of care by the programmer. With interrupts you are dealing with dynamite. Nest them and you're dealing with nitroglycerine. Handled carefully, they are useful. Handled badly, the outcome usually isnt good.

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

theusch wrote:
Quote:

At any rate, I don't see why SEI in the interrupt context is any more difficult (or dangerous) ...

tpappano is in your camp:
https://www.avrfreaks.net/index.p...

Others aren't, including in the linked thread.

Another lively thread:
https://www.avrfreaks.net/index.p...

Both threads are very informative, and I say that not because some of the former regulars therein agree with my position. :)

Regarding code maintainability, that seems to be a matter of perspective dependent largely on how much of the whole sky one is able to keep in mind at any given time.

(Reference: "Out of memory. We wish to hold the whole sky, but we never will.")