Atomic read/write to uint16_t variable

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

Hi all,
I've been reading about the atomic read and write of variables (bigger than a byte) and I would like to know a few things:

- Is the avr-libc approach to atomic operations (util/atomic.h) the best one to use? I think it's not quite intuitive and code reading friendly as a "static inline function" or a "#define macro" would be.

- Does avr-libc approach also work with writing to variables?

Thanks!

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

Quote:

Is the avr-libc approach to atomic operations (util/atomic.h) the best one to use?

Define "best".

Quote:

I think it's not quite intuitive and code reading friendly as a "static inline function" or a "#define macro" would be.

How would a "static inline function" or a "#define macro" help in avoiding non-atomic access? Are you sure you understood the non-atomic problem in the first place?

Quote:

Does avr-libc approach also work with writing to variables

Yes. The problem of non-atomicity is that an access to a larger-than-byte variable might be disturbed mid-way by an interrupt. The atomic stuff in avrlibc makes sure this wot happen by disabling interrupts in the atomic block. This works just as well for writes as for reads.

The somewhat convoluted syntax is because the avrlibc stuff has to work on regardless of the interrupt state before the atomic block, and that it restores that state at the end of the atomic block. Otherwise it would be as easy as having a

cli();

at the start of the atomic access and a

sei();

at the end of it.

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

Yes, I understand the concept.
What I mean was to have a function like

static inline uint16_t atomic_read_word(uint16_t *var)
{
  ...
  ATOMIC_BLOCK(ATOMIC_FORCEON)
  ...
}

that would read the variable using the avr-libc macros and return the result.
This way the code would look like:

volatile uint16_t var;
...
if (atomic_read_word(var) == 0x200) {
...
}
...

You have to agree that it is easier to read the code than using the blocks and temporary variables of avr-libc.

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

Quote:

You have to agree that it is easier to read the code than using the blocks and temporary variables of avr-libc.

That's entirely possible, however it can be non-optimal. Consider the case where you have a large number of variables to read atomically. Using your function, this would waste many cycles saving, disabling and restoring interrupts. It's up to you to decide what tradeoffs to make in your particular application.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

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

I am just used to disable interrupts myself because I find it very straightforward, but I am not yet grown accustomed to the new atomicity macros.

cli();
tempvar=sharedvar;
sei(); 

There are also several other methods making sure words are read and written as a whole, but of course it depends on what are you are trying to achieve, which we don't know yet. Such methods include the use of a simple flag variable, which can indicate if it is safe to write or read something or not, but it only works in certain situations.

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

Your original question was formulated as if you wanted to replace the ATOMIC stuff in avrlibc with something else. You latest post shows how you encapsulate the ATOMIC stuff in something else. My reply was in the context of replacing stuff, not encapsulating. I might have been confusing .. but so was the original question.

Now that we're leveled: I agree with Dean. It depends...

A static inline function might introduce extra run-time costs.
Macro programming is always a double-egged sword - especially when the macro gets non-trivial.

YMMV.

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

Sorry for the misunderstanding...
I guess I'll use the static inline function in case I want to read/write only one variable and the block from avr-libc directly if more than one operation is needed.
Thank you all!

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

Note that it's been proven that the atomic.h macros can still be "dangerous" as the code optimiser gives no guarantees about where the CLI and SEI opcodes are actually placed. It is, however, the best solution you've got unless you are willing to code your access functions in a .S file in Asm.

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

clawson wrote:
Note that it's been proven that the atomic.h macros can still be "dangerous" as the code optimiser gives no guarantees about where the CLI and SEI opcodes are actually placed. It is, however, the best solution you've got unless you are willing to code your access functions in a .S file in Asm.

And how would be those functions in asm?
Is there any implementation available?

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

clawson wrote:
Note that it's been proven that the atomic.h macros can still be "dangerous" as the code optimiser gives no guarantees about where the CLI and SEI opcodes are actually placed. It is, however, the best solution you've got unless you are willing to code your access functions in a .S file in Asm.
I thought that someone had written a macro to do just what the OP wanted.
It expanded to inline assembler, of course.
It's not that hard.

Iluvatar is the better part of Valar.

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

I've just concocted these two:

#define atomic_read_word_restore(__addr)              \
(__extension__({                                      \
  uint8_t  __tmp;                                     \
  uint16_t __result;                                  \
  __asm__ __volatile__ (                              \
    "in  %[tmp], __SREG__     \n\t"                   \
    "cli                      \n\t"                   \
    "lds %A[res], %[addr]     \n\t"                   \
    "out __SREG__, %[tmp]     \n\t"                   \
    "lds %B[res], %[addr] + 1 \n\t"                   \
    : [res]  "=&r" (__result)                         \
    , [tmp]  "=&r" (__tmp)                            \
    : [addr] "p"   (&(__addr))                        \
    : "memory"                                        \
  );                                                  \
  __result;                                           \
}))


#define atomic_read_word(__addr)                      \
(__extension__({                                      \
  uint16_t __result;                                  \
  __asm__ __volatile__ (                              \
    "cli                      \n\t"                   \
    "lds %A[res], %[addr]     \n\t"                   \
    "sei                      \n\t"                   \
    "lds %B[res], %[addr] + 1 \n\t"                   \
    : [res]  "=&r" (__result)                         \
    : [addr] "p"   (&(__addr))                        \
    : "memory"                                        \
  );                                                  \
  __result;                                           \
}))

#define atomic_write_word_restore(__addr, __data)     \
(__extension__({                                      \
  uint8_t  __tmp;                                     \
  __asm__ __volatile__ (                              \
    "in  %[tmp], __SREG__      \n\t"                  \
    "cli                       \n\t"                  \
    "sts %[addr], %A[data]     \n\t"                  \
    "out __SREG__, %[tmp]      \n\t"                  \
    "sts %[addr] + 1, %B[data] \n\t"                  \
    : [tmp]  "=&r" (__tmp)                            \
    : [data] "r"   (__data)                           \
    , [addr] "p"   (&(__addr))                        \
    : "memory"                                        \
  );                                                  \
}))

The elders here would be so kind to review.

JW

[EDIT] write added [/EDIT]

Last Edited: Thu. Nov 4, 2010 - 03:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you can't live with any CLI then use ASM here you can update with MOVW that is atomic, but only works for reg's.(the interrupt can make 8 bit updates and "main" only MOVW ).
There is no easy way for more than 16 bit at the time.

In a project where I played a sound in the interrupt (fast timer) I had the "main" to set a bit, and then a while loop until it was clear'ed (the interrupt did that) then I knew that I had more than 1ms before the next interrupt and could read/write.

An other way is double buffer and flags!

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

Wek,
That's great, thanks!
I'll try those macros!

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

wek wrote:

#define atomic_read_word_restore(__addr)              \
(__extension__({                                      \
  uint8_t  __tmp;                                     \
  uint16_t __result;                                  \
  __asm__ __volatile__ (                              \
    "in  %[tmp], __SREG__     \n\t"                   \
    "cli                      \n\t"                   \
    "lds %A[res], %[addr]     \n\t"                   \
    "out __SREG__, %[tmp]     \n\t"                   \
    "lds %B[res], %[addr] + 1 \n\t"                   \
    : [res]  "=&r" (__result)                         \
    , [tmp]  "=&r" (__tmp)                            \
    : [addr] "p"   (&(__addr))                        \
    : "memory"                                        \
  );                                                  \
  __result;                                           \
}))


#define atomic_read_word(__addr)                      \
(__extension__({                                      \
  uint16_t __result;                                  \
  __asm__ __volatile__ (                              \
    "cli                      \n\t"                   \
    "lds %A[res], %[addr]     \n\t"                   \
    "sei                      \n\t"                   \
    "lds %B[res], %[addr] + 1 \n\t"                   \
    : [res]  "=&r" (__result)                         \
    : [addr] "p"   (&(__addr))                        \
    : "memory"                                        \
  );                                                  \
  __result;                                           \
}))

#define atomic_write_word_restore(__addr, __data)     \
(__extension__({                                      \
  uint8_t  __tmp;                                     \
  __asm__ __volatile__ (                              \
    "in  %[tmp], __SREG__      \n\t"                  \
    "cli                       \n\t"                  \
    "sts %[addr], %A[data]     \n\t"                  \
    "out __SREG__, %[tmp]      \n\t"                  \
    "sts %[addr] + 1, %B[data] \n\t"                  \
    : [tmp]  "=&r" (__tmp)                            \
    : [data] "r"   (__data)                           \
    , [addr] "p"   (&(__addr))                        \
    : "memory"                                        \
  );                                                  \
}))

The elders here would be so kind to review.

Would you settle for me?

"memory" is unnecessary.

Since its address is taken and it doesn't have to be an address,
I'd rename __addr __word.

I'd replace uint16_t with with typeof(__word).

Iluvatar is the better part of Valar.

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

skeeve wrote:

I wrote:
The elders here would be so kind to review.
Would you settle for me?
Sure. Thanks.

Michael wrote:
"memory" is unnecessary.

I'm not so sure. It is supposed to prevent code reordering, whatever that means (see threads on atomic.h stuff at the time of [avr-libc v1.7.0]'s labour). I know it does not do that absolutely, but let's have high hopes.

skeeve wrote:
Since its address is taken and it doesn't have to be an address,
I'd rename __addr __word.

It used to be __w before I edited the post above ;-) OK, I take it.
skeeve wrote:
I'd replace uint16_t with with typeof(__word).
Humm. OK, but what are exactly the ramifications of this?

JW

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

wek wrote:
Michael wrote:
"memory" is unnecessary.

I'm not so sure. It is supposed to prevent code reordering, whatever that means (see threads on atomic.h stuff at the time of [avr-libc v1.7.0]'s labour). I know it does not do that absolutely, but let's have high hopes.
The code that needs nonreordering is now entirely within a single __asm__ statement.
Quote:
skeeve wrote:
I'd replace uint16_t with with typeof(__word).
Humm. OK, but what are exactly the ramifications of this?
The type of the fetched thing will be the same as the original.

Edit: I just noticed the use of lds and sts rather than ld and st.
That implies that the address will have to be known at link time.
That will usually be the case for things whose atomicity must be guaranteed,
but it does preclude using the same code on more than one array element.

Hennebry, scourge of heaven, Klah and Deva

Iluvatar is the better part of Valar.

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

Perhaps I'm stupid but what is rule that make that to work?
For me you enable the interrupt after only 1 read and not both!

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

sparrow2 wrote:
Perhaps I'm stupid but what is rule that make that to work?
For me you enable the interrupt after only 1 read and not both!
avr-libc FAQ wrote:
Why are interrupts re-enabled in the middle of writing the stack pointer?
When setting up space for local variables on the stack, the compiler generates code like this:

/* prologue: frame size=20 */
push r28
push r29
in r28,__SP_L__
in r29,__SP_H__
sbiw r28,20
in __tmp_reg__,__SREG__
cli
out __SP_H__,r29
out __SREG__,__tmp_reg__
out __SP_L__,r28
/* prologue end (size=10) */

It reads the current stack pointer value, decrements it by the required amount of bytes, then disables interrupts, writes back the high part of the stack pointer, writes back the saved SREG (which will eventually re-enable interrupts if they have been enabled before), and finally writes the low part of the stack pointer.

At the first glance, there's a race between restoring SREG, and writing SPL. However, after enabling interrupts (either explicitly by setting the I flag, or by restoring it as part of the entire SREG), the AVR hardware executes (at least) the next instruction still with interrupts disabled, so the write to SPL is guaranteed to be executed with interrupts disabled still. Thus, the emitted sequence ensures interrupts will be disabled only for the minimum time required to guarantee the integrity of this operation.

Iluvatar is the better part of Valar.

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

sparrow2 wrote:
Perhaps I'm stupid but what is rule that make that to work?
For me you enable the interrupt after only 1 read and not both!
I've started a thread in the general AVR forum to highlight this issue.

JW