BTW I'm student what is meaning of self documenting?

In this example, comments is not needed here because the line itself is telling that LED is off.

Is this self documenting?

Yeppers, that is exactly what we mean. With such code comments can be directed not to what is being done, but why.

LED = LED_OFF;  // inform user the CPU will now halt and catch fire

 

BTW I'm student what is meaning of self documenting?

LED = LED_OFF;  // Turn off the led

In this example, comments is not needed here because the line itself is telling that LED is off.


Is this self documenting?

Not self-documenting:

unsigned a = 0xdeadbeef;

Self-documenting:

unsigned LED = LED_OFF;

As far as how much is too much with comments just do whatever feels comfortable. That said, better to avoid stating the obvious. Comments should be meaningful.

Good:

/*
A sample program showing why it is
so important to initialize variables
*/
int main(void) {
  int garbage; // Not initialized!
  int data = 1234;
  printf("Data: %d\n", data);
  printf("Garbage: %d\n", garbage);
}

Bad:

/*
program entry point
*/
int main(void) {
/*
declare variables
*/
  int g; // ?
  int d = 1234;// ok
  printf("d: %d\n", d); // print d
  printf("g: %d\n", g); // print g
}

 

I like what Ernie said.
Don’t state what the statement does.
State why you are doing this.

 

I like what Ernie said.
Don’t state what the statement does.
State why you are doing this.

It is very important for me to document code very well

If I don't document my code properly then it's big trouble for someone who is helping me on the forum. If the code is well documented, anyone can understand code easily and they can help me quickly to fix my problem.

I just want to see how you guys document the code and write meaningful comments.I want this from you because you have many years of experience. I can learn a lot from your technique and style. I just want to learn your techniques so that I can save myself from big trouble.

Maybe it will be a great help to me if someone can documents following source code with meaningful comments

I'll do some code documents myself after seeing your style

#define _XTAL_FREQ   20000000

#define LCD_E                   RB0
#define LCD_RS                  RB1
#define LCD_Data_Bus_D4         RB4
#define LCD_Data_Bus_D5         RB5
#define LCD_Data_Bus_D6         RB6
#define LCD_Data_Bus_D7         RB7


#define LCD_E_Dir               TRISB0
#define LCD_RS_Dir              TRISB1
#define LCD_Data_Bus_Dir_D4     TRISB4
#define LCD_Data_Bus_Dir_D5     TRISB5
#define LCD_Data_Bus_Dir_D6     TRISB6
#define LCD_Data_Bus_Dir_D7     TRISB7


#define E_Delay       500

void WriteCommandToLCD(unsigned char);
void WriteDataToLCD(char);
void InitLCD(void);
void WriteStringToLCD(const char*);
void ClearLCDScreen(void);

int main(void)
{
    CMCON = 0x07;
    InitLCD();
    int siLoop;
    int msgLen = 0;
    const char *msg ="Hello World!";

    msgLen = strlen(msg);
    while(1)
    {
        WriteCommandToLCD(0x8f);
        WriteStringToLCD(msg);
        for(siLoop=0; siLoop < msgLen; siLoop++)
        {
            WriteCommandToLCD(0x1c);
            __delay_us(100000); 
        }
    }

    return 0;
}
void ToggleEpinOfLCD(void)
{
    LCD_E = 1;           
    __delay_us(E_Delay); 
    LCD_E = 0;           
    __delay_us(E_Delay);
}
void WriteCommandToLCD(unsigned char Command)
{
    LCD_RS = 0;     
    PORTB &= 0x0F;   
    PORTB |= (Command&0xF0);
    ToggleEpinOfLCD(); 
    PORTB &= 0x0F;   
    PORTB |= ((Command<<4)&0xF0);
    ToggleEpinOfLCD(); 
}
void WriteDataToLCD(char LCDChar)
{
    LCD_RS = 1;     
    PORTB &= 0x0F;   
    PORTB |= (LCDChar&0xF0);
    ToggleEpinOfLCD(); 
    PORTB &= 0x0F;   
    PORTB |= ((LCDChar<<4)&0xF0);
    ToggleEpinOfLCD(); 
}
void InitLCD(void)
{

    LCD_E                = 0;
    LCD_RS               = 0;
    LCD_Data_Bus_D4      = 0;
    LCD_Data_Bus_D5      = 0;
    LCD_Data_Bus_D6      = 0;
    LCD_Data_Bus_D7      = 0;
    LCD_E_Dir            = 0;
    LCD_RS_Dir           = 0;
    LCD_Data_Bus_Dir_D4  = 0;
    LCD_Data_Bus_Dir_D5  = 0;
    LCD_Data_Bus_Dir_D6  = 0;
    LCD_Data_Bus_Dir_D7  = 0;

    __delay_ms(40);
    PORTB &= 0x0F;   
    PORTB |= 0x30;   
    ToggleEpinOfLCD(); 
    __delay_ms(6);
    PORTB &= 0x0F;   
    PORTB |= 0x30;   
    ToggleEpinOfLCD(); 
    __delay_us(300);
    PORTB &= 0x0F;   
    PORTB |= 0x30;   
    ToggleEpinOfLCD(); 
    __delay_ms(2);
    PORTB &= 0x0F;   
    PORTB |= 0x20;   
    ToggleEpinOfLCD(); 
    __delay_ms(2);

    WriteCommandToLCD(0x28);
    WriteCommandToLCD(0x0c);
    WriteCommandToLCD(0x01);
    WriteCommandToLCD(0x06);
}
void WriteStringToLCD(const char *s)
{
    while(*s)
    {
        WriteDataToLCD(*s++);
    }
}
void ClearLCDScreen(void) 
{
    WriteCommandToLCD(0x01);
    __delay_ms(2);         
}

Edit LCD Discription
https://en.m.wikipedia.org/wiki/Hitachi_HD44780_LCD_controller

 

I'd start by getting rid of all those magic numbers and define what those numbers that you send actually are (i.e. their command name in the lcds manual)

    WriteCommandToLCD(0x28); 
    WriteCommandToLCD(0x0c); 
    WriteCommandToLCD(0x01); 
    WriteCommandToLCD(0x06);

I personally don't like shortened names - Just spell out the whole thing

msgLen 
to...
messageLength

What is "siLoop"? When it comes to loops where its a single variable like that, it's generally accepted (in just about every language) to use "i" as the variable name. I'd only give it a name if it had functionallity and naming it something else added value.

At the top of the file, make a comment block that says what PIC you are using, what hardware you are using (i.e. LCD part, digikey numbers), date, list any known bugs (TODO list, date reported, who fixed it and when)

Most companies will get you to use things like doxygen, so that they can easily export documentation. That might be worth looking into

As a side note, you would usually write to the LAT resister and read from the PORT - It avoids read/modify/write bugs

 

Also with the XC8 compiler, you can define your own "putch" - This allows you to use functions like puts and printf

 

Example of a useful comment...

CMCON = 0x07;  // Turn off comparators (see datasheet 10.1)

 

1) Use #define to declare your mystery commands as pointed out above. For example,
#define CLEAR_SCREEN 0x01

2) CMCON is not defined or used. The compiler will flag as error.

3) Lines 40-43 are not necessary.

4) Add white space around operators, Line #63, 73.

5) Add blank line before new function.

7) For complex functions, add comments and specifications at the top of the function.

8) #define your delay times
#define CMD_DELAY 2

9) Use all upper case for constant definitions.

 

>CMCON is not defined or used. The compiler will flag as error

I believe that it is the comparator configuration register for a PIC microprocessor.

It would be defined when including xc.h (or equivalent) - Although I didn't see it in the code, I'm assuming that the TS just left out their includes when posting.

(They'd also need string.h for strlen)

 

I usually add extra comments for things that are tricky or not obvious from looking at the code like the NVRAM storage page variable. Too many comments are distracting.

/* 
 * File:   magic.h
 * Author: root
 *
 * Created on October 31, 2020, 7:38 AM
 */

#ifndef MAGIC_H
#define    MAGIC_H

#ifdef    __cplusplus
extern "C" {
#endif

    static const double rps = 0.0174532925f; // degrees per second -> radians per second
    static const uint8_t CAL_DIS_MS = 1; // calibration data element screen display time
    static const char *build_date = __DATE__, *build_time = __TIME__;
    static const char imu_missing[] = " MISSING \r\n";
    /*
     * NVRAM storage page variable
     * const volatile here means it's stored in FLASH for read-only program data access in normal program flow but
     * also set to volatile because the FLASH write controller can modify the contents out of normal program flow
     * so we need the compiler TO NOT optimize out reads from the actual memory locations because of read-only data caching
     * optimizations
     */
#include "nvram_page.h"
    const volatile uint32_t myflash[4096] __attribute__((section("myflash"), space(prog), address(NVM_STARTVADDRESS)));

    const uint32_t update_delay = 5; // ms delay to make a total of 10ms between IMU updates

#ifdef    __cplusplus
}
#endif

#endif    /* MAGIC_H */

 

...When it comes to loops where its a single variable like that, it's generally accepted (in just about every language) to use "i" as the variable name. I'd only give it a name if it had functionallity and naming it something else added value.

I agree with this, but to be overly pedantic I will oft spell it out, using "Index" as the name of my indexing value. It is just a bit clearer to me when visually scanning code.

Also, use the compiler as your friend whenever possible. My example comes from VB (which we still use at my job due to the existing code base). I *always* turn on "Option Explicit" which informs the compiler *not* to recognize any variable that is not defined as VB will assume you meant a variant otherwise.

What you gain is identifying typos! Imagine you use a variable "FaultDetect" but typo the test line as "FaltDetect". Best to catch that at compile time instead of a weeks worth of debugging later.

 

1) Use #define to declare your mystery commands as pointed out above. For example,
#define CLEAR_SCREEN 0x01

Let's focus on comments for code. Just ignore rest of other things for now. I was hoping that someone would write comments for source code in post #24 to show your techniques. As you all have advised I was curious to see how would you write comments

Don’t state what the statement does.
State why you are doing this.

This is my attempt and I would like to see your attempt

#define _XTAL_FREQ   20000000          //  Define processor Frequency

#define LCD_E                   RB0    // Enable pin for LCD
#define LCD_RS                  RB1    // Register Select pin for LCD
#define LCD_Data_Bus_D4         RB4    // Data Pin 4
#define LCD_Data_Bus_D5         RB5    // Data Pin 5
#define LCD_Data_Bus_D6         RB6    // Data Pin 6
#define LCD_Data_Bus_D7         RB7    // Data Pin 7


#define LCD_E_Dir               TRISB0   //
#define LCD_RS_Dir              TRISB1
#define LCD_Data_Bus_Dir_D4     TRISB4
#define LCD_Data_Bus_Dir_D5     TRISB5
#define LCD_Data_Bus_Dir_D6     TRISB6
#define LCD_Data_Bus_Dir_D7     TRISB7

#define FUNCTION_SET                0x28
#define CLEAR_DISPLAY               0x01
#define DISPLAY_ON_CURSOR_OFF       0x0c
#define  ENTRY_MODE_SET_INCREMENT   0x06


#define E_Delay       500


// Function Declarations
void WriteCommandToLCD(unsigned char);
void WriteDataToLCD(char);
void InitLCD(void);
void WriteStringToLCD(const char*);
void ClearLCDScreen(void);

int main(void)
{
    CMCON = 0x07;  //Turn off comparator
    InitLCD();     // Initialize LCD in 4bit mode
    int siLoop;
    int msgLen = 0;
    const char *msg ="Hello World!";
   
    msgLen = strlen(msg);
    while(1)
    {
        WriteCommandToLCD(0x8f);
        WriteStringToLCD(msg);
        for(siLoop=0; siLoop < msgLen; siLoop++)
        {
            WriteCommandToLCD(0x1c);
            __delay_us(100000);    
        }
    }
   
    return 0;
}
void ToggleEpinOfLCD(void)
{
    LCD_E = 1;              
    __delay_us(E_Delay);    
    LCD_E = 0;              
    __delay_us(E_Delay);
}
void WriteCommandToLCD(unsigned char Command)
{
    LCD_RS = 0;        
    PORTB &= 0x0F;      
    PORTB |= (Command&0xF0);  //Write Upper nibble of data
    ToggleEpinOfLCD();    
    PORTB &= 0x0F;      
    PORTB |= ((Command<<4)&0xF0); // Write Lower nibble of data
    ToggleEpinOfLCD();    
}
void WriteDataToLCD(char LCDChar)
{
    LCD_RS = 1;        
    PORTB &= 0x0F;      
    PORTB |= (LCDChar&0xF0);     //Write Upper nibble of data
    ToggleEpinOfLCD();    
    PORTB &= 0x0F;      
    PORTB |= ((LCDChar<<4)&0xF0); // Write Lower nibble of data
    ToggleEpinOfLCD();    
}
void InitLCD(void)
{
 
    LCD_E                = 0;
    LCD_RS               = 0;
    LCD_Data_Bus_D4      = 0;  
    LCD_Data_Bus_D5      = 0;  
    LCD_Data_Bus_D6      = 0;  
    LCD_Data_Bus_D7      = 0;  
    LCD_E_Dir            = 0;
    LCD_RS_Dir           = 0;  
    LCD_Data_Bus_Dir_D4  = 0;  
    LCD_Data_Bus_Dir_D5  = 0;
    LCD_Data_Bus_Dir_D6  = 0;  
    LCD_Data_Bus_Dir_D7  = 0;  
   
    __delay_ms(40);
    PORTB &= 0x0F;      
    PORTB |= 0x30;      
    ToggleEpinOfLCD();    
    __delay_ms(6);
    PORTB &= 0x0F;      
    PORTB |= 0x30;      
    ToggleEpinOfLCD();    
    __delay_us(300);
    PORTB &= 0x0F;      
    PORTB |= 0x30;      
    ToggleEpinOfLCD();    
    __delay_ms(2);
    PORTB &= 0x0F;      
    PORTB |= 0x20;      
    ToggleEpinOfLCD();    
    __delay_ms(2);
   
    WriteCommandToLCD(FUNCTION_SET);  
    WriteCommandToLCD(DISPLAY_ON_CURSOR_OFF);  
    WriteCommandToLCD(CLEAR_DISPLAY);  
    WriteCommandToLCD(ENTRY_MODE_SET_INCREMENT);  
}
void WriteStringToLCD(const char *s)
{
    while(*s)
    {
        WriteDataToLCD(*s++);  
    }
}
void ClearLCDScreen(void)    
{
    WriteCommandToLCD(0x01);  
    __delay_ms(2);            
}

 

Sorry, I cannot do that.

Code spacing, indentation, variable styles and definitions are all part and parcel of making code readable.
If you choose your labels properly there is less need to comment each line of code.

As I pointed out, comments go at the start of the declaration of the function. You describe the purpose of the function, input and output parameters and limitations when they exist. The code itself ought to be self explanatory unless you are doing something tricky.