• State Resolved
  • Locked Locked
  • Replies 9 replies
  • Subscribers 32 subscribers
  • Views 682 views
  • Users 0 members are here
Support feedback
Options
Options
Related

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.

MSPM0G3507: Typo in SDK for FOC motor control

Robert Chen
Mastermind 9135 points

Part Number: MSPM0G3507
Other Parts Discussed in Thread: DRV8316

Tool/software:

Hi,

I am using mspm0_sdk_2_02_00_05 with DRV8316 to develop FOC motor control project.

For the following function in file "focHALInterface.c",

void HAL_SelectShuntMeasure(HAL_MEASURE_MOTOR_INPUTS_T *pMotorInputs)
{
    HAL_MEASURE_CURRENT_T *pCurrent = &(pMotorInputs->current);

    MC_PHASE_TYPES maxDutyPhase = *(pCurrent->pMaxDutyPhase);

#ifdef CURRENT_THREE_SHUNT_DYNAMIC
    if(maxDutyPhase != pCurrent->prevMaxDutyPhase &&
        pCurrent->flags.b.threeShuntDynamic)
    {
        /* Max duty phase has changed and dynamic update on 3
         * shunt is enabled */
        switch(maxDutyPhase)
        {
            case PHASE_A:
                pCurrent->currentShunt = CURRENT_TWO_SHUNT_B_C;
            break;

            case PHASE_B:
                pCurrent->currentShunt = CURRENT_TWO_SHUNT_A_C;
            break;

            case PHASE_C:
                pCurrent->currentShunt = CURRENT_TWO_SHUNT_A_B;
            break;

            case PHASE_INVALID:
                pCurrent->currentShunt = pCurrent->currentShuntSet;
            break;
        }

        HAL_SetPhaseCurrentChannels(pMotorInputs);
    }
#endif
}

I think

#ifdef CURRENT_THREE_SHUNT_DYNAMIC

should be

#ifdef __CURRENT_THREE_SHUNT_DYNAMIC

It is a typo, right?

Robert.

  • 0
    TI__Expert 7825 points

    Hi Robert, thanks for reaching out. Let me look into this and I will get back to you shortly with an answer.

    -Brian

  • 0
    TI__Expert 7825 points

    Hey Robert,

    I looked into this a little deeper and found the #ifdef you are referring to. I think this is okay either way, but you can add in the pre-processor guards if you choose (I have verified it compiles correctly both ways on my setup). You may consider adding this if you will have several hardware configurations and want to reduce the code size and the compilation time, but you shouldn't see an issue with the example running correctly as is. Hope this helps!

    -Brian

  • 0 in reply to Brian Dempsey
    Mastermind 9135 points

    Thanks for your reply.

    I think the meanings of both are fundamentally different. CURRENT_THREE_SHUNT_DYNAMIC is defined as an enum value of 1 while __CURRENT_THREE_SHUNT_DYNAMIC is defined by #define for conditional compilation.

    CCS editor's syntax parser will treat CURRENT_THREE_SHUNT_DYNAMIC as undefined and grayed out while __CURRENT_THREE_SHUNT_DYNAMIC is defined and not grayed out as shown in the following two images.

    So, the final result will depend on the behavior of the compiler. If the Clang compiler produces the same result for both cases then it will produce incorrect result for projects which don't use "dynamic three shut current" since CURRENT_THREE_SHUNT_DYNAMIC is always defined while __CURRENT_THREE_SHUNT_DYNAMIC isn't defined.

    So, it is still essential to fix it for the next SDK release.

    Robert.

  • +1 in reply to Robert Chen
    TI__Mastermind 31225 points

    Hi Robert,

    I agree with you. It should be the macro definition you posted here. Or it will make the code running out of expectation.

    I will feedback to software team to double check this, and if it is the bug, it will be fixed in next SDK release version.

    B.R.

    Sal

  • 0 in reply to Sal Ye
    Mastermind 9135 points

    Hi Sal,

    I also found another bug. Please check this, too. In file pi.h, the KP_TERM_SAT macro definition

    #define KP_TERM_SAT ((int32_t)1) << (30 - GLOBAL_IQ + KP_IQ_SCALING)
    should be braced:
    #define KP_TERM_SAT (((int32_t)1) << (30 - GLOBAL_IQ + KP_IQ_SCALING))
     
    Otherwise, the compiler will complain "shift a negative value..." in piRun() and piRun_LowPriority(). This is because a negative sign is used:

    kpTerm = _IQsat((_IQmpy_mathacl(error, pPI->kp)), KP_TERM_SAT, -KP_TERM_SAT);

    Robert.
  • 0 in reply to Robert Chen
    TI__Mastermind 31225 points

    Hi Robert,

    Maybe it is compiler specific warning, I got the current one when I look into the macro definition without warning:

    [Update: this is wrong but no compiler warning here.]

    But you are correct, additional brackets will be more standardized. “-” has a higher priority compared to "<<", which has some risks here.

    Will pin your suggestion to them within this thread. Thanks for your feedback.

    B.R.

    Sal

  • +1 in reply to Robert Chen
    TI__Mastermind 31225 points

    Hi Robert,

    I got the feedback from software team that the bug will be fixed in next released. Thanks for feedback.

    As for the macro definition of KP_TERM_SAT, from my point of view, it is better to fixed also.

    While, software team think it could be handled by compiler, so they consider keeping the unchanged.

    B.R.

    Sal

  • 0 in reply to Sal Ye
    Mastermind 9135 points

    Hi Sal,

    Thanks for the notification. For KP_TERM_SAT issue. Let's see some examples,

    -1 << 10 = -1024

    -(1 << 10) = -1024

    -4 >> 1 = -2

    -(4 >> 1) = -2

    -5 >> 1 = -3

    -(5 >> 1) = 2

    The results of left-shifting are the same. This is the case for KP_TERM_SAT. But the results might be different for right-shifting. As the warning message given by the compiler, it is no meaning to shift a negative value.

    I still recommend software team to fix this issue.

    Robert.

  • 0 in reply to Robert Chen
    TI__Mastermind 31225 points

    Hi Robert,

    There is some difference for right-shifting which I do not know before. Thanks for clarification.

    I will do some test and forward this message again.

    B.R.

    Sal