Opinion needed on this code that uses the ADC to modify the PWM duty cycle

[FC]

Hello,

This code reads the ADC (hardware trigger), calculates the duty cycle by scaling the adc reading by 100, and looks up the timer channel value register by table in rom.

volatile unsigned char duty_cycle;
const unsigned char pwm_table[100][2] = {   {0x00, 0x00},

                                                                           .

                                                                           .

                                                                         {0xFF, 0xFF} };

__interrupt void isrVadc(void)
{
 
  adc_reading = ADCRL;                        // clear COCO flag
 
  asm
  {
    lda adc_reading
    ldx #100
    mul
    pshx
    pulh
    ldx #100
    div
    sta  duty_cycle
  }
 
  TPMC0VH = pwm_table[duty_cycle][0];
  TPMC0VL = pwm_table[duty_cycle][1];
 

Any comments?

[bigmac]

Hello FC,

I am not sure why you need to scale the ADC reading from 0 to 99.  Since you are using an 8-bit result, why not simply divide this by 2 (shift right) to give 128 table entries for PWM value - a marginally larger table size.  A divide by 4 would give 64 table entries.

I notice that you are handling the timer data as separate high and low bytes within the table.  An alternative may be to have 16-bit entries in the pwm_table array variable.

A further comment - since you are using a substantial amount of inline assembly code within your ISR, I would tend to do the whole ISR using inline assembly, rather than mix.

See if the following code will work as intended.

volatile unsigned char duty_cycle;
const unsigned int pwm_table[128] = {0x0000,

                                    .

                                    .

                                   0xFFFF};

__interrupt void isrVadc(void)
{
  asm {
     ldx  ADCRL          // clear COCO flag

     lsrx                // range 0-127
     stx  duty_cycle

     lslx                // word index for table

     lda  pwm_table,x

     sta  TPMC0VH

     lda  pwm_table+1,x

     sta  TPMC0VL
  }

}
  
Regards,
Mac

[FC]

Inline assembly version:

asm
  {
    lda adc_reading
    ldx #100
    mul
    pshx
    pulh
    ldx #255
    div
    sta  duty_cycle
    ldhx @pwm_table
    lda  @duty_cycle, x
    sta  TPMC0VH
    lda  @duty_cycle+1, x
    sta  TPMC0VL    
  } 

The @ is required, it took me some time to figure this out!

[bigmac]

Hello FC,

Since pwm_table is the array variable, and duty_cycle is the index to the array, I might have expected the following code.  You seem to have reversed the two variables.  The lslx instruction would be necessary because each array entry is two bytes, so the x value would need to be 0, 2, 4, etc. to select the correct entry.  This is certainly the case with normal assembler.  But perhaps it may differ for inline assembly?

asm
  {
    lda adc_reading
    ldx #100
    mul
    pshx
    pulh
    ldx #255
    div
    sta  duty_cycle

    tax

    lslx

    clrh
    lda  @pwm_table, x
    sta  TPMC0VH
    lda  @pwm_table+1, x
    sta  TPMC0VL    
  } 

Regards,

Mac

Message Edited by bigmac on 2006-06-23 04:00 PM

[bigmac]

Hello again FC,

I have just noticed a further issue with your code.  You have used a divisor value of 255, rather than the expected value of 256 (potentially done by ignoring the low byte of the multiplication result).  This will give a duty_cycle index decimal value of 0, 1, 2,  ...   99, 100, a total of 101array entries.  So the size of the array would need to be increased to allow for this.

For the PWM, the values of 0 and/or 100 will probably need to be treated as special cases, and may require more than just setting the timer value.

Regards,
Mac

[FC]

Hello,

How to access arrays using high level inline assembly? The debugger gives an error saying "attempting to access unitialized memory.."  In HLI, var:1 or var:0 indicates a number offset.  But, how about arrays with a variable offset?  Putting varffset gives an error.  It must be number.

[FC]

Thanks for the responses.

The scaled ADC reading is (ADC reading * 100) / 255

The result will indicate the duty cycle, the remainder will not be used.

As Rocco mentioned, the second ldx instruction should be ldx #255.

A one dimensional array of unsigned ints is a good idea, but pointer notation will have to be used in C or complete assembly implementation for array access.

[rocco]

Hi, FC, it's me again.

That assembly code would work, but it is multiplying the ADC value by 100, and then dividing it by 100. I suspect that you want to divide it by 256, which is simply taking the high byte of the result. If I understand what you are trying to do, then here it is simplified:

volatile unsigned char duty_cycle;
const unsigned char pwm_table[100][2] = {  {0x00, 0x00},
                                                .
                                                .
                                           {0xFF, 0xFF} };
__interrupt void isrVadc(void)
{
  adc_reading = ADCRL;                     // clear COCO flag

  asm
  {
    lda     adc_reading
    ldx     #100
    mul
    stx     duty_cycle
  }

  TPMC0VH = pwm_table[duty_cycle][0];
  TPMC0VL = pwm_table[duty_cycle][1];
}