On Tue, Apr 17, 2012 at 9:03 AM, David Brown <da...@westcontrol.com> wrote:
> 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.
I doubt it would ever get taken up officially; the implementation and
interface is always in the target-specific code. The good back ends
try to see how the other ones did it and stay as consistent as
possible.
Peter
> 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
