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\$

closed as unclear what you're asking by 200_success Feb 22 '15 at 7:59

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\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.

\$\endgroup\$

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