I am not a professional coder, I am passionate about coding and aspire to write code that aligns with professional standards.

I have developed a basic LED blinking example for the PIC18F45K22 microcontroller on my board, using XC8 2.36 compiler and MPLAB X 6.00. The code aims to blink four LEDs (Red, Green, Yellow, and Blue) in a loop with a 1-second interval. The LEDs are connected to PORTD pins RD0 (RED), RD1 (GREEN), RD2 (YELLOW), and RD3 (BLUE). Code work fine on my own development board

I would greatly appreciate it if you could take some time to review the code and provide feedback. I am particularly interested in improving the clarity, efficiency, and adherence to best practices in coding. Your insights and suggestions would be immensely valuable in enhancing my coding skills.

Here is a brief summary of the code:

/*
* Summary : Basic LED Blinking Example
*
* Device: PIC18F45K22
* Compiler: XC8 2.36
* MPLAB: MPLAB X 6.00
* Language: C
*
* File: main.c
* Author: NAME
*
* Created on 9 November, 2023, 6:35 PM
* Description: This is a simple example code for the PIC18F45K22 microcontroller
*              that blinks four LEDs (Red, Green, Yellow, and Blue)  every 1 seconds in a loop.
*              The LEDs are connected to PORTD pins RD0 (RED), RD1 (GREEN),
*              RD2 (YELLOW), and RD3 (BLUE).
*/

#include <xc.h>
#include "device_config.h"

// Define constants for clarity
#define ON      1
#define OFF     0
#define true    1

// Define LED pins
#define RED_PIN     LATDbits.LATD0
#define GREEN_PIN   LATDbits.LATD1
#define YELLOW_PIN  LATDbits.LATD2
#define BLUE_PIN    LATDbits.LATD3

// Function prototypes
void OSCILLATOR_Initialize(void);
void PORT_Initialize(void);
void SYSTEM_Initialize(void);

/*
* Function: Initializes the oscillator settings for the PIC18F45K22.
* Returns: None
* Parameters: None
*/
void OSCILLATOR_Initialize(void)
{
    // Configure oscillator settings for the PIC18F45K22 16 MHz speed
    // SCS FOSC; HFIOFS not stable; IDLEN disabled; IRCF 8MHz_HF;
    OSCCON = 0x60;
    // SOSCGO disabled; MFIOSEL disabled; SOSCDRV Low Power;
    OSCCON2 = 0x00;
    // INTSRC INTRC; PLLEN disabled; TUN 0;
    OSCTUNE = 0x00;
}

/*
* Function: Initializes the ports (LATx, TRISx, ANSELx) for the PIC18F45K22.
* Returns: None
* Parameters: None
*/
void PORT_Initialize(void)
{
    // Initialize LATx registers to ensure all outputs are low
    LATA = LATB = LATC = LATD = LATE = 0x00;

    // Set all TRISx registers to configure all pins as outputs
    TRISA = 0x00;
    TRISB = 0x00;
    TRISC = 0x00;
    TRISD = 0x00;
    TRISE = 0x00;

    // Configure ANSELx registers to make all pins digital
    ANSELA = 0x00;
    ANSELB = 0x00;
    ANSELC = 0x00;
    ANSELD = 0x00;
    ANSELE = 0x00;
}

/*
* Function: Initializes the entire system by calling the necessary initialization functions.
* Returns: None
* Parameters: None
*/
void SYSTEM_Initialize(void)
{
    // Initialize the system by configuring ports and oscillator
    PORT_Initialize();
    OSCILLATOR_Initialize();
}

/*
* Function: The main function that initializes the system and blinks the LEDs in a loop.
* Returns: None
* Parameters: None
*/
void main(void)
{

    // Initialize the system components
    SYSTEM_Initialize();

    // Initialize all LEDs to the OFF state at start
    RED_PIN = GREEN_PIN = YELLOW_PIN = BLUE_PIN = OFF;

    // Main loop
    while (true)
    {

        // Turn on all LEDs
        RED_PIN = GREEN_PIN = YELLOW_PIN = BLUE_PIN = ON;
        __delay_ms(1000);

        // Turn off all LEDs
        RED_PIN = GREEN_PIN = YELLOW_PIN = BLUE_PIN = OFF;
        __delay_ms(1000);
    }
}

device_config.c source File

// CONFIG1H
#pragma config FOSC = INTIO7    // Oscillator Selection bits (Internal oscillator block, CLKOUT function on OSC2)
#pragma config PLLCFG = OFF     // 4X PLL Enable (Oscillator used directly)
#pragma config PRICLKEN = OFF   // Primary clock enable bit (Primary clock can be disabled by software)
#pragma config FCMEN = OFF      // Fail-Safe Clock Monitor Enable bit (Fail-Safe Clock Monitor disabled)
#pragma config IESO = OFF       // Internal/External Oscillator Switchover bit (Oscillator Switchover mode disabled)

// CONFIG2L
#pragma config PWRTEN = OFF     // Power-up Timer Enable bit (Power up timer disabled)
#pragma config BOREN = SBORDIS  // Brown-out Reset Enable bits (Brown-out Reset enabled in hardware only (SBOREN is disabled))
#pragma config BORV = 190       // Brown Out Reset Voltage bits (VBOR set to 1.90 V nominal)

// CONFIG2H
#pragma config WDTEN = OFF      // Watchdog Timer Enable bits (Watch dog timer is always disabled. SWDTEN has no effect.)
#pragma config WDTPS = 32768    // Watchdog Timer Postscale Select bits (1:32768)

// CONFIG3H
#pragma config CCP2MX = PORTC1  // CCP2 MUX bit (CCP2 input/output is multiplexed with RC1)
#pragma config PBADEN = OFF     // PORTB A/D Enable bit (PORTB<5:0> pins are configured as digital I/O on Reset)
#pragma config CCP3MX = PORTB5  // P3A/CCP3 Mux bit (P3A/CCP3 input/output is multiplexed with RB5)
#pragma config HFOFST = ON      // HFINTOSC Fast Start-up (HFINTOSC output and ready status are not delayed by the oscillator stable status)
#pragma config T3CMX = PORTC0   // Timer3 Clock input mux bit (T3CKI is on RC0)
#pragma config P2BMX = PORTD2   // ECCP2 B output mux bit (P2B is on RD2)
#pragma config MCLRE = EXTMCLR  // MCLR Pin Enable bit (MCLR pin enabled, RE3 input pin disabled)

// CONFIG4L
#pragma config STVREN = ON      // Stack Full/Underflow Reset Enable bit (Stack full/underflow will cause Reset)
#pragma config LVP = ON         // Single-Supply ICSP Enable bit (Single-Supply ICSP enabled if MCLRE is also 1)
#pragma config XINST = OFF      // Extended Instruction Set Enable bit (Instruction set extension and Indexed Addressing mode disabled (Legacy mode))

// CONFIG5L
#pragma config CP0 = OFF        // Code Protection Block 0 (Block 0 (000800-001FFFh) not code-protected)
#pragma config CP1 = OFF        // Code Protection Block 1 (Block 1 (002000-003FFFh) not code-protected)
#pragma config CP2 = OFF        // Code Protection Block 2 (Block 2 (004000-005FFFh) not code-protected)
#pragma config CP3 = OFF        // Code Protection Block 3 (Block 3 (006000-007FFFh) not code-protected)

// CONFIG5H
#pragma config CPB = OFF        // Boot Block Code Protection bit (Boot block (000000-0007FFh) not code-protected)
#pragma config CPD = OFF        // Data EEPROM Code Protection bit (Data EEPROM not code-protected)

// CONFIG6L
#pragma config WRT0 = OFF       // Write Protection Block 0 (Block 0 (000800-001FFFh) not write-protected)
#pragma config WRT1 = OFF       // Write Protection Block 1 (Block 1 (002000-003FFFh) not write-protected)
#pragma config WRT2 = OFF       // Write Protection Block 2 (Block 2 (004000-005FFFh) not write-protected)
#pragma config WRT3 = OFF       // Write Protection Block 3 (Block 3 (006000-007FFFh) not write-protected)

// CONFIG6H
#pragma config WRTC = OFF       // Configuration Register Write Protection bit (Configuration registers (300000-3000FFh) not write-protected)
#pragma config WRTB = OFF       // Boot Block Write Protection bit (Boot Block (000000-0007FFh) not write-protected)
#pragma config WRTD = OFF       // Data EEPROM Write Protection bit (Data EEPROM not write-protected)

// CONFIG7L
#pragma config EBTR0 = OFF      // Table Read Protection Block 0 (Block 0 (000800-001FFFh) not protected from table reads executed in other blocks)
#pragma config EBTR1 = OFF      // Table Read Protection Block 1 (Block 1 (002000-003FFFh) not protected from table reads executed in other blocks)
#pragma config EBTR2 = OFF      // Table Read Protection Block 2 (Block 2 (004000-005FFFh) not protected from table reads executed in other blocks)
#pragma config EBTR3 = OFF      // Table Read Protection Block 3 (Block 3 (006000-007FFFh) not protected from table reads executed in other blocks)

// CONFIG7H
#pragma config EBTRB = OFF      // Boot Block Table Read Protection bit (Boot Block (000000-0007FFh) not protected from table reads executed in other blocks)

device_config.h header File

#define _XTAL_FREQ 16000000

I understand the importance of other peripheral initialization in a complete program, and I would include the necessary initialization for all relevant peripherals such as timers, interrupts, uart and any other components required for the overall system functionality

 

Overall, it looks good. I have three comments to make, which are my personal preferences.

1) Whether it is a small or large project, you will always make changes to the code. I would include a section after Description, followed by Created, a section on History that has a date and the changes, corrections and additions made.

2) Defines are always all upper case, true becomes TRUE.

3) As the project grows, the list of defines will also grow. I put these in a separate .h file which I call constant.h.

 

Multiple assignment in one line. I don't like that because of readability and the lack of sequences when reading and writing hardware ports as volatile variables.


Putting them all on the same line won't make it faster as the compiler will look at each assignment independently for code construction.

 

Overall, it looks good. I have three comments to make, which are my personal preferences.

1) Whether it is a small or large project, you will always make changes to the code. I would include a section after Description, followed by Created, a section on History that has a date and the changes, corrections and additions made.

2) Defines are always all upper case, true becomes TRUE.

3) As the project grows, the list of defines will also grow. I put these in a separate .h file which I call constant.h.

Multiple assignment in one line. I don't like that because of readability and the lack of sequences when reading and writing hardware ports as volatile variables.

I've made the suggested modifications to the code.

1. Added a "History" section after the "Description" to track changes, corrections, and additions.
2. Changed the true to uppercase TRUE
3. Created separate constant.h file
4. Created Multiple assignment for LAT registers

/*
* Summary : Basic LED Blinking Example
*
* Device: PIC18F45K22
* Compiler: XC8 2.36
* MPLAB: MPLAB X 6.00
* Language: C
*
* File: main.c
* Author: NAME
*
* Created on 9 November, 2023, 6:35 PM
* Description: This is a simple example code for the PIC18F45K22 microcontroller
*              that blinks four LEDs (Red, Green, Yellow, and Blue)  every 1 seconds in a loop.
*              The LEDs are connected to PORTD pins RD0 (RED), RD1 (GREEN),
*              RD2 (YELLOW), and RD3 (BLUE).
*
* History:
* 2023-11-09:
* Version 1 created by [Author's Name]
*/

#include <xc.h>
#include "device_config.h"
#include "constant.h"

/*
* Function: Initializes the oscillator settings for the PIC18F45K22.
* Returns: None
* Parameters: None
*/
void OSCILLATOR_Initialize(void)
{
    // Configure oscillator settings for the PIC18F45K22 16 MHz speed
    // SCS FOSC; HFIOFS not stable; IDLEN disabled; IRCF 8MHz_HF;
    OSCCON = 0x60;
    // SOSCGO disabled; MFIOSEL disabled; SOSCDRV Low Power;
    OSCCON2 = 0x00;
    // INTSRC INTRC; PLLEN disabled; TUN 0;
    OSCTUNE = 0x00;
}

/*
* Function: Initializes the ports (LATx, TRISx, ANSELx) for the PIC18F45K22.
* Returns: None
* Parameters: None
*/
void PORT_Initialize(void)
{
    // Initialize LATx registers to ensure all outputs are low
    LATA = 0x00;
    LATB = 0x00;
    LATC = 0x00;
    LATD = 0x00;
    LATE = 0x00;

    // Set all TRISx registers to configure all pins as outputs
    TRISA = 0x00;
    TRISB = 0x00;
    TRISC = 0x00;
    TRISD = 0x00;
    TRISE = 0x00;

    // Configure ANSELx registers to make all pins digital
    ANSELA = 0x00;
    ANSELB = 0x00;
    ANSELC = 0x00;
    ANSELD = 0x00;
    ANSELE = 0x00;
}

/*
* Function: Initializes the entire system by calling the necessary initialization functions.
* Returns: None
* Parameters: None
*/
void SYSTEM_Initialize(void)
{
    // Initialize the system by configuring ports and oscillator
    PORT_Initialize();
    OSCILLATOR_Initialize();
}

/*
* Function: The main function that initializes the system and blinks the LEDs in a loop.
* Returns: None
* Parameters: None
*/
void main(void)
{

    // Initialize the system components
    SYSTEM_Initialize();

    // Initialize all LEDs to the OFF state at start
    RED_PIN = GREEN_PIN = YELLOW_PIN = BLUE_PIN = OFF;

    // Main loop
    while (TRUE){

        // Turn on all LEDs
        RED_PIN = GREEN_PIN = YELLOW_PIN = BLUE_PIN = ON;
        __delay_ms(1000);

        // Turn off all LEDs
        RED_PIN = GREEN_PIN = YELLOW_PIN = BLUE_PIN = OFF;
        __delay_ms(1000);
    }
}