# Blinking LED code at a particular frequency [closed]

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?

• 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. – 200_success Feb 22 '15 at 7:59
• 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. – sam brosteain Feb 22 '15 at 14:45
• 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. – 200_success Feb 22 '15 at 15:37

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
}
}


PORTCbits.RC4=0XFF;  // RC4 on

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