"pure" C programming: PWM issues

Hi! I've been trying to learn a little bit about low(er?) level programming so I'm quite new to this.
I'm trying to recreate a sketch I made that takes a reading from a potentiometer and treats it as a hue to calculate RGB values to then output on an RGB LED. The original worked fine, and all my code seems to work fine seperately, it's just putting it together that seems the issue. Here is my code ("colours.c"):

#include <avr/io.h>
#include <util/delay.h>

#define RED_PIN PB1   // OC1A (9)
#define GREEN_PIN PB2 // OC1B (10)
#define BLUE_PIN PB3  // OC2A (11)
#define INPUT_PIN PC5 // ADC5 (A5)

int main(void) {
  // Set up for ADC
  ADCSRA |= _BV(ADPS2) | _BV(ADPS1) | _BV(ADPS0); // Set prescaler to 128
  ADCSRA |= _BV(ADEN); // Enable analog to digital conversion

  // Set pins for output
  DDRB |= _BV(RED_PIN) | _BV(GREEN_PIN) | _BV(BLUE_PIN);

  // Set up for PWM
  // Set up 8-bit Fast PWM for OC1x outputs
  // See https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061B.pdf#page=141&zoom=auto,-98,455
  TCCR1A |= _BV(WGM10);
  TCCR1B |= _BV(WGM12);
  // No prescaling
  TCCR1B |= _BV(CS10);
  // Output on OC1A and OC1B when counter reaches compare value
  // See https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061B.pdf#page=140&zoom=auto,-98,61
  TCCR1A |= _BV(COM1A1) | _BV(COM1B1);

  // Set up 8-bit Fast PWM for OC2x outputs
  // See https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061B.pdf#page=164&zoom=auto,-98,539
  TCCR2A |= _BV(WGM20) | _BV(WGM21);
  // Output on OC2A when counter reaches compare value
  TCCR2A |= _BV(COM2A1);
  // No prescaling
  TCCR2B |= _BV(CS20);

  // loop forever
  while (1) {
    float h = 0, s = 1, v = 0.5;

    int input = analog_read(INPUT_PIN);

    h = input * 360.0 / 1024;

    int r = 0, g = 0, b = 0;

    HSVtoRGB(h, s, v, &r, &g, &b);

    OCR1A = r;
    OCR1B = g;
    OCR2A = b;

    _delay_ms(5);
  }
}

// h = [0, 360]; s = [0, 1]; v = [0, 1]
// r = [0, 255]; g = [0, 255]; b = [0, 255]
// See https://www.rapidtables.com/convert/color/hsv-to-rgb.html
void HSVtoRGB(float h, float s, float v, int *r, int *g, int *b) {
  float C = v * s;
  float X = C * (1 - abs(fmod(h / 60.0, 2.0) - 1));
  float m = v - C;

  int H = h / 60;
  float R, G, B;

  switch (H) {
    case 0:
      R = C;
      G = X;
      B = 0;
      break;

    case 1:
      R = X;
      G = C;
      B = 0;
      break;

    case 2:
      R = 0;
      G = C;
      B = X;
      break;

    case 3:
      R = 0;
      G = X;
      B = C;
      break;

    case 4:
      R = X;
      G = 0;
      B = C;
      break;

    case 5:
      R = C;
      G = 0;
      B = X;
      break;
  }

  *r = (R + m) * 255;
  *g = (G + m) * 255;
  *b = (B + m) * 255;
}

int analog_read(int pin) {
  // Select pin in multiplexer and set reference to VCC (ADMUX)
  ADMUX = (1 << REFS0) | (pin & 0x07);

  // Start the analog to digital conversion
  ADCSRA |= (1 << ADSC);

  // Wait for the read to finish. ADSC is kept high while the conversion is in progress
  while (ADCSRA & (1 << ADSC));

  // Get high and low bytes of reading into one integer
  // Must read ADCL first!
  uint8_t low = ADCL;
  uint8_t high = ADCH;
  return (high << 8) | low;
}

I've tested my HSVtoRGB and read_analog functions a lot and they seem to work fine in general and in conjunction with each other (in a sketch that handles all the PWM using Arduino's analogWrite).
Additionally, I've tested that the PWM works in general by just writing constants to OCR1A, OCR1B and OCR2A respectively to make sure the right values were being output, and it seems that that works fine.
I also tested by scaling input to 0-255 and writing that directly to OCR1A, OCR1B and OCR2A and that also worked.

The problem only comes up when I try to write r, g, and b to OCR1A, OCR1B, and OCR2A, which results in really weird stuttery colours, as opposed to the nice smooth transition I had in my original and in the version where I used analogWrite.
I also tried to increase the prescaling on the PWM clock which made no difference.

Here's a quick circuit diagram if it helps (fixed silly circuit diagram mistake!)
circuit

Thanks!

You were using functions before they were declared (Arduino doesn't generate prototypes for .c and .cpp files).

You were using the integer 'abs()' function in float calculations. I included "math.h" and switched to fabs().

You were getting warnings that R, G, and B might be used uninitialized because the compiler couldn't be sure that any of the 'case' clauses would be selected. I initialized them to 0.

Now it compiles without warnings. Perhaps one of those warnings was meaningful.

#include <avr/io.h>
#include <util/delay.h>
#include <math.h> // fabs()

#define RED_PIN PB1   // OC1A (9)
#define GREEN_PIN PB2 // OC1B (10)
#define BLUE_PIN PB3  // OC2A (11)
#define INPUT_PIN PC5 // ADC5 (A5)

int analog_read(int pin)
{
  // Select pin in multiplexer and set reference to VCC (ADMUX)
  ADMUX = (1 << REFS0) | (pin & 0x07);

  // Start the analog to digital conversion
  ADCSRA |= (1 << ADSC);

  // Wait for the read to finish. ADSC is kept high while the conversion is in progress
  while (ADCSRA & (1 << ADSC));

  // Get high and low bytes of reading into one integer
  // Must read ADCL first!
  uint8_t low = ADCL;
  uint8_t high = ADCH;
  return (high << 8) | low;
}

// h = [0, 360]; s = [0, 1]; v = [0, 1]
// r = [0, 255]; g = [0, 255]; b = [0, 255]
// See https://www.rapidtables.com/convert/color/hsv-to-rgb.html
void HSVtoRGB(float h, float s, float v, int *r, int *g, int *b)
{
  float C = v * s;
  float X = C * (1 - fabs(fmod(h / 60.0, 2.0) - 1));
  float m = v - C;

  int H = h / 60;
  float R=0, G=0, B=0;

  switch (H)
  {
    case 0:
      R = C;
      G = X;
      B = 0;
      break;

    case 1:
      R = X;
      G = C;
      B = 0;
      break;

    case 2:
      R = 0;
      G = C;
      B = X;
      break;

    case 3:
      R = 0;
      G = X;
      B = C;
      break;

    case 4:
      R = X;
      G = 0;
      B = C;
      break;

    case 5:
      R = C;
      G = 0;
      B = X;
      break;
  }

  *r = (R + m) * 255;
  *g = (G + m) * 255;
  *b = (B + m) * 255;
}

int main(void)
{
  // Set up for ADC
  ADCSRA |= _BV(ADPS2) | _BV(ADPS1) | _BV(ADPS0); // Set prescaler to 128
  ADCSRA |= _BV(ADEN); // Enable analog to digital conversion

  // Set pins for output
  DDRB |= _BV(RED_PIN) | _BV(GREEN_PIN) | _BV(BLUE_PIN);

  // Set up for PWM
  // Set up 8-bit Fast PWM for OC1x outputs
  // See https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061B.pdf#page=141&zoom=auto,-98,455
  TCCR1A |= _BV(WGM10);
  TCCR1B |= _BV(WGM12);
  // No prescaling
  TCCR1B |= _BV(CS10);
  // Output on OC1A and OC1B when counter reaches compare value
  // See https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061B.pdf#page=140&zoom=auto,-98,61
  TCCR1A |= _BV(COM1A1) | _BV(COM1B1);

  // Set up 8-bit Fast PWM for OC2x outputs
  // See https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061B.pdf#page=164&zoom=auto,-98,539
  TCCR2A |= _BV(WGM20) | _BV(WGM21);
  // Output on OC2A when counter reaches compare value
  TCCR2A |= _BV(COM2A1);
  // No prescaling
  TCCR2B |= _BV(CS20);

  // loop forever
  while (1)
  {
    float h = 0, s = 1, v = 0.5;

    int input = analog_read(INPUT_PIN);

    h = input * 360.0 / 1024;

    int r = 0, g = 0, b = 0;

    HSVtoRGB(h, s, v, &r, &g, &b);

    OCR1A = r;
    OCR1B = g;
    OCR2A = b;

    _delay_ms(5);
  }
}


1 Like

Thank you! I think it must've been fabs vs abs :slight_smile: