The |(0xff) part of this line

 CCP1CON = ((LSB[x]<<4)|(0xff));

looks like a mistake. In fact loading anything to CCP1CON that you've read in via an A/D converter seems very odd.

The CCP1CON contains two last LSBs of the pwm.

I tried in real hardware, the distortion is unbearable, and the delay seems to be minimal.

Maybe I really need to learn dsPIC..

Thanks again all.

 

Yes, but the |(0xff) part of the line will set every bit to 1, regardless of what was in LSB[x].

 

Hi all,
This is what I've come out with.
Apparently it does shift the phase, but with excessive distortion.
The array maximum size allowed by the compiler is somehow 96.

while(1)
    {
        GO = 1;
        while(GO);
        
        MSB[x] = ADRESH;
        LSB[x] = ADRESL;

        x++;

        if (x>=97)
        {
            x = 0;
        }

        CCPR1L = MSB[x];
        CCP1CON = ((LSB[x]<<4)|(0xff));
    }

The A2D ADCON1.ADFM should be configured to be 0 for a left justified reading, where ADRESH holds the most significant byte of data, and ADRESL holds their least 2 significant bits. These two bits are in the highest 2 slots, so you need to shift them right 2 bits to align with CCP1CON.CCP1X and Y.

Then you can use: CCP1CON = ((LSB[x]>>2)|(0x0F));

to set the PWM least 2 bits.

 

Then you can use: CCP1CON = ((LSB[x]>>2)|(0x0F));
to set the PWM least 2 bits.

Shouldn't it be:

 CCP1CON &= 0xCF;
 CCP1CON |= ((LSB[x]>>2) & 0x30);

As not to overwrite other bits.

<edit> The & 0x30 is not actually needed</edit>

 

AfdhalAtiffTan, I hope it's clear now that you've made various mistakes, so you really haven't given your design a fair trial. Fix it up and try again.

A lot hangs on the frequency the incoming waveform, and how often you sample it, then what the frequency of the output PWM signal is. The timing may just not work for audio.

 

Shouldn't it be...

It's mostly a matter of style. CCP1CON needs two things: the 2 lowest bits of the PWM and the 4 bits to set the PWM mode.

Now the PWM mode is set by the pattern of 11xx so anything between 0x0C and 0x0F works.

I prefer to set a register in one shot, as opposed to setting it once to clear some bits then setting it a second time to fill those bits. With this register there is the possibility of a cycle having incorrect least significant bits set for one cycle. There may be more significant side effects with other registers, which is why I prefer to set in one go and not worry.

As far as what is the most efficient way to do this... it may be just as simple as I suggest, but I would not be surprised if there was a simpler way. But not much simpler, a few less machine instructions for an occasional task. That's not something I look to optimize.

 

You're right, it seemed odd at first cause I didn't checked what the PWM setting was. The probability of incorrect least significant bits is the same with both methods though, sincce they take the same # of operations (after taking out & 0x30). With mine they're more likely to be zeroes vs the last value with yours.

 

You can write the CCP1CON control bits and the two LSB's into a single "shadow variable", then write that one shadow byte to the CCP1CON in a single cycle.

 

Implicitly that is what EarnieM did but the variable is a register one. A memory variable if not optimized away by the compiler takes more cycles overall. If the rest of the code is correct and the clock chose appropriately it shouldn't happen though.

In this case I see that AfdhalAtiffTan may be writing to the period registers more than once per cycle so it's possible this is happening. Also I'm not very familiar with the ADC on this part but I think while (GO); is only waiting for the conversion to complete, shouldn't there be a sampling delay before setting GO?

 

while(1)
    {
        GO = 1;
        while(GO);
        __delay_us(40);
        
        MSB[x] = ADRESH;
        LSB[x] = ADRESL;

        x++;

        if (x>=70)
        {
            x = 0;
        }

        CCPR1L = MSB[x];
        CCP1CON = ((LSB[x]>>2)|(0x0f));
    }

Thanks all, I just realized my careless mistakes.
I get used to set the adc for right-justified, thanks ErnieM!

As fernan82 advice, I've reduced the counter to 70 to avoid error.

I probe the output and noticed that the adc only sample positive-half of the cycle,
then I add a voltage divider, biasing it to 1/2Vcc for full swing.
The audio is then fed via decoupling cap to remove any dc-component.

I retry it in hardware and surprised the distortion is now nearly gone.
10-bit audio is not that bad.

I roughly measure the delay with my scope and gets close to 3ms of delay.

Then I try this:

    while(1)
    {
        //__delay_us(100);
        if(x<=79)
        {
            GO = 1;
            while(GO);

            MSB0[x] = ADRESH;
            LSB0[x] = ADRESL;

            x++;

            if (x>=80)
            {
                CCPR1L = MSB0[0];
                CCP1CON = ((LSB0[0]>>2)|(0x0f));
            }
            else if (x<=79)
            {
                CCPR1L = MSB0[x];
                CCP1CON = ((LSB0[x]>>2)|(0x0f));
            }
        }

        else if(x<=159)
        {
            GO = 1;
            while(GO);

            MSB1[x-80] = ADRESH;
            LSB1[x-80] = ADRESL;

            x++;

            if (x>=160)
            {
                CCPR1L = MSB1[0];
                CCP1CON = ((LSB1[0]>>2)|(0x0f));
            }
            else if (x<=159)
            {
                CCPR1L = MSB1[x-80];
                CCP1CON = ((LSB1[x-80]>>2)|(0x0f));
            }
        }

        else if(x>=160)
        {
            x=0;
        }
    }

I make the max array size to 80 as John P point to me.
It works wonderfully! Now I achieve delay of almost 30ms.

I guess that's the highest delay I can do.

@THE_RB
Can you describe more about shadow variable? I never know about it before.
Am I doing the 'bucket brigade delay' right?


Thanks again all.

 

A shadow variable is one memory location that "shadows" the value in one of the hardware registers. One main use is for Port registers where no corresponding Latch register exists.

Doing certain operations on Port registers can cause problems, such as setting one bit. This is due to how the register gets read, modified, then written back into. The problem is called "read-modify-write" or RWM for short.

One very good work-around for this problem is to use a shadow register: do all your modifications on the shadow register, and when complete copy that value to the Port register.

As there is no R on the Port you can't get a RMW error.

As far as setting CCP1CON goes, since you can completely determine the new value without having to read the old value, no shadow register is necessary. It is just a waste to use one.

 

...
Can you describe more about shadow variable? I never know about it before.
...

That was a general suggestion for a way to write to CCP1CON in minimum cycles and avoid the error where the PWM is updated during the write.
Your code
CCPR1L = MSB1[x-80];
CCP1CON = ((LSB1[0]>>2)|(0x0f));
takes quite a bit of time to do ((LSB1[0]>>2)|(0x0f)) before it writes the two LSBs, so there is a delay between writing CCPR1L and then later writing the two LSBs.

That delay is caused by two right shifts and a OR, and the relative indexing to get the value from LSB1[]. That might be 10 or 15 instructions total (depends on PIC type and compiler etc).

If you do it like this;
unsigned char shad_CCPR1L; // make a shadow variable
shad_CCPR1L = MSB1[x-80];
CCP1CON = ((LSB1[0]>>2)|(0x0f));
CCPR1L = shad_CCPR1L;

Then there are only about 2 instructions between loading the LSBs and the CCPL. So you get more accurate PWM.

...
Am I doing the 'bucket brigade delay' right?
...

I don't like the way you are doing it. First you have redundancy in your if/else. Please look up the help file for how to use if/else.

The main change I would make is to use two separate index values (instead of just x);
index_in (like x, controls where the input data goes into the circular array).
index_out (tells where to pull data out of the array).

Then by adjusting the difference between both indexes you can adjust the delay.

your code would then look more like this (simplified for clarity);

index_in = 0;
index_out = 159; // set delay, difference between the two indexes
while(1)
{
  // input new sample into array
  array[index_in] = ADC;
  index_in++;
  if(index_in >= ARRAY_SIZE) index_in = 0;

  // output delayed sample from array
  PWM = array[index_out];
  index_out++;
  if(index_out >= ARRAY_SIZE) index_out = 0;
}

That is a basic 8bit delay, your code is a bit more complex because you have 10bit ADC and 10bit PWM. That's why I suggested getting it going with 8bit samples in my earlier post. Once working well in 8bit you can then expand to 10 bit.

 

Hi again, I made a mistake there in the setting up of the delay time. The index_out should be immediately after the index in, to give a delay buffer size of ARRAY_SIZE-1 (ie 159). I have edited the code below to fix it.

Also to get the full delay size of ARRAY_SIZE (ie 160) you can do the PWM out first, and the array loading second. In that case you can set both index_in and index_out tot he same value. (see below);

index_in = 0;
index_out = (ARRAY_SIZE - 159); // set delay to 159. (zero here gives full array size)
while(1)
{
  // this delay per sample sets the main delay (* index difference)
  Delay_uS(20);

  // output delayed sample from array
  PWM = array[index_out];
  index_out++;
  if(index_out >= ARRAY_SIZE) index_out = 0;

  // input new sample into array (do this second, allows full index size if needed)
  array[index_in] = ADC;
  index_in++;
  if(index_in >= ARRAY_SIZE) index_in = 0;
}

I also added the delay per sample.

For a precise delay you should use a hardware timer, so that the time taken by the other code does not add onto the delay time.