eeprom and timer/interrupts

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

Hello,

Some question around interrupts, handler, eeprom on AtMega128:

The eeprom.h file reads:

As these functions modify IO registers, they are known to be non-reentrant. If any of these functions are used from both, standard and interrupt context, the applications must ensure proper protection (e.g. by disabling interrupts before accessing them).

Good. I have a timer handler (I'm using AVRLib) which does some eeprom read/write access among other (actually it manages keypad input, manages TCP/IP dialog through ethernet and contains some logic to control the I/O). Beside this, the main code manages a finite state machine for configuring the complete thing through LCD and keypad.
The first instruction in the handler is cli() and the last sei(), so no interrupts within interrupts. I guess this is safe.

Does the eeprom.h comment means that any eeprom_read or eeprom_write has to be between a cli() and sei() ? Just to be sure...

I'm having some strange behavior at times, and I'm not able to track this down. Keypad and LCD stops answering at some point of time, but TCP/IP is still OK, so the AVR is still alive and processing ethernet frames.....

Is putting everything in a single timer not recommended ? I don't think it is worth creating tasks for only 3 of them (TCP/IP, keypad/LCD state machine, I/O control) as my app is not time-critical.

Any comments/hints ?

Thanks.
V.

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

Quote:
Does the eeprom.h comment means that any eeprom_read or eeprom_write has to be between a cli() and sei() ? Just to be sure...

Not necessarily even that - you could use a "semaphore" to protect access to the EEPROM so that only one "user" could ever access it and therefore avoid the eeprom routines being called re-entrantly from more than one place at the same time.

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

Quote:
I have a timer handler (I'm using AVRLib) which does some eeprom read/write access among other (actually it manages keypad input, manages TCP/IP dialog through ethernet and contains some logic to control the I/O).

It sounds to me that you are doing entirely too much in an interrupt handler. If this is going to be your only interrupt (and you are sure that what happens inside the interrupt will complete before being triggered again), then it may be alright. But in general you want to keep interrupt routines as short as possible.

Quote:
The first instruction in the handler is cli() and the last sei()

You do not need this. An interrupt will automatically clear and set the global interrupt flag at the proper time. The sei() is especially worrisome since it allows another interrupt to take control before the current interrupt is completely finished.

Regards,
Steve A.

The Board helps those that help themselves.

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

I agree that my interrupt code is long, but this is the only way I found to have an asynchronous task for ethernet communication to occur.

Here is the code, just in case:

void timer0handler(void) {
	static uint8_t key_state; // debounced and inverted key state:
	static uint8_t ct0, ct1; // holds two bit counter for each key
	static uint8_t i;
		
	/*
	 * read current state of keys (active-low),
	 * clear corresponding bit in i when key has changed
	 */
	cli();
	i = (key_state ^ ~KEYS_PIN); // key changed ?
	/*
	 * ct0 and ct1 form a two bit counter for each key,
	 * where ct0 holds LSB and ct1 holds MSB
	 * After a key is pressed longer than four times the
	 * sampling period, the corresponding bit in key_state is set
	 */
	ct0 = ~(ct0 & i); // reset or count ct0
	ct1 = (ct0 ^ ct1) & i; // reset or count ct1
	i &= ct0 & ct1; // count until roll over ?
	key_state ^= i; // then toggle debounced state
	/*
	 * To notify main program of pressed key, the correspondig bit
	 * in global variable key_press is set.
	 * The main loop needs to clear this bit
	 */
	key_press |= key_state & i & 0x1f; // 0->1: key press detect
	
	if (init_done){// we need to make sure everything has been initialized before doing anything network related 
		if ( eeprom_read_byte(FUNCTION_ENABLE_REG2) & (_BV(EN_ETH))) 
		{
			networkService2();
		} // end nic 
	} 
	sei();
}

The init section of the main:

	timerInit();
	timer0SetPrescaler(TIMER_CLK_DIV1024);
	timerAttach(KEYS_TIMER, timer0handler);

The main is just a for(;;) to manage the finite state machine.

V.

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

Why are you doing cli/sei inside an interrupt handler?

I would strongly advise against a structure as you have shown. Do the debounce if you must--I never do for button handling. Just indicate that the tick has occurred.

If you do decide to debounce in the interrupt handler, still fine. But just set a flag indicating "there is a new key state to process" and poll it in the mainline to do "stuff".

I have gotten to the point where I do no EEPROM operations in ISRs, even reads. In a case such as yours with a config parameter, I create an SRAM shadow copy. I just don't like the higher overhead of calling a "proper" EEPROM read routine including the delays.

A proper EEPROM read routine will not do cli/sei, but rather it will save the global interrupt enable state, clear (cli), then restore the state. If your compiler doesn't provide that the I would develop it (or switch brands).

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

Hello,

Thanks for your comments.
I use the key_press var as a flag, as it contains all keys that haven't been processed by the mainline. If it contains something, there is something for the mainline to do.
Shadow copying is indeed a solution. That means when I have to write to eeprom, I also have to update the copy. This will probably for v2 :) , once I get everything robust. eeprom access delays are not a problem for my application.

Could you tell me if the WinAVR 20080610 eeprom routines are doing what you mention ? Based on the warning in the .h code, I doubt it but I"m quite new to AVR, so maybe I don't interpret correctly this. In such case, best thing would be to "wrap" the existing eeprom functions.....

V.

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

Quote:
Could you tell me if the WinAVR 20080610 eeprom routines are doing what you mention ?

You can see yourself. Here's an excerpt from eeprom.h:

static __inline__ void eeprom_write_byte (uint8_t *__p, uint8_t __value)
{
    do {} while (!eeprom_is_ready ());

#if	defined(EEPM0) && defined(EEPM1)
    EECR = 0;		/* Set programming mode: erase and write.	*/
#elif	defined(EEPM0) || defined(EEPM1)
# warning "Unknown EECR register, eeprom_write_byte() has become outdated."
#endif

#if	E2END <= 0xFF
    EEARL = (unsigned)__p;
#else
    EEAR = (unsigned)__p;
#endif
    EEDR = __value;

    __asm__ __volatile__ (
        "/* START EEPROM WRITE CRITICAL SECTION */\n\t"
        "in	r0, %[__sreg]		\n\t"
        "cli				\n\t"
        "sbi	%[__eecr], %[__eemwe]	\n\t"
        "sbi	%[__eecr], %[__eewe]	\n\t"
        "out	%[__sreg], r0		\n\t"
        "/* END EEPROM WRITE CRITICAL SECTION */"
        :
        : [__eecr]  "i" (_SFR_IO_ADDR(EECR)),
          [__sreg]  "i" (_SFR_IO_ADDR(SREG)),
          [__eemwe] "i" (EEMWE),
          [__eewe]  "i" (EEWE)
        : "r0"
    );
}

Note in this the mention of "CRITICAL SECTION" and the fact that it's reading SREG to __temp_reg (R0), doing a CLI then restoring SREG afterwards.

Cliff

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

clawson wrote:
Quote:
Does the eeprom.h comment means that any eeprom_read or eeprom_write has to be between a cli() and sei() ? Just to be sure...

Not necessarily even that - you could use a "semaphore" to protect access to the EEPROM so that only one "user" could ever access it and therefore avoid the eeprom routines being called re-entrantly from more than one place at the same time.

Talking of semaphore, I identified the reason of lookup: concurrent I2C requests. So I have to protect i2c bus with semaphores. Is there an easy way to achieve this in WinAVR ? I remember my system lessons, with P() and V() .... but it is quite far away.....

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

WinAVR is a C compiler, not an RTOS. However there are a number of AVR RTOS that are available in a GCC variant that might be used.

But I guess you don't need anything as "fancy" as a full on RTOS just to protect the use of a resource. Just hold a single, atomically accessible variable somewhere (bits in GIPOR0 on a modern AVR are good for this) and when a part of the system wants to use the resource it first checks the bit varible (0=free, 1=busy). If 'busy' it simply waits for it to become free (go to 0). Then as it starts to use the resource it sets the bit to 1 to show that it is now 'busy' anyway.

The issue in that, of course, is "simply waits for it to become free". If you have an RTOS that is "time slicing" the CPU between a number of tasks then it's quite possible for one thread to simply remain "stuck" in a "wait for semaphore" loop until the thing it wants becomes available. If you haven't got an RTOS and there aren't "multiple threads of execution", then the work really needs to be split between mainline and an ISR or two ISRs. (but this can raise issues of enabling interrupts inside the ISRs so the other "task" gets a chance to run and free up the requested resource).

Maybe implementing a simple RTOS is the way forward but if you already have "two people trying to use I2C at once" causing you a problem it suggests you are probably already doing this anyway (either deliberately or as accident of the existing design).

Cliff

EDIT: typing errors

Last Edited: Fri. Oct 24, 2008 - 02:27 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

In fact, I have some controls done within a timer interrupt which uses I2C components. Beside this, I also have some code accessing I2C as well. In a RTOS the timer is generally to switch beetween existing tasks. In y case, the timer is the task itself :)
This is my problem. I could switch to a RTOS (will probably do for another project), but in fact, I have just two tasks in my app now. I don't think it would be relevant to switch to a RTOS now, especially as I don't have real time requirements (I can wait for a couple of seconds for i2C bus, but this would also mean I have a design issue somewhere...)
Maybe I'll write basic semaphore functions (two if I remember), which disable interrupts when called. Or just just a plain global variable as you mention.

Thanks Cliff for your comments.
V.

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

EEAR and EEDR are the registers you need to worry about. They are not protected in any way when using eeprom.h- If for example you read eeprom in main code and also read eeprom in an irq, you could end up with a 'bad' eeprom address or 'bad' eeprom data in the main code because the irq changes both registers (most likely, anyhow).

Eeprom reading/writing from isr's is probably not the best idea unless you can afford to wait around the ~3.5ms that may be required if main code just wrote a byte to eeprom. I think I would simply check if eeprom is ready, and if not, do plan b (skip it, and use previous 'cached' value or something).

I wrote my own eeprom library-
http://www.mtcnet.net/~henryvm/e...
where irq's are off when checking if eeprom ready, and when EEAR and EEDR are being used.

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

Quote:

Eeprom reading/writing from isr's is probably not the best idea unless you can afford to wait around the ~3.5ms that may be required if main code just wrote a byte to eeprom.

Agreed--I use a >>lot<< of EEPROM configuration parameters in my code, with frequent settings checks (sometimes every main loop pass). I have evolved to doing no EEPROM operations, even reads inside ISRs because one cannot afford to have a "fast" interrupt held up for ms. E.g., if a message is coming in over a USART link at 57600 there are several characters every millisecond and there would be overruns. Same with an SPI slave. Neither require latency down in the low us, but cannot afford n ms.

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.