Quiz of the week: How to disable ADC interrupt temporarily?

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

Let's have an AVR with ADC, say ATMega128. Let's have written an ADC ISR which re-triggers the ADC, and let's initialise the ADC for single-conversion and enable the interrupt. That should yield us a continuously running AD conversion, isn't it.

Now if we want to exchange some variables between main and the ISR, a well-behaving main won't temporarily disable the interrupts globally (through sei/cli), but only the ADC interrupt.

How to do that properly?

Jan Waclawek

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

Surely just clearing ADIE temporarily will do what you want. You could alse clear ADEN.

But presumably this is a trick question.

David.

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

David wrote:
Surely just clearing ADIE temporarily will do what you want.
That would be my approach.
David wrote:
You could also clear ADEN.
Not if the intention is to hold-off the interrupt. I'm not sure of the behavior of the ADC if you disable it in the middle of a conversion. Will the conversion complete? Get screwed up? In any case, Murphy says the processor will choose the option you least want. :twisted:
David wrote:
But presumably this is a trick question.
Not at all. It's a good question -- Suppose I want to hold-off the ADC ISR because I want to load parameters that (presumably) the ISR will want to use the next time it fires up. On the other hand, I may not want to hold off some other more important interrupt(s). Learning to control a single interrupt is valuable.

Jan: I believe the datasheet says that the ADIF flag is set regardless of whether ADIE is set or not. Thus, if you have disable ADIE but the ADC completes, the ADIF flag is set and when you enable ADIE later, the interrupt will be serviced.

I actually use this concept in controlling access to my SPI controller. I have several SPI DACs tied to my main processor along with a slave AVR. The protocol to the DACs is simple, while the slave protocol is very complex and can take some time (many SPI transactions). I allow the DAC code to "hold off" the slave by temporarily turning off the slave's interrupt line, doing the DAC transaction, then releasing the slave interrupt. It works remarkable well -- I never "lose" a slave interrupt while the DAC controls work just fine.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

Stu San,

In fact, my only purpose is atomicity of access to variables. Particularly, in the interrupt, I read out the ADC value and store it in an uint16_t variable (volatile, of course, and an array of them, as I switch the MUX in the interrupt), and then I would like to read it out in the main - as it is a 2-byte value, atomicity is an issue.

However, you are of course right, there might be other valid reasons to selectively disable the ADC interrupt, eg. to delay its execution for some reason - I did not think about that previously.

The "trickiness" in the question is not related to interrupt-related flags as a concept, nor ADC as such, but to the particular implementation of the ADC "machine" in AVRs.

David,

david.prentice wrote:
Surely just clearing ADIE temporarily will do what you want.
Yes, it will.

david.prentice wrote:
You could alse clear ADEN.
No, I don't think so. Although the datasheet is not crystal clear in this regard, I would interpret "Turning the ADC off while a conversion is in progress, will terminate this conversion." as "the current conversion is sc***ed, you then must start a new one", which is not quite what I would want.
david.prentice wrote:

But presumably this is a trick question.
Yes it is.

So, how to clear ADIE temporarily *properly*?

Jan

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

I can see where apps that want to reduce jitter in (say) bit-banged PWM out of timer interrupts would want detailed precautions, but I've never found the need over the years and just do CLI/SEI pairs around each atomic read/write.

I assume the trick part has to do with ADIF being in the same I/O register as ADIE? Thus, this old bit-pusher doesn't strain the brain but just does the CLI/SEI.

Lee

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

wek wrote:
Now if we want to exchange some variables between main and the ISR, a well-behaving main won't temporarily disable the interrupts globally (through sei/cli), but only the ADC interrupt.

I disagree :!:

The right programming style was always using only cli/sei for every atomic access. :idea:

The reason was, that disabling only one interrupt may cause priority inversion.

With using cli/sei the disable time was always limited to about 4 cycle. :idea:

But with disabling only one interrupt, all other interrupts may stretch the disabling time many times more, maybe 10000 cyle or more. :twisted: :twisted:

Peter

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

danni wrote:
wek wrote:
Now if we want to exchange some variables between main and the ISR, a well-behaving main won't temporarily disable the interrupts globally (through sei/cli), but only the ADC interrupt.

I disagree :!:

The right programming style was always using only cli/sei for every atomic access. :idea:

The reason was, that disabling only one interrupt may cause priority inversion.

With using cli/sei the disable time was always limited to about 4 cycle. :idea:

But with disabling only one interrupt, all other interrupts may stretch the disabling time many times more, maybe 10000 cyle or more. :twisted: :twisted:

And what? What if my hand-made timer-interrupt-based PWM (courtesy of Lee) can't stand 4 cycles of jitter but the ADC interrupt is happy with 10000 cycles delay? What if it is not 2 bytes I want to move atomically but an array of 8x8 bytes? What makes you believe that I am happy with the hardwired interrupt polling order anyway? What if I WANT to change that order?

And, I reserve to myself the right not to conform to any "right" programming style, wherever I find it appropriate. It's my gun, my bullets, and my foot...

Anyway, this is not what the quiz is about.

Jan

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

Quote:

It's my gun, my bullets, and my foot...

LOL!

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

wek wrote:
And what? What if my hand-made timer-interrupt-based PWM (courtesy of Lee) can't stand 4 cycles of jitter
Then you can't enable the ADC interrupt anyway. ;-)

Quote:
And, I reserve to myself the right not to conform to any "right" programming style, wherever I find it appropriate. It's my gun, my bullets, and my foot...
But then I would not call it "well-behaving".

Stefan Ernst

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

sternst wrote:
wek wrote:
And what? What if my hand-made timer-interrupt-based PWM (courtesy of Lee) can't stand 4 cycles of jitter
Then you can't enable the ADC interrupt anyway. ;-)

That's fully correct. :!:

Already an empty interrupt stall all other interrupts for at least 10 cycle.
If a 4 cycle jitter can not be tolerated, there exist only one solution: let no other interrupts enabled at all.
But also then e.g. every RET instructon cause a jitter of 4 cycle.

The best approach was always, make all interrupts and all atomic access as short as possible.

Also inside a timer interrupt it's easy to compensate a small jitter, since the timer register contain the exact delay value until the interrupt was handled.

Peter

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

sternst wrote:
Quote:
And, I reserve to myself the right not to conform to any "right" programming style, wherever I find it appropriate. It's my gun, my bullets, and my foot...
But then I would not call it "well-behaving".

I agree.

"well-behaving" was an approach, which covers about 99% of tasks.

If a special solution was needed for the remaining 1%, it should never be named as "well-behaving".

Peter

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

OK. Once again: this is not what the quiz is about.

And, I don't really care what you call "well behaving" - that's your arbitrary definition against my arbitrary definition.

But, if you want a reason for selectively disabling interrupts, again, assume, you need to make a more substantial operation, say, copy an array of values collected in previous interrupts.

However, if you want to discuss the merits of using selective or global interrupt disables, please, start a new thread.

JW

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

wek wrote:
Once again: this is not what the quiz is about.

Your question was already solved on the second post (ADIE-bit).

Thus I wanted to add, that it may be used only, if you know exactly, what you are doing.
Since typically the "stupid" cli/sei approach was more safe (less side effects), faster and less code. :idea:

On every write to ADIE, you must watch, that ADIF was set to 0, otherwise a pending interrupt was lost.:!:
So "CBI ADCSRA, ADIE", "SBI ADCSRA, ADIE" can not be used.

Peter

Last Edited: Wed. Oct 7, 2009 - 09:25 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

danni wrote:
Your question was already solved on the second post (ADIE-bit).

No, that's not enough. The question is, *how* to disable ADIE temporarily.

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
ADCSRA &= ~(1<<ADIE);

will surely generate an atomic CBI won't it?

Cliff

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

wek wrote:
No, that's not enough. The question is, *how* to disable ADIE temporarily.

The ADIE can never be disabled, it can be accessed always.

The ADIE disable/enable only the ADC-interrupt.
The function of the ADC itself was unaffected by ADIE.

Peter

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

clawson wrote:

ADCSRA &= ~(1<<ADIE);

will surely generate an atomic CBI won't it?

Yes, that's the pitfall.

Peter

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

What "pitfall"? What's wrong with a single opcode that only affects one bit? Or are you talking about the older models of AVR where this was a read-modify-write?

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

clawson wrote:
What "pitfall"? What's wrong with a single opcode that only affects one bit? Or are you talking about the older models of AVR where this was a read-modify-write?

I assume, it's related to all AVRs.

E.g. ATmega1280:
"Beware that if doing a Read-Modify-
Write on ADCSRA, a pending interrupt can be disabled. This also applies if the SBI and CBI
instructions are used."

Peter

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

clawson wrote:
What "pitfall"? What's wrong with a single opcode that only affects one bit?

There would be nothing wrong with a single opcode that only affects one bit.

clawson wrote:
Or are you talking about the older models of AVR where this was a read-modify-write?
What older models of AVR do you have in mind? I don't believe the behaviour of the AVR core did change.

And both Peter and Lee hinted towards the answer already (although I believe there might be one more similar issue) - I just would like to see it written down as a routine.

JW

(PS. Yes, Peter, it's not temporarily ADIE *disable* but temporarily ADIE *clear*, I deserve to be slapped for not being precise enough :-) ).

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

Ah I see - a conundrum indeed - presumably this was the whole point of Jan's post? I guess there isn't a solution then without potentially losing one pending conversion then? (apart from the already suggested full CLI/SEI thing)

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

clawson wrote:
Ah I see - a conundrum indeed - presumably this was the whole point of Jan's post?

Yes.
clawson wrote:
I guess there isn't a solution then without potentially losing one pending conversion then? (apart from the already suggested full CLI/SEI thing)
Why wouldn't there be one? If it would be so, I would demand a proof that there is none, rather than a solution! :-)

JW

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

danni wrote:
clawson wrote:
What "pitfall"? What's wrong with a single opcode that only affects one bit? Or are you talking about the older models of AVR where this was a read-modify-write?

I assume, it's related to all AVRs.

E.g. ATmega1280:
"Beware that if doing a Read-Modify-
Write on ADCSRA, a pending interrupt can be disabled. This also applies if the SBI and CBI
instructions are used."

Peter

Peter,

It appears he got us - although it's all Atmel's fault ;-) .

A quick look into the newest Tiny datasheets ('T4-5-9-10 and 'T261-461-861 Automotive), under the SFR table at the end, footnote 3:

Quote:
Note that, unlike most other AVRs, the CBI and SBI
instructions will only operation the specified bit, and can therefore be used on registers containing such Status Flags.

The similarly dated ATmega164PA/324PA/644PA datasheet still warns about the not-only-one-bit problem.

Jan

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

Well to be honest, from previous discussion here I thought the change of CBI/SBI to true bit ops actually dated much earlier and affected more devices. I was surprised when Peter said the 1280 still had this warning for example. But I just looked at the 48PA datasheet which must surely be one of the very newest devices and it still has the SBI/CBI warning.

OTOH, knowing the cut/paste nature of Atmel datasheets I wonder if this is an old warning that's just a hangover from the past in some of the newer models?

If simulator 2 really is as accurate as they say for new models then presumably it could be used to test the premise?

EDIT Tiny48/88 has the "unlike most other" comment.

EDIT2: Oh just wait a moment. The mega48/88/168PA datasheet contains both:

Quote:
This bit is set when an ADC conversion completes and the Data Registers are updated. The
ADC Conversion Complete Interrupt is executed if the ADIE bit and the I-bit in SREG are set.
ADIF is cleared by hardware when executing the corresponding interrupt handling vector. Alternatively,
ADIF is cleared by writing a logical one to the flag. Beware that if doing a Read-Modify-
Write on ADCSRA, a pending interrupt can be disabled. This also applies if the SBI and CBI
instructions are used
.

and
Quote:
3. Some of the Status Flags are cleared by writing a logical one to them. Note that, unlike most other AVRs, the CBI and SBI
instructions will only operate on the specified bit, and can therefore be used on registers containing such Status Flags. The
CBI and SBI instructions work with registers 0x00 to 0x1F only.

EDIT3: actually even the original 48/88/168 has footnote 3 so I think it's a fair bet that all AVRs at least since that point (and maybe before?) have the single bit feature. The older mega128, however has:
Quote:
2. Some of the status flags are cleared by writing a logical one to them. Note that the CBI and SBI instructions will operate on
all bits in the I/O register, writing a one back into any flag read as set, thus clearing the flag. The CBI and SBI instructions
work with registers $00 to $1F only.

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

I would suggest a solution for all AVRs:

#define ADIntDisable()  (ADCSR = ADCSR & ~(1<<ADIF | 1<<ADIE))

#define ADIntEnable()  (ADCSR = (ADCSR & ~(1<<ADIF)) | 1<<ADIE)

Peter

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

A second solution:

The main set a flag to signal to the interrupt, that it was reading the data.
Then after finishing the flag was cleared again.

And the interrupt check the flag, and if it was set, the conversion result was stored into a temporary variable.

Peter

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

clawson wrote:
OTOH, knowing the cut/paste nature of Atmel datasheets I wonder if this is an old warning that's just a hangover from the past in some of the newer models?

I'd say, the cut&paste applies to the core as well (it's written in VHDL or alike, isn't it? Looking at the published pictures of actual chips it's quite a mess, nothing like a hand layout).

The ATMega128A is supposedly a remake of the ATMega128, and still, all the cbi/sbi warnings are on.

I'd say, this means, for library-like stuff the caution mode should be on.

clawson wrote:
If simulator 2 really is as accurate as they say for new models then presumably it could be used to test the premise?
Can't comment on it, but would like to know (have no time to try).

Jan

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

danni wrote:
I would suggest a solution for all AVRs:

#define ADIntDisable()  (ADCSR = ADCSR & ~(1<<ADIF | 1<<ADIE))

#define ADIntEnable()  (ADCSR = (ADCSR & ~(1<<ADIF)) | 1<<ADIE)

Peter

I would be also concerned about ADSC.

Jan