Simple C code and PIC microcontroller: need it looked at

[axe34]

Homework Statement

I'm using a PIC 16F877A microcontroller. An LDR and moisture sensor connect to the ADC. The main body of the code is ''void main'', and there is also a sub-code called "void light_the_led'' which is read inside the 'void main code'.

I cannot get access easily to hardware or software to test this code below. Does it look ok? Particularly is the ADC code set-up correctly?

Homework Equations

see code below

3. The Attempt at a Solution
see code below. NOTE: this microcontroller gives logic 0 when output is high, and 1 when low!

Thanks. I wish I had access to someone who could try this on a microcontroller (and use an led instead of solenoid).

/*****************************************************************************************************

Description: lights at pin A5 will come on when it is quite dark and/or quite wet. The lights at pin B4 will come on when it is very dark and/or very wet (and the lights at pin A5 will go off). No lights on when it is not dark enough or wet enough.A push type solenoid will active only when either of the lights (pin A5 or B4) come on.

**********************************************************************************************************/#include  <16f877a.h>  // compiler directives

#device  ICD=TRUE

#fuses  HS,NOLVP,NOWDT,PUT

#use delay (clock=20000000)
#define  SIDE_LIGHTS  PIN_A5  // define the pin allocations

#define  FULL_LIGHTS  PIN_B4

#define_SOLENOID PIN_E0#define  cutoff1 128    // quite dark (reading the LDR input)

#define  cutoff2 192    // very dark  (reading the LDR input)

#define cutoff3 128  //quite wet  (reading the moisture input)

#define cutoff4 192  //very wet  (reading the moisture input)
void light_the_led(int led)  // define a function to be read inside main function

{

Output_high(SIDE_LIGHTS);  // turn OFF led

Output_high(FULL_LIGHTS);  //turn led OFF
Switch(LED) 

{

  Case 0 : output_low(SIDE_LIGHTS);  //turn this light on

  Case 1 : output_low(FULL_LIGHTS);  //turn this light on

 

}}

void main( )

{

int reading; 

Setup_adc_ports  (RA1_RA2_Analog);  // set up ADC

Setup_adc  (ADC_CLOCK_INTERNAL);  // Setup ADC

 

while(true)

{

Set_adc_channel (1 ); 

reading = read_adc( 1);  //read LDR value

if (reading > cutoff1)

  light_the_led(0);

  output_low(SOLENOID);  //activates the push type solenoid

else if (reading > cuttoff2)

  light_the_led(1);

  output_low(SOLENOID);  //activates the push type solenoid

else

  output_high(SOLENOID);  //switches off the solenoidSet_adc_channel ( 2 ); 

reading = read_adc(2);  //read moisture sensor value

if (reading > cuttoff3)

  light_the_led(0);

{ Set_adc_channel (1)

if (reading > cuttoff2)

light_the_led(1); }

  output_low(SOLENOID);  //activates the push type solenoid

else if (reading > cuttoff4)

  light_the_led(1);

  output_low(SOLENOID);  //activates the push type solenoid

else

  output_high(SOLENOID);  //switches off the solenoid

  }

}

 

[DEvens]

First, int main not void main. Be standard.

Maybe pasting this into the forum has mangled some things. Maybe it capitalized some symbols that should not be. I seem to remember C being case sensitive. It looks like you are passing in the argument led, but using the variable LED. Also, maybe you could make better use of white space and indents and such. But maybe that got mangled by pasting here.

Think about defining symbols for the turn-on and turn-off values. Possibly you want to make them Booleans if you are confident that you will never get any other state for an LED than on and off.

I always have to look up the behaviour of the switch/case statement. When you get case 0, does it do the code in case 0 then also do the code in case 1? Do you want a break statement in there? What happens if LED (or is it led) is other than 1 or 0?

There are a ton of things here that seem to be referring to the speciality library evoked by #include <16f877a.h>. I have no way to see if any of those is done correctly. All the items about output_high and the references to ADC. Those I have no way to check.

I see the while(true) idiom. This can be useful. But I see no exit from this loop. So this program looks like it will run forever. Is that intended? How do you get out? Usually when the while(true) is used, there is something in the loop that breaks you out of the loop on some pre-defined condition.

 

[cellurl]

code looks good. Its PIC's version of C.

 

[CWatters]

Is there a simulator for the PIC?

 

[berkeman]

(Fixed some of the bolding and added code tags to the OP)...

 

[Mark44]

First, int main not void main. Be standard.

Maybe pasting this into the forum has mangled some things. Maybe it capitalized some symbols that should not be. I seem to remember C being case sensitive. It looks like you are passing in the argument led, but using the variable LED. Also, maybe you could make better use of white space and indents and such. But maybe that got mangled by pasting here.

Think about defining symbols for the turn-on and turn-off values. Possibly you want to make them Booleans if you are confident that you will never get any other state for an LED than on and off.

I always have to look up the behaviour of the switch/case statement. When you get case 0, does it do the code in case 0 then also do the code in case 1? Do you want a break statement in there? What happens if LED (or is it led) is other than 1 or 0?

There are a ton of things here that seem to be referring to the speciality library evoked by #include <16f877a.h>. I have no way to see if any of those is done correctly. All the items about output_high and the references to ADC. Those I have no way to check.

I see the while(true) idiom. This can be useful. But I see no exit from this loop. So this program looks like it will run forever. Is that intended? How do you get out? Usually when the while(true) is used, there is something in the loop that breaks you out of the loop on some pre-defined condition.

In addition to what DEvens said,

• C is case-sensitive. Switch should be switch, and Case should be case.
• With regard to the switch statement, if the switch variable matches a case label, the block of code in the case label executes, and every statement after that block executes, unless there is a break statement.
• Also in the switch statement, LED is not declared. is no variable named LED. There is a parameter named led in your light_the_led function.
• It's difficult to know what your intent is in this code in main():
  
  if (reading > cutoff1)
    light_the_led(0);
    output_low(SOLENOID);  //activates the push type solenoid
  else if (reading > cuttoff2)
    light_the_led(1);
    output_low(SOLENOID);  //activates the push type solenoid
  else
    // etc.
  berkeman added some indentation that you didn't have, but as the code exists now, I don't think it will compile. The indentation suggests that output_low() gets called if reading > cutoff1 or if reading > cutoff2. That's not the case. If you intend the two output_low() calls to be executed as part of the if and else if clauses, you must put braces around each block.