Feedback greatly appriciated on interrupt code

Go To Last Post
28 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello everyone!

 

I will try  to make this short and simple.

I like motor control, and i have actually constructed a VFD.

 

https://www.youtube.com/watch?v=...

https://www.youtube.com/watch?v=....

 

Now i have become a motor control fanatic blush and i have decided to make a new one with a proper pcb and some more features, same controller.

 

The pwm modulation strategy will be based on Space vector modulation.

I got working code on my main project.

 

But all i can manage is about 8khz carrier frequency at 2mhz fcy atm. (with prescaled RAM array) vs 5khz (scaling inside ISR)

 

I set up a simulator with the almost identical code from the interrupt to get cycle count.

 

#define ts 512

uint16_t array[8]=
{
    202,180,155,127,97,65,33,0
};

uint16_t value = 0, acc, ta,tb,tz,ptr, amplitude, pwm0l,pwm1l,pwm2l,pwm0h,pwm1h,pwm2h,sector,pwmTb,pwmTa,pwmTz;

void main(void)
{
    amplitude = 255;
    acc = 0;
    sector = 0;
    while(true)
    {
       __nop();
       CARRY = false;
       acc += 8000;
       if(CARRY)
       {
           sector++;
           CARRY = false;
           if(sector > 6) sector = 0;
       }
        ptr = acc >> 13;
        tb = (array[ptr] * amplitude) >> 7;
        ta = (array[7-ptr] * amplitude) >> 7;
        tz = (ts - ta - tb)>>1;
        pwmTa = ta+tz;
        pwmTb = tb+tz;
        pwmTz = ts-tz;
        switch(sector)
        {
            case 0:
                pwm0l = pwmTz & 0xFF;
                pwm0h = pwmTz >> 8;
                pwm1l = pwmTa & 0xFF;
                pwm1h = pwmTa >> 8;
                pwm2l = tz & 0xFF;
                pwm2h = tz >> 8;
                break;

                case 1:
                    pwm0l = pwmTb & 0xFF;
                    pwm0h = pwmTb >> 8;
                    pwm1l = pwmTz & 0xFF;
                    pwm1h = pwmTz >> 8;
                    pwm2l = tz & 0xFF;
                    pwm2h = tz >> 8;
                break;

                    case 2:
                        pwm0l = tz & 0xFF;
                        pwm0h = tz >> 8;
                        pwm1l = pwmTz & 0xFF;
                        pwm1h = pwmTz >> 8;
                        pwm2l = pwmTa & 0xFF;
                        pwm2h = pwmTa >> 8;
                    break;

                        case 3:
                            pwm0l = tz & 0xFF;
                            pwm0h = tz >> 8;
                            pwm1l = pwmTb & 0xFF;
                            pwm1h = pwmTb >> 8;
                            pwm2l = pwmTz & 0xFF;
                            pwm2h = pwmTz >> 8;
                        break;

                            case 4:
                                pwm0l = pwmTa & 0xFF;
                                pwm0h = pwmTa >> 8;
                                pwm1l = tz & 0xFF;
                                pwm1h = tz >> 8;
                                pwm2l = pwmTz & 0xFF;
                                pwm2h = pwmTz >> 8;
                            break;

                                case 5:
                                    pwm0l = pwmTz & 0xFF;
                                    pwm0h = pwmTz >> 8;
                                    pwm1l = tz & 0xFF;
                                    pwm1h = tz >> 8;
                                    pwm2l = pwmTb & 0xFF;
                                    pwm2h = pwmTb >> 8;
                                break;
        }
        __nop();
    }

}

With the simulator at 10mhz,  i get at most 408 cycles (40,8 µs) between the NOP's. So with this i should be able to reach  19531 hz (51,2us).

in the main code i am using correct variable types lenghts, and the PWMn-L:H are representing actual pwm registers.

and i am prescaling the the pwm values in a RAM array for even quicker access.

 

compiler in free mode with optimizations disabled.

 

So my main questions is this, do you see any glaring fault or in some way i could improve the speed that i have missed or i that i simply do not know.

 

Any feedback greatly appriciated!

 

Best regards!

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

do you see any glaring fault

 

YES:

 

with optimizations disabled

Greg Muth

Portland, OR, US

Atmel Studio 7.0 on Windows 10

Xplained/Pro/Mini Boards mostly

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Greg_Muth wrote:

do you see any glaring fault

 

YES:

 

with optimizations disabled

 

Well, with optimization enabled and favoured for speed still i get at most 389 cycles (38,9us).

And it makes some unexpected jumps in the switch statement. So thats why i disabled it for now.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:
Any feedback greatly appriciated!

All of the case blocks within a switch statement should be indented to the same level !

 

Not like that:

        switch(sector)
        {
            case 0:
                :
                break;

                case 1:
                    :
                break;

                    case 2:
                        :
                    break;

                        case 3:
                            :
                        break;

                            case 4:
                                :
                            break;

                                case 5:
                                    :
                                break;
        }

 

Like that:

        switch(sector)
        {
            case 0:
                :
            break;

            case 1:
                :
            break;

            case 2:
                :
            break;

            case 3:
                :
            break;

            case 4:
                :
            break;

            case 5:
                :
            break;
        }

Or:

        switch(sector)
        {
            case 0:
                :
                break;

            case 1:
                :
                break;

            case 2:
                :
                break;

            case 3:
                :
                break;

            case 4:
                :
                break;

            case 5:
                :
                break;
        }

depending on where you like to take your breaks ...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Look at this code:

       CARRY = false;
       acc += 8000;
       if(CARRY)
       {
           sector++;
           CARRY = false;
           if(sector > 6) sector = 0;
       }

If CARRY is always false, why the if statement?

 

Greg Muth

Portland, OR, US

Atmel Studio 7.0 on Windows 10

Xplained/Pro/Mini Boards mostly

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

I'm still looking for the "interrrupt code"???

 

Jim

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

ghoetic wrote:
Any feedback greatly appriciated!

All of the case blocks within a switch statement should be indented to the same level !

 

Not like that:

        switch(sector)
        {
            case 0:
                :
                break;

                case 1:
                    :
                break;

                    case 2:
                        :
                    break;

                        case 3:
                            :
                        break;

                            case 4:
                                :
                            break;

                                case 5:
                                    :
                                break;
        }

 

Like that:

        switch(sector)
        {
            case 0:
                :
            break;

            case 1:
                :
            break;

            case 2:
                :
            break;

            case 3:
                :
            break;

            case 4:
                :
            break;

            case 5:
                :
            break;
        }

Or:

        switch(sector)
        {
            case 0:
                :
                break;

            case 1:
                :
                break;

            case 2:
                :
                break;

            case 3:
                :
                break;

            case 4:
                :
                break;

            case 5:
                :
                break;
        }

depending on where you like to take your breaks ...

 

Yes ofcourse, its the damn IDE that likes to do that so was just lazy and didnt fix it  by myself laugh

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Greg_Muth wrote:

Look at this code:

       CARRY = false;
       acc += 8000;
       if(CARRY)
       {
           sector++;
           CARRY = false;
           if(sector > 6) sector = 0;
       }

If CARRY is always false, why the if statement?

 

 

Well the CARRY(Bit) is actually a status registers bit and it might be toggled by some other operation that produces a carry in the real code. So i set it to false before doing the accumulator add, so i only look for carry bit to increment the current SVM sector.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:
I set up a simulator with the almost identical code from the interrupt to get cycle count.
But a closed loop is quite different from an interrupt. E.g. the compiler can optimize a variable to be living only in registers in a closed loop. In an interrupt the variable has to be loaded from and saved to the memory each time. And an interrupt has a prologue and an epilogue executed each time.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:
 it makes some unexpected (sic) jumps in the switch statement.

Why is that "unexpected"?

Your switch statement looks like it contains a  lot of identical, repeated code - so you should expect that to get combined!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ki0bk wrote:

I'm still looking for the "interrrupt code"???

 

Jim

 

 

The interrupt code is actually in a high priority interrupt in the main project, im just counting the cycles preteding the (While) is the interrupt.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:
Well the CARRY(Bit) is actually a status registers bit

Is it?

 

You haven't shown that in your code - so how are we expected to guess it?!

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

ghoetic wrote:
 it makes some unexpected (sic) jumps in the switch statement.

Why is that "unexpected"?

Your switch statement looks like it contains a  lot of identical, repeated code - so you should expect that to get combined!

 

Well what it does is this.

1. Jumps to the correct case.

2. Goes through all the statements. (sometimes it goes to another case and performe the same statement)

3. Goes  to case X, goes through the same statement, then break. example ( performes last line of case 4 "pwm2h = pwmTz >> 8;", jumps to the same line in case 3 "pwm2h = pwmTz >> 8;", then break. Doing the same statement twice.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well the CARRY(Bit) is actually a status registers bit and it might be toggled by some other operation that produces a carry in the real code.

Let me see if I understand you.  Your code clears the carry bit in SREG, you do some arithmetic, then you check the carry bit in the SREG?  Is this correct?

Greg Muth

Portland, OR, US

Atmel Studio 7.0 on Windows 10

Xplained/Pro/Mini Boards mostly

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

ghoetic wrote:
Well the CARRY(Bit) is actually a status registers bit

Is it?

 

You haven't shown that in your code - so how are we expected to guess it?!

 

 

Oh, sorry about that. "CARRY" is the compilers built in name for the bit.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Greg_Muth wrote:

Well the CARRY(Bit) is actually a status registers bit and it might be toggled by some other operation that produces a carry in the real code.

Let me see if I understand you.  Your code clears the carry bit in SREG, you do some arithmetic, then you check the carry bit in the SREG?  Is this correct?

 

Yes! it's readable and writeable. :)

All i am looking for is for overflow of the unsigned 16bit variable acc.

Last Edited: Tue. Oct 31, 2017 - 05:28 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:
All i am looking for is for overflow of the unsigned 16bit variable acc.
Messing around with the carry bit in C is a quite bad idea.

Either do it the "C way"

       acc += 8000;
       if (acc < 8000)  // acc overflowed

or if you want to program in "assembler style" then simply use assembler.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

sternst wrote:

ghoetic wrote:
All i am looking for is for overflow of the unsigned 16bit variable acc.
Messing around with the carry bit in C is a quite bad idea.

Either do it the "C way"

       acc += 8000;
       if (acc < 8000)  // acc overflowed

or if you want to program in "assembler style" then simply use assembler.

 

Ah, sorry for my bad explenation i forgot to mention the "ACC" variable will gets its "deltaACC" value from the raw 10bit ADC multiplied with step size constant, so i can control the speed/frequency of the rotating vector.

Last Edited: Tue. Oct 31, 2017 - 05:42 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:
Ah, sorry for my bad explenation i forgot to mention the "ACC" variable will gets its "deltaACC" value from the raw 10bit ADC multiplied with step size constant,
So what?

       acc += someVariableDerivedFromSomeAdcReading;
       if (acc < someVariableDerivedFromSomeAdcReading)  // acc overflowed

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

sternst wrote:

ghoetic wrote:
Ah, sorry for my bad explenation i forgot to mention the "ACC" variable will gets its "deltaACC" value from the raw 10bit ADC multiplied with step size constant,
So what?

       acc += someVariableDerivedFromSomeAdcReading;
       if (acc < someVariableDerivedFromSomeAdcReading)  // acc overflowed

 

Oh is see, i just checked, and your right. I thought 65535 + 1 would be 1, but its actually 0, so it will always be one less. Thank you! :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

"CARRY" is the compilers built in name for the bit.

Which compiler are you using?

I want to see the actual ISR code.

You have a log of shfit/mask operations that ought to optimize to bytewise operations on an 8bit CPU, but I'm not sure that I have any confidence that that they would actually do so when using some arbitrary compiler.

Moving the code to a loop and using simulation seems like a horribly error-prone way of analyzing the code, compared to, say, actually looking at the ASM code that the compiler produces.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ghoetic wrote:

compiler in free mode with optimizations disabled.

 

So I'm guessing that we're not talking about an AVR here are we? Plus, you mention a PIC in one of the videos.

'This forum helps those who help themselves.'

 

pragmatic  adjective dealing with things sensibly and realistically in a way that is based on practical rather than theoretical consideration.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I hope its okay here, as i mainly wanted to check the actual C code for some real pitfalls and improvements. But that might be hard without knowing the compiler and assembly output.

 

Well yes, i am using a PIC and microchips IDE MPLAB X, with XC8 compiler. I am sorry if this offends anyone or if you think its not the right place.

I thank you all for taking the time, and i like that you have a thriving community that i hope to be apart off :).

 

And i have to add, i am horrible with ASM. I basically know about the concepts, so i can't personally look at ASM output and determine if its good or bad. Do you think there is something to gain from learning assembly?

I have tried to look around the general mindset and get a grasp of the "C vs ASM" debate, and i found that "most" users, not all, came to the same conclusion that "Today's compilers generally generate ASM that you cant beat/optimize, and that it's basically not worth learning it.
This is probably neither 100% correct or wrong i guess. Thoughts?

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

westfw wrote:

"CARRY" is the compilers built in name for the bit.

Which compiler are you using?

I want to see the actual ISR code.

You have a log of shfit/mask operations that ought to optimize to bytewise operations on an 8bit CPU, but I'm not sure that I have any confidence that that they would actually do so when using some arbitrary compiler.

Moving the code to a loop and using simulation seems like a horribly error-prone way of analyzing the code, compared to, say, actually looking at the ASM code that the compiler produces.

 

 

I made a post with some information, but for the real ISR code here goes nothing.

Some points:

 

1. I started out trying to keep it modular, so this is one function that i call from the high priority interrupt, i will not do this in the final code as it is not recommended to do function calls.

2. I have not touched this in a couple of months, so i have not yet done the "C" way of checking for overflow as proposed by STERNST.

3. Any questions or improvements just shoot :)

void high_priority interrupt isr_high(void)
{

    if(PTIF && PTIE) 
    {
        PTIF = false;
        Modulate();
    }
}
void Modulate(void)
{
    VectorAngle += VectorUpdateStepSize; // sum the vector update step size for vector angle table index
    if(CARRY) // Check for carry bit set by overflow of vector_angle
    {
        sector++; // increment sector
        CARRY = false; // reset status bit
    }
    if(sector > 5)
    {
        sector = 0; // reset sector to 0
    }
        AngleIndex = HIGH_BYTE(VectorAngle); //  Assign index variable by shifting right 8 bits (65535 divide by 256) > 0-255
        T2 = PreScaledAngle[AngleIndex]; // angle 0-60
        T1 = PreScaledAngle[REVERSE_ANGLE_INDEX]; // angle in reverse 60-0
        if(modulation_index >= IDEAL_MODULATION && AngleIndex < 128 && T2 > OvermodulationAngle1)
        {
            T2 = OvermodulationAngle1;
            T1 = PWM_PERIOD - T2;
        }
        if(modulation_index >= IDEAL_MODULATION && AngleIndex > 128 && T2 < OvermodulationAngle2)
        {
            T2 = OvermodulationAngle2;
            T1 = PWM_PERIOD - T2;
        }
        T0 = (PWM_PERIOD - T1 - T2)  >>1; // Calculate T0 time and right shift 1 bit (divide by 2) for half T0 time.
        T7 = (PWM_PERIOD - T0); // Calculate the reverse of half T0 time;
  
    switch(sector) // Determine switching pattern based on sector, Standard SVPWM
    {
        case 0:
            pwm0 = T0;
            pwm1 = T0+T1;
            pwm2 = T7;
            break;
        
        case 1:
            pwm0 = T0+T2;
            pwm1 = T0;
            pwm2 = T7;
            break;
        
        case 2:
            pwm0 = T7;
            pwm1 = T0;
            pwm2 = T0+T1;
            break;
        
        case 3:
            pwm0 = T7;
            pwm1 = T0+T2;
            pwm2 = T0;
            break;
        
        case 4:
            pwm0 = T0+T1;
            pwm1 = T7;
            pwm2 = T0;
            break;
        
        case 5:
            pwm0 = T0;
            pwm1 = T7;
            pwm2 = T0+T2;
            break;
    }
    
    
    UDIS = 1; // Disable PWM updates until duty cycles values are loaded into register
    PDC0L = LOW_BYTE(pwm0) ;
    PDC0H = HIGH_BYTE(pwm0);
    PDC1L = LOW_BYTE(pwm1) ;
    PDC1H = HIGH_BYTE(pwm1);
    PDC2L = LOW_BYTE(pwm2) ;
    PDC2H = HIGH_BYTE(pwm2); 
    UDIS = 0; // Enable PWM updates when done
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Any comments on optimisation etc are moot as the pic8 is rather hostile in the support of C, so what works on an AVR is most likely not applicable to pic8.
Realistically why would you use a pic8 these days? There’s better, faster, cheaper options available that avoid many problems - specific timer features and higher speed processing with better development support for less dollars.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
Any comments on optimisation etc are moot as the pic8 is rather hostile in the support of C, so what works on an AVR is most likely not applicable to pic8. Realistically why would you use a pic8 these days? There’s better, faster, cheaper options available that avoid many problems - specific timer features and higher speed processing with better development support for less dollars.

 

Okay,  to be specific  its 18F4431, still in production and has some good hardware modules, also alot of errata meh, but main reason i am using it now is because i bought 2 when i did the first prototype smiley.

Concidering how fun this type of application is will probably try a dspic and another vendor controller for sure.

 

But overall then the "C" looks okay? no really obvious faults?

Thanks for your input.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The C looks "ok", I guess.  I shudder to think of the PIC code produced :-(

i get at most 408 cycles

 Are you sure that that's clock cycles, and not instruction cycles?  I guess I could imagine ~100 instructions, since a lot of the bulk is cases where only one is executed.  But I'm not sure I believe that the free C compiler would do that...

 

You could ask on PICList  ( http://piclist.com ), but arduino seems to have taken the wind out of the lively code optimization discussions that used to go on (though those were rarely for C.)

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I was looking around again, and i actually managed to cut down my cycles in half laugh. I substituted all shifts with this handy define

#define acc_lo (*((uint8_t*)&acc))
#define acc_hi (*((uint8_t*)&acc+1))

made "ptr" to uint8_t and assigned it acc_hi, and for the reverse lookup i bit xor it with 255. same with subtractions instead of" tz = (ts-ta-tb)>>1" i use" tz = ((ta_hi+tb_hi)^0xFF)>>1" and just invert or xor  instead of "ts-tz"

so now im down to at most 202 cycles. 20,2us.  Hopefully there is more to be gained!

 

 

Last Edited: Thu. Nov 2, 2017 - 07:13 PM