On 17/04/2012 13:44, Peter Bigot wrote:
> On Tue, Apr 17, 2012 at 5:02 AM, David Brown<da...@westcontrol.com> wrote:
>> On 17/04/2012 09:53, Wayne Uroda wrote:
>>> Hi,
>>>
>>> I feel quite foolish asking the following question... Please don't judge me
>>> too harshly...
>>>
>>> In my MSPGCC based system I have got the following #defines which I use for
>>> critical (interrupts disabled) access around volatile memory which is
>>> modified by interrupt routines:
>>>
>>> #define criticalStart() __asm__ __volatile__("push r2"::); __asm__
>>> __volatile__("dint"::)
>>> #define criticalEnd() __asm__ __volatile__("pop r2"::)
>>>
>>
>> /Never/ mess with the stack inside a C function. You are asking for
>> trouble. Simply save the old SR in a local variable (as you do in the
>> new versions below).
>>
>>> Usually I use this code to write to a counter which is decremented by my
>>> timer ISR, eg
>>>
>>> void setTimeout(UInt16 timeout)
>>> {
>>> // Other work before setting the volatile, ISR driven value
>>> criticalStart();
>>> busyTimeout = timeout;
>>> criticalEnd();
>>> }
>>
>> Of course, it is pointless disabling interrupts here. The msp430 does
>> the 16-bit write as an atomic operation anyway. If there is only one
>> instruction, it cannot be interrupted. You only need the critical
>> section if you are trying to do a 32-bit write.
>>
>>>
>>> Today I am using Code composer for a new project and I tried a similar
>>> trick. After several hours of debugging, it turns out that my little "push
>>> r2" trick causes code composer to access local variables incorrectly, since
>>> these are accessed relative to the stack pointer.
>>>
>>> For example, the following code does not function correctly because the
>>> "localTimer" variable is not written correctly (the memory access is one
>>> word off owing to the extra stack push).
>>>
>>> while(true)
>>> {
>>> UInt32 localTimer;
>>> criticalStart();
>>> localTimer = globalTimer;
>>> criticalEnd();
>>> if (localTimer == 0)
>>> {
>>> break;
>>> }
>>> }
>>>
>>>
>>> My question is, does GCC analyse the inline assembly code and notice that I
>>> have altered the stack, or have I just been *extremely* lucky for many
>>> years? Certainly code composer does not see that I have modified the stack.
>>
>> You've just been lucky.
>>>
>>> I am going to change my critical macros like so (this is how I am now doing
>>> critical access in code composer, which, unless I am missing something, has
>>> no builtin way to do critical functions):
>>>
>>> #define criticalStart() { UInt16 __oldInterruptStatus__ = READ_SR();
>>> __asm__ __volatile__("dint"::)
>>> #define criticalEnd() WRITE_SR(__oldInterruptStatus__); }
>>>
>>
>> That should work. Like many people (and all serious coding standards),
>> I don't like putting brackets in macros - but that's your choice.
>
> Nothing wrong with brackets in macros. Mismatched brackets that
> require macro pairs, now---that's a Bad Thing(tm).
That's what I meant, though it's not what I wrote - it is /mismatched/
brackets that are the problem. Sometimes they sound like a good idea,
such as in this case, but they are always a bit risky.
>
>>
>>> Any major issues with doing this? Should I instead wrap all reads and
>>> writes to volatile memory/variables with critical functions? I usually only
>>> wrap one line of code with the critical macros.
>>>
>>
>> You would be better off learning what critical sections actually /are/,
>> and when you need them, rather than wasting time, effort, code space and
>> run time when they are not necessary. As a very general indication, you
>> need interrupt disabling like this if you have code that needs atomic
>> access to several parts of a complex memory structure, and the code
>> could be interrupted by other code that also then accesses the same
>> structure.
>>
>> In particular, you don't need them within interrupt functions (or other
>> places with interrupts disabled), and you don't need them to access data
>> that the cpu can access atomically (like 8-bit and 16-bit data).
>
> One clarification: you don't need a critical section around a data
> access that the mcu *does* atomically. That it could (even should)
> have been atomic, and the compiler didn't make it so, should always be
> taken into account. Trust, but verify.
Good point.
You have to be particularly careful about read-modify-write codings.
The msp430 can do some of these actions atomically, but it is possible
that the compiler will split the actions up. For example, if you write
"a += 1; b = a" then then compiler might read "a" into a register, do
the addition, then save the value to "a" as three separate actions, so
that it can re-use the saved value for "b".
>
>>> Also if anybody knows code composer v3.1 and knows a better way, please let
>>> me know.
>>> Thanks,
>>>
>>
>> With gcc for the msp430, there is also a "critical" function attribute -
>> you can put your critical accesses within a small static inline
>> "critical" function.
>
> Interesting thought: I haven't tried to see whether the "critical"
> attribute (which is expanded when the function prolog and epilog are
> emitted) is preserved if a function is inlined. Does that actually
> work?
>
As far as I remember, it works fine. However, I haven't actually used
mspgcc much for the last few years, and the projects using it are all on
an older gcc 3.x version - I can't comment about current versions. And
most of my uses of "critical" functions were not declared inline - but
as single-use static functions, I expect that some of them were inlined.
Would it ever be possible to get the "critical" attribute pushed into
mainline gcc? I'm sure other ports would benefit from the same idea
(with the avr port being the obvious example). It always struck me as
silly that all the different ports for gcc define their own attributes
for things like interrupt functions or critical functions, rather than
combining them.
mvh.,
David
> Peter
>
>> With Code Composter 3.1, the best option is to throw away the tools and
>> use something else. The toolchain has major problems, and you need to
>> work hard to get around them. At the very least, use CCS 5, as it has a
>> much better IDE. But the toolchain still has the critical failing that
>> it doesn't zero out uninitialised statically allocated data, and it has
>> the most appalling choices you could imagine of which issues are
>> warnings, errors, or ignored. It will give errors or warnings on
>> perfectly good code, while accepting total crap without question.
>>
>>
------------------------------------------------------------------------------
Better than sec? Nothing is better than sec when it comes to
monitoring Big Data applications. Try Boundary one-second
resolution app monitoring today. Free.
http://p.sf.net/sfu/Boundary-dev2dev
_______________________________________________
Mspgcc-users mailing list
Mspgcc-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mspgcc-users
