0
\$\begingroup\$

I wrote a function for interacting with my PIC16F1788. The code sends pulses out the RC4 port at a specific frequency, to make an LED blink according to that frequency. So the LED will blink faster or slower depending on the magnitude of the frequency. I decided to take a blinking code and vary the delay time according to my input frequency as follows.

void blink(int frequency){
/* starts the LED blinking at a particular frequency */

while(1)
{ 
 PORTCbits.RC4=0XFF;  // RC4 on
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
 PORTcbits.RC4=0X00;   // RC4 off
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
}



}

would this code work, is this a good idea ,and is there anyway that I could improve it?

\$\endgroup\$
  • \$\begingroup\$ I find this an odd question. It resembles the sample code on page 7 of this tutorial, except you've changed some stuff. Yet you haven't included the relevant struct definitions to make that change meaningful. Furthermore, your inconsistent capitalization is suspicious — probably wrong — so I'm putting this question on hold as probably broken code. Please add some relevant detail to make it a fully working program. \$\endgroup\$ – 200_success Feb 22 '15 at 7:59
  • \$\begingroup\$ what I am trying to do is to blink an LED at pin RC4 depending on the input frequency of a signal input. so as the signal frequency changes the LED's will blink faster or slower. This is just a beginners code as I am trying to learn how to code PIC's thanks. \$\endgroup\$ – sam brosteain Feb 22 '15 at 14:45
  • \$\begingroup\$ What you wrote is an infinite loop, so the frequency will never get a chance to change. Code Review is the place to review working, almost-finished code. If you need help getting something working, Stack Overflow or Electrical Engineering would be the place to ask. \$\endgroup\$ – 200_success Feb 22 '15 at 15:37
4
\$\begingroup\$

Your code:

void fault_blink(int frequency){
/* starts the LED blinking at a particular frequency */

while(1)
{ 
 PORTCbits.RC4=0XFF;  // RC4 on
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
 PORTcbits.RC4=0X00;   // RC4 off
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
}
}

Indentation

The body of the fault_blink function should be indented, along with the body of the while loop.

The code now:

void fault_blink(int frequency){
/* starts the LED blinking at a particular frequency */

    while(1)
    { 
         PORTCbits.RC4=0XFF;  // RC4 on
         __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
         PORTcbits.RC4=0X00;   // RC4 off
         __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
    }
}

Spacing

To enhance readability, there should be spaces around arithmetic operators and = signs.

The code now:

void fault_blink(int frequency) {
/* starts the LED blinking at a particular frequency */

    while(1)
    { 
         PORTCbits.RC4 = 0XFF;  // RC4 on
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
         PORTcbits.RC4 = 0X00;   // RC4 off
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
    }
}

0X vs 0x

This isn't the biggest thing, but most people have a lower-case 'x' when defining a number as a hexadecimal number.

The code now:

void fault_blink(int frequency) {
/* starts the LED blinking at a particular frequency */

    while(1)
    { 
         PORTCbits.RC4 = 0xFF;  // RC4 on
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
         PORTcbits.RC4 = 0x00;   // RC4 off
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
    }
}

Unnecessary comments

Don't repeat your comments; especially when the original comment is very near.

In this part of your code:

PORTCbits.RC4=0XFF;  // RC4 on
__delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
PORTcbits.RC4=0X00;   // RC4 off
__delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz

You can remove the comment on the second __delay_ms call as the comment is identical to the call right above it.

| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.