• State
  • Locked Locked
  • Replies 4 replies
  • Subscribers 78 subscribers
  • Views 570 views
  • Users 0 members are here
Support feedback
Options
Options
Similar topics

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

msp430fr59xx_adc12_10.c bug?

Nick de Smith
Intellectual 775 points

Tiny issue: in the msp430fr59xx_adc12_10.c that comes with CCS6, we have:

[110] __bis_SR_register(LPM0_bits + GIE); // LPM4 with interrupts enabled

Either the comment is wrong or the code is.

In the ISR we have:

[146] __bic_SR_register_on_exit(LPM4_bits); // Exit active CPU

Cheers

Nick

  • 0
    Guru 46710 points

    LMP4_bits clear on exit can be used universally for any LPM mode. Look at the LMP0_bits and LPM4_bits definitions - you will see.

    Possibly during development for some reason it was decided to not use LPM4 but LPM0 instead. So basically problem is just misleading comment for line 110

  • 0 in reply to Ilmars
    Intellectual 775 points

    Yup - realise that - however, examples are supposed to be just that - exemplars.

    The comment should be changed and the ISR should only clear LPM0 - the fact that clearing the extra bits in LPM4 is a no-op is incidental - if clearing LPM4 rather than LPM0 was deliberate, it should be commented as such...

    I know I'm being picky, but these are manufacturer's examples and for a beginner it could be confusing...

  • 0 in reply to Nick de Smith
    Guru 46710 points

    Nick de Smith said:
    the ISR should only clear LPM0

    I tend to clear all the bits needed for intended result disregarding prior knowledge of which bits were actually set. Such way code is safe for future code modifications. For example imagine code having both, LPM0 and LPM4 entries

    Nick de Smith said:
    if clearing LPM4 rather than LPM0 was deliberate, it should be commented as such...

    Yes, proper comment would be great.

  • 0 in reply to Nick de Smith
    Guru 227245 points

    I second Ilmars’ suggestion to clear without prior knowledge. This is not only for LPM (and the example from entering LPM0 here and LPM4 there and exiting it in the same ISR is more than just an example - I’ve seen it happening - and failing.). It applies to all kinds of bitfield operations. Like setting a timer in CONT mode with |= MC_2, but an &=~MC_2 would turn a timer that is running in UP/DOWN mode into UP mode rather than stopping it. Same instruction, different result, depending on current state.

     

    Nick de Smith said:
    if clearing LPM4 rather than LPM0 was deliberate, it should be commented as such...
    I know I'm being picky, but these are manufacturer's examples and for a beginner it could be confusing...

    I completely agree. The comments are enough (or even too much) for an experienced user, but for a beginner who needs the examples, they are way too short.
    Best example:
    nop(); // for debugger
    This implies, the nop is there to put a breakpoint on it. However, it is there to NOT put a breakpoint on it, but allow you to put a breakpoint on the next instruction without getting triggered on LPM entry.

    Intentions and assumptions and (implicit) prerequisites should be clearly stated in the comments.
    Another example, there once was an example doing something with timer capture. The demo code only worked because on this single specific MSP, a certain output signal and the timer input were on the same pin. No chance to run it on a different MSP without an external bridge.

**Attention** This is a public forum