Noob project needs some critique. EagleCADsoft file attached

Thread Starter

jameschristian

Joined Nov 24, 2011
58
Rich (BB code):
#include <pic.h>

void delay_sec(int sec)

void main()
{
   TRIS A==0;      //sets output to fan
   PORTB==0;       //CLEARS PORT B
   while(1)            
   {
          if (TRISB0==1)
         {
          TRISA==1;
          delay_sec(60);     //ONE MINUTE ON
          }
          else if (TRISB1==1)
          {
           TRISA==1;
           delay_sec(120);    //2 MINUTES ON
          }
           else if (TRISB2==1)
          {
          TRISA==1;
          delay_sec(300);    //5 MINUTES ON
          }
          else if (TRISB3==1)
          {
          TRISA==1;
          delay_sec(600);    //10 MINUTES ON
          }
          else if (TRISB4==1)
          {
          TRISA==1;
          delay_sec(1200);   //20 MINUTES ON
          }
          else if (TRISB5==1)
          {
          TRISA==1;
          delay_sec(1800);   //30 MINUTES
          }
          else if (TRISB6==1)
          {
         TRISA==1;
          delay_sec(3600);   //ONE HOUR ON
          }
          else if (TRISB7==1)
          {
          TRISA==1;
          delay_sec(7200);   //2 HOURS ON
          } 
     }  
}
return ();

void delay_sec(int secs)
{
   for(int ms=0;ms<secs;ms++)
   {
      delay_ms(1000);      // delay 1 second
   }
}
return ();
Am wondering about the two return calls at the end. Necessary or no?

Per the crystals' size, the footprint is 1.05 cm total crystal length, leads are 1.3 cm in length (from bottom of crystal) and .5 cm apart. Crystal itself is .3 cm in depth and .4 cm wide with a .5 mm lip around the bottom. I will draw it out if need be later this afternoon, gotta run some errands and work tonight so it may be tonight before I get it posted.

Nerdegutta: I love the redesign. Very clean. I am going to make a prototype or two and then, who knows? If I can sell the thing I may need a bunch of them. I had asked earlier about finding someone to make them, and the Sarge reco'd someone but I think they just produce the PCB's. I'd like to find someone to do assembly as well; someone I can send a spec sheet on the enclosure. There has to be someone out there who does that, and I would like to use a local company if possible. Lord knows we need the jobs here in the U.S..
 
Last edited:

nerdegutta

Joined Dec 15, 2009
2,684
I think you need some config word settings in your code. And some definitions. When everything is done, and it's been a few months, do you remember the settings?

Actually, I did something similar to this a while back. Here's my definitions and configurations:
Rich (BB code):
#include  <htc.h>
#define _XTAL_FREQ 4000000

/* Configuration */
__CONFIG    (WDTDIS &
PWRTEN &
MCLREN &
BOREN &
LVPDIS &
DATUNPROT &
UNPROTECT &
XT);
I used the PIC16F628.
 

MrChips

Joined Oct 2, 2009
30,708
The return( ) statements should appear before the previous curly brace but in your case since they return nothing you can just delete them.
 

Thread Starter

jameschristian

Joined Nov 24, 2011
58
Sarge, I really like your version too. I will mull it over and decide over the next few days which to go with, and may adjust the design a little myself. My next step is finalizing the program, and am pretty tired tonight so will be working on it tomorrow and posting it for your critique. I finally got my programmer software installed, so I can start to test the project once I get a voltage regulator of the right specification. Did you look at the specs for the crystal? I have seen a few over the years, and the one I have seems to be a pretty common one in my limited experience. The footprint of the one on the boards looks a bit different than the one I have, but I can always buy a couple of new ones if need be. Well, off to bed. Good night gentlemen.
 

SgtWookie

Joined Jul 17, 2007
22,230
Mulling is good, you'll always think of something you forgot.

Like, there is no place to get other information in, or out of the PIC. :)

I went ahead and added pads for RA0 through RA4, and a GND pad too.

Pushed things around a bit to make the ground plane more contiguous, and improve the aesthetics somewhat:

 

Attachments

Last edited:

Thread Starter

jameschristian

Joined Nov 24, 2011
58
I have just learned about ICSP programming, but was planning on programming off board and then soldering everything together once I have tested it on a breadboard to make sure it all works as it should. Great thought on the pads, Sarge, I may have missed that and it wouldn't have been pretty.


Rich (BB code):
#include <pic.h>
#include <delay.h>

void delay_sec(int sec)

void main()
{
   TRISA==0;      //sets output to fan
   PORTB==0;       //CLEARS PORT B
   while(1)            
   {
          if (TRISB0==1)
         {
          TRISA==1;
          delay_sec(60);     //ONE MINUTE ON
          }
          else if (TRISB1==1)
          {
           TRISA==1;
           delay_sec(120);    //2 MINUTES ON
          }
           else if (TRISB2==1)
          {
          TRISA==1;
          delay_sec(300);    //5 MINUTES ON
          }
          else if (TRISB3==1)
          {
          TRISA==1;
          delay_sec(600);    //10 MINUTES ON
          }
          else if (TRISB4==1)
          {
          TRISA==1;
          delay_sec(1200);   //20 MINUTES ON
          }
          else if (TRISB5==1)
          {
          TRISA==1;
          delay_sec(1800);   //30 MINUTES
          }
          else if (TRISB6==1)
          {
         TRISA==1;
          delay_sec(3600);   //ONE HOUR ON
          }
          else if (TRISB7==1)
          {
          TRISA==1;
          delay_sec(7200);   //2 HOURS ON
          } 
     }  
}


void delay_sec(int secs)
{
   for(int ms=0;ms<secs;ms++)
   {
      __delay_ms(1000);      // delay 1 second
   }
}
Nerdegutta: this is from the datasheet, and is the only mention of configurations from it:

REGISTER 6-1: PIC16F84A CONFIGURATION WORD
R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u R/P-u
CP CP CP CP CP CP CP CP CP CP PWRTE WDTE F0SC1 F0SC0
bit13 bit0


bit 13-4 CP: Code Protection bit
1 = Code protection disabled
0 = All program memory is code protected


bit 3 PWRTE: Power-up Timer Enable bit
1 = Power-up Timer is disabled
0 = Power-up Timer is enabled


bit 2 WDTE: Watchdog Timer Enable bit
1 = WDT enabled
0 = WDT disabled


bit 1-0 FOSC1:FOSC0: Oscillator Selection bits
11 = RC oscillator
10 = HS oscillator
01 = XT oscillator
00 = LP oscillator



I put spaces so the configuration words are more apparent, but there only appear to be four, unless I am just missing out on the notion of what configurations are for and if they are in fact more applicable across multiple chips. Here's a link to the datasheet:

http://ww1.microchip.com/downloads/en/devicedoc/35007b.pdf

Do you see something I don't? I know the μC is pretty old, and am actually thinking of going with a p16f88 now that I have learned that they are the same cost and much more functional. I don't want to complicate things, though, so I may just stick with the p16f84 if it's not something that would make a big difference and just go with the p16f88 for my next project. I hope you guys are having a great weekend, and thanks again for the time and thoughts.

Note: Edited per Nerdegutta's next post.
 
Last edited:

nerdegutta

Joined Dec 15, 2009
2,684
A few comments:

Rich (BB code):
void main()
{
   TRIS A==0;      //sets output to fan
   PORTB==0;       //CLEARS PORT B
   while(1)            
   {
TRIS A, should be TRISA

And the two
Rich (BB code):
return ();
are outside the {}, so they have no function.

And the
Rich (BB code):
delay_ms(1000)
Should be
Rich (BB code):
__delay_ms(1000)
For now...

Which IDE and compiler are you using?
 

Thread Starter

jameschristian

Joined Nov 24, 2011
58
I've got MPLAB's IDE and had planned on using it to compile as well. I also downloaded Visual C++ 2010 Express from Microsoft, but am not sure whether I can use it for the C program or not.

p.s. I am new to the idea of configurations words, so do I just include the four from the datasheet?
 

nerdegutta

Joined Dec 15, 2009
2,684
I would strongly recommend you to use Hi-tech C with MPLAB. Some might want to tell you otherwise...

My
Rich (BB code):
__delay_ms(1000)
might not work on your setup.

I did something similar, lighting some LEDs. My source code is below:

Rich (BB code):
/* 
 
Program:        main.c 
Description:    Timer program for UV-box 
PIC:            16F628 
IDE:            MPLAB 
Compiler:        Hi - Tech C 
Date:            Nov 2010 
Author:            nerdegutta
web:            www.nerdegutta.org 
 
*/ 
 
#include  <htc.h>
#define _XTAL_FREQ 4000000 
 
/* Configuration */ 
 
__CONFIG    (WDTDIS & 
 PWRTEN & 
 MCLREN & 
 BOREN & 
 LVPDIS & 
 DATUNPROT & 
 UNPROTECT & 
 XT); 
 
/* Prototyping the functions */ 
void lightLED(); 
void beep(); 
 
/* Global variables */ 
unsigned char i; 
 
/* Functions */ 
void lightLED() 
{ 
 PORTB = 0b00000011; // Setting bit 0-1 to HIGH 
 __delay_ms(1000); 
 
 PORTB = 0b00000000; // Setting all bits to LOW 
} // end lightLED 
 
 
void beep() 
{ 
 for (i=0;i<125;i++) 
 { 
 PORTB = 0b00000100; // Setting bit 1 to HIGH 
 __delay_ms(1); 
 
 PORTB = 0b00000000; // Setting bit 1 to LOW 
 __delay_ms(1); 
 
 } // end for-loop 
} // end beep 
 
/* Main program */ 
void main() 
{ 
TRISA = 0b11111111;    // Setting all bits on port a to input 
TRISB = 0b00000000;    // Setting all bits on port b to output 
 
PORTA = 0b00000000; // Setting all bits on port a to LOW 
PORTB = 0b00000000;    // Setting all bits on port b to LOW 
 
CMCON = 0x07;    // Disabling the analogue comparators 
 
while (1) 
{ 
 if (RA0) 
 { 
 
 for(i=0;i<2;i++) // Create a delay for 2 seconds 
 { 
 lightLED(); 
 } // end for-loop 
 beep(); 
 } // end if 
 
 if (RA1) 
 { 
 
 for(i=0;i<120;i++) 
 { 
 lightLED(); 
 } // end for-loop 
 beep(); 
 } // end if 
 
 if (RA2) 
 { 
 
 for(i=0;i<240;i++) 
 { 
 lightLED(); 
 } // end for-loop 
 beep(); 
 } // end if 
 
 if (RA3) 
 { 
 
 for(i=0;i<240;i++) 
 { 
 lightLED(); 
 } // end for-loop 
 for (i=0;i<120;i++) 
 { 
 lightLED(); 
 } 
 beep(); 
 } // end if 
 
 if (RA4) 
 { 
 
 for(i=0;i<240;i++) 
 { 
 lightLED(); 
 } // end for-loop 
 for(i=0;i<240;i++) 
 { 
 lightLED(); 
 } 
 beep(); 
 } // end if 
 if (RB7) 
 { 
 for (i=0;i<60;i++) 
 { 
 lightLED(); 
 } 
 beep(); 
 } 
 
} // end while 
 
}  //end main
 
Last edited:

Thread Starter

jameschristian

Joined Nov 24, 2011
58
Rich (BB code):
#include <pic.h>
#include <delay.h>
#define _XTAL_FREQ 4000000
void delay_sec(int sec)

void main()
{
   TRISA=0;      //sets pin connected to fan as output
   TRISB=1;     //sets pins connected to rotary switch to input

     delay_sec (int sec)    // declaration of function




     for (int x, x<4, x++);   // fan will pulse four times for chosen time length
          {
               while (x<=4);
               {
                    if (input(pin_B0));        //option 1 of switch, labeled '1 minute'
                     {
                     output_high(pin_A0);         // set pin 1, 'A0', high (powers fan)
                     delay_sec(60);          // on for one minute
                     }
                    else if (input(pin_B1));  //option 2 of switch, labeled '2 minutes'
                     {
                     output_high(pin_A0);           // same as above reference
                     delay_sec(120);         // on for two minutes
                      }
                    else if (input(pin_B2));   //option 3 of switch, labeled '5 minutes'
                     {
                      output_high(pin_A0);          //same as above
                      delay_sec(300);
                     } 
                     else if (input(pin_B3));  //option 4 of switch, labeled 'stay on'
                     {
                      output_high(pin_A0); //this time it's on until the switch is turned off
                     }
                      delay_sec(300);   // time between pulses
           return()
          }
   return()
 }


void delay_sec(int secs)  // function to enable use of seconds via 'delay_ms'
{
   for(int ms=0;ms<secs;ms++)
   {
      delay_ms(1000);      // delay 1 second
   }
}

Man, that felt good to write. there are some differences in the construct of the program in that there are only 4 options from which the user may choose, and the pulsing of the an four times when in use (except when the 'constant on' is chosen). I had a discussion with a business partner and we decided those changes would be good. I don't want to celebrate too early, but it feels really close to being right. Thanks to the guys and gals at MIT (see: http://fab.cba.mit.edu/content/tools/microcontrollers/c_for_microcontrollers.html) and Swarthmore (see: http://www.swarthmore.edu/NatSci/echeeve1/Ref/C for PIC/C_Intro.html) for their informative tutorials on PIC programming, and to the guys and gals here and at Cprogramming.com for the help. I know there are some tweaks that still need to be done both to the design and to the program, and your thoughts would be appreciated. I hope y'all are having a good weekend. :)


Edit: Whoops! Good eye Nerdegutta, that was a copy and paste error. Thanks :)

Any more thoughts on
 
Last edited:

nerdegutta

Joined Dec 15, 2009
2,684
Is it right to have two

Rich (BB code):
void main ()
in a program?

Rich (BB code):
output_high
is not Hi-Tech C syntax.
 
Last edited:

MrChips

Joined Oct 2, 2009
30,708
Two equal signs == is an equality test ( is equal to?).
The correct statement should be
TRISA = 0;
 
Last edited:

Thread Starter

jameschristian

Joined Nov 24, 2011
58
i corrected the code to reflect your input. thank you for that. i am also wondering about the inclusion of a four way rotary switch instead of a twelve way as is shown currently in the schematic. is there such a thing? if so, what's it called? I could even do with a three way switch, and it really doesn't have to be a rotary switch at that. just something to toggle the timing for the fan. has anybody looked over the new code? does it look pretty much finished? i know nerdegutta had mentioned the inclusion of some configuration code but i hadn't heard back about the necessity of that. is it compulsory to the program? and do the four configuration code words i mentioned act as relevant code for this program? i feel like i am getting close to being finished here, but want to make sure all my i's are dotted and my t's are crossed before i start testing. any thoughts would be great. thanks again for all your help so far.
 

SgtWookie

Joined Jul 17, 2007
22,230
Well, I added an ICSP header strip to the board - just used a 5-pin header. Also increased the spacing between the traces a tad.

Have you considered just using two buttons - "faster" and "slower"? And perhaps using Charlieplexed LEDs to indicate where the setting is as far as faster or slower?



BTW, the grid in the board is set to 12.5 mils - change it to 25 mils or 50 mils if you're going to move things or add things.
 

Attachments

Top