Prev: ALP for Real Time Clock using timer unit of HC12
Next: How to create a code in C++ for LM3S811 in IAR Embedded Workbench
From: ChrisQ on 30 Oct 2009 18:44
Hans-Bernhard Br�ker wrote:
> David Brown wrote:
>
>> {
>> uint8_t oldState = u8PwmReadIntState ();
>> vPwmMaskInterrupts ()
>
> That won't work, because it has the exact same problem that the concept
> of a critical section is supposed to work against. Interrupt state can
> change between these two instructions, and that would leave you
> restoring an out-of-date oldState.
>
> Ultimately, you need atomic query-and-change operations to resolve this.

In this case no. It's a single threaded system + interrupts with no rtos
where the interrupt state is never changed outside known boundaries.
Within that we don't necessarily know the current state, hence the
solution. Elsewhere, there's plenty of more conventional locks around
queues etc, but the led driver section needed something a bit smarter,
due to hardware constraints and other requirements. The overall software
load is very high and would have preferred to have hardware dma support
for the display driver, but there's unlikely to be any movement on that
for a year at least.

Anyway, there are by now probably thousands of boards out there running
this code without fault for a year or more, so I guess it must work :-)...

Regards,

Chris



From: David Brown on 31 Oct 2009 12:21
Hans-Bernhard Br�ker wrote:
> David Brown wrote:
>
>> {
>> uint8_t oldState = u8PwmReadIntState ();
>> vPwmMaskInterrupts ()
>
> That won't work, because it has the exact same problem that the concept
> of a critical section is supposed to work against. Interrupt state can
> change between these two instructions, and that would leave you
> restoring an out-of-date oldState.
>

That's a point worth thinking about - when dealing with these sorts of
issues, you need to think about /all/ possibilities, and all places when
interrupts could occur.

However, to change the interrupt state between these two instructions
would require an interrupt routine that deliberately left interrupts
disabled on exit, rather than restoring their prior state - that would
definitely be a very unusual situation for an interrupt routine. For
correctness of code like I posted, you simply have to disallow writing
such an interrupt routine. Note that I'm talking about global interrupt
enables here, as sometimes an interrupt routine will disable or enable
particular peripheral interrupts.

> Ultimately, you need atomic query-and-change operations to resolve this.

But for lots of processors, you simply don't have that kind of
operation. The only way you can get around it would be in cases when
you know for sure that interrupts are enabled before you disable them -
that way, you don't need to store the previous state.
From: David Brown on 1 Nov 2009 05:40
Stefan Reuther wrote:
> David Brown wrote:
>> Stefan Reuther wrote:
>>> All this is totally simple when you can use CLI/STI:
>>> void atomic_swap(node** a, node** b, node* c) {
>>> cli(); *a = *b; *b = c; sti();
>>> }
>>>
>>> // prepend new node to a list (easier than appending...)
>>> node* list;
>>> node* newNode;
>>> atomic_swap(&newNode->next, &list, newNode);
>>> (some strategically-placed 'volatile' can be appropriate.)
>> Here you have missed one important point that many people fail to
>> understand about interrupt disabling (and the related topic of volatile
>> accesses) - memory barriers. Unless your cli() and sti() macros /
>> assembly functions specifically include memory barriers, then your
>> atomic_swap function is not safe.
>
> Yep. The declaration should've been
> void atomic_swap(node*volatile* a, node*volatile* b, node* c);
> I was just too lazy to figure out where to place the 'volatile's :-)

I have a rule that any type consisting of more than two parts should be
typedef'ed. That makes it a little easier to see when you are talking
about a pointer to a volatile, or a volatile pointer, or whatever.
However, "volatile" in itself is not always the best solution, since it
might cause unnecessary memory accesses (as well as forcing the
necessary ones).

> That, together with cli/sti macros acting as memory barriers (honestly,
> cli/sti which are not such barriers are useless) fixes the routine for a
> single processor system. Multiprocessor is a little harder.
>

I'm just point this out because cli/sti macros do not always act as
memory barriers (/you/ may understand this, but others reading this
thread might not). They are very commonly implemented with something as
simple as "asm volatile ("cli")" - with gcc, that would not imply a
memory barrier.

>> In the atomic_swap function above, for example, the compiler will
>> probably see the cli() and sti() as volatile accesses (depending on the
>> compiler and the definition of these macros). But the code in the
>> middle does not use volatile accesses - the compiler can therefore
>> freely move these operations before or after the cli() and sti()
>> operations.
>
> Actually, our compiler moved the accesses even though I used 'volatile'.
> Fortunately the vendor fixed it quickly, but in the meantime I've made
> an asm version to fix it forever.
>

That brings up another good point - it is a good idea to check the
generated code for such critical constructs. Compiler vendors are
typically good, but they are not perfect. Sometimes in their quest for
generating smaller and faster code, they make mistakes.
First  |  Prev  | 
Pages: 1 2 3 4 5 6 7 8 9
Prev: ALP for Real Time Clock using timer unit of HC12
Next: How to create a code in C++ for LM3S811 in IAR Embedded Workbench