0
\$\begingroup\$

I'm working on a firmware for a product which uses a PIC18F26J50, an external ADC and an external I2C temperature sensor, besides a RS485 link through UART. So far so good. I'm writing small test programs for each feature I implemented just to make sure nothing breaks and so I can test everything later.

I just implemented the clock module and delay module: The clock module just sets up the internal oscillator and enables the PPL so that I have a 48MHz signal entering the main CPU and the delay module just implements a blocking 1us delay routine.

The problem is: When I run the test clock hex, it shows me in RA6 a beautiful 12MHz signal, which is OK because this output receives CLK / 4 when INTOSCPLLO is enabled in fuses. Now, I expect that delay tests would give a 1MHz (or something close to it), but it is giving a weird 5.45kHz waveform. I don't know how can I fix this.

Useful information:

  1. INTOSCPLLO is enabled in clock test and disabled in delay test;

  2. Relevant part of the circuit is like this:

Circuit

  1. I'm using a single fuses.h file to setup the fuses. I'm not sure if this approach is OK or should I use the IDE interface for setting up the fuses?

  2. Firmware package is here.

Relevant source code

testClock.c

#include "fuses.h"
#include "processor.h"
#include "defs.h"

#include "clock.h"

// running this program will make
// a 12MHz clock signal avaliable
// on RA6 to be checked with a
// oscilloscope

VOID main( VOID )
{
    clockInit();

    while(1);   
}

clock.c

#include "defs.h"
#include <p18cxxx.h>

enum
{
    IOF_8MHz    = 7,
    IOF_4MHz    = 6,
    IOF_2MHz    = 5,
    IOF_1MHz    = 4, 
    IOF_500kHz  = 3, 
    IOF_250kHz  = 2, 
    IOF_125kHz  = 1, 
    IOF_32kHz   = 0
};



/*----------------------------------------------------------------------------*/
VOID clockInit( VOID )
{
    // disables reference oscillator output
    REFOCONbits.ROON = 0;

    // device enters IDLE mode on SLEEP instruction
    OSCCONbits.IDLEN = 1;     
    // system clock is from primary oscillator
    OSCCONbits.SCS = 0;  
    // selects base frequency as 8MHz
    OSCCONbits.IRCF = IOF_8MHz;

    // calibration for internal oscillator (need to store this)
    OSCTUNEbits.TUN = 0b111110;    

    // wait until oscillator is running
    while( !OSCCONbits.OSTS );

    // low speed USB (6MHz clock)
    UCFGbits.FSEN = 0;   

    // enable PLL
    OSCTUNEbits.PLLEN = 1;     
}

fuses.h

#ifndef FUSES_H
#define FUSES_H

#include "config.h"
#include <p18cxxx.h>

#ifdef  __cplusplus
extern "C" {
#endif

#ifdef __18CXX

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.

// CONFIG1L

#ifdef _USE_WDT
    #pragma config WDTEN = ON      // Watchdog Timer (disabled)
#else
    #pragma config WDTEN = OFF     // Watchdog Timer (enabled)
#endif

#pragma config PLLDIV = 2       // PLL Prescaler Selection bits (Divide by 2 (8 MHz internal oscillator input))
#pragma config STVREN = ON      // Stack Overflow/Underflow Reset  (Enabled)

#ifdef _USE_EXTENDED_MODE
#pragma config XINST = ON       // Extended Instruction Set (Enabled)
#else
#pragma config XINST = OFF      // Extended Instruction Set (Disabled)    
#endif    

// CONFIG1H
#pragma config CPUDIV = OSC1    // CPU System Clock Postscaler (CPU system clock divide by 2)
#pragma config CP0 = OFF        // Code Protect (Program memory is not code-protected)

// CONFIG2L
#ifdef _CLOCK_OUT_ON_RA6
#pragma config OSC = INTOSCPLLO  // Oscillator (INTOSCPLL + clock out on RA6)
#else
#pragma config OSC = INTOSCPLL  // Oscillator (INTOSCPLLO)  
#endif    
#pragma config T1DIG = OFF      // T1OSCEN Enforcement (Secondary Oscillator clock source may be selected)
#pragma config LPT1OSC = OFF    // Low-Power Timer1 Oscillator (High-power operation)
#pragma config FCMEN = OFF      // Fail-Safe Clock Monitor 
#pragma config IESO = OFF       // Internal External Oscillator Switch Over Mode

// CONFIG2H
#pragma config WDTPS = 32768    // Watchdog Postscaler (1:32768)

// CONFIG3L
#pragma config DSWDTOSC = INTOSCREF // DSWDT Clock Select (DSWDT uses INTRC)
#pragma config RTCOSC = T1OSCREF    // RTCC Clock Select (RTCC uses T1OSC/T1CKI)
#pragma config DSBOREN = OFF        // Deep Sleep BOR (Enabled)
#pragma config DSWDTEN = OFF        // Deep Sleep Watchdog Timer (Enabled)
#pragma config DSWDTPS = G2         // Deep Sleep Watchdog Postscaler (1:2,147,483,648 (25.7 days))

// CONFIG3H
#pragma config IOL1WAY = OFF     // IOLOCK One-Way Set Enable bit (The IOLOCK bit (PPSCON<0>) can be set/cleared as needed)
#pragma config MSSP7B_EN = MSK7  // MSSP address masking (7 Bit address masking mode)

// CONFIG4L
#pragma config WPFP = PAGE_63    // Write/Erase Protect Page Start/End Location (Write Protect Program Flash Page 63)
#pragma config WPEND = PAGE_WPFP // Write/Erase Protect Region Select (valid when WPDIS = 0) (Page WPFP<5:0> through Configuration Words erase/write protected)
#pragma config WPCFG = OFF       // Write/Erase Protect Configuration Region (Configuration Words page not erase/write-protected)

// CONFIG4H
#pragma config WPDIS = OFF       // Write Protect Disable bit (WPFP<5:0>/WPEND region ignored)

#endif // #ifdef __18CXX

#ifdef  __cplusplus
}
#endif

#endif  /* FUSES_H */

processor.h

#ifndef PROCESSOR_H
#define PROCESSOR_H

#ifdef __18F26J50

#ifdef __DEBUG
#warning CPU is PIC18F26J50
#endif

#define CLOCK 48000000UL

#else /* #ifdef __18F26J50 */

#error WRONG microcontroller defined!

#endif /* #else #ifdef __18F26J50 */

#endif  /* PROCESSOR_H */

defs.h

#ifndef DEFS_H
#define DEFS_H

#ifdef __18CXX
#include <GenericTypeDefs.h>
#else
#error NEED DEFINES FOR DATATYPES!
#endif

#define DIGITAL_INPUT  1
#define DIGITAL_OUTPUT 0

#endif  /* DEFS_H */

testDelay.c

#include "fuses.h"
#include "processor.h"
#include "defs.h"

#include "led.h"
#include "mydelay.h"
#include "clock.h"

VOID main( VOID )
{
    ledAliveInit();
    clockInit();

    ledAliveOn();   

    while(1)
    {
        ledAliveOff();
        delay1us();
        ledAliveOn();
        delay1us();
    }

}

led.h

#ifndef __LED_H__
#define __LED_H__

#include "defs.h"

VOID    ledAliveInit    ( VOID );
VOID    ledAliveOn      ( VOID );
VOID    ledAliveOff     ( VOID );
VOID    ledAliveToogle  ( VOID );

#endif

mydelay.h

#ifndef MYDELAY_H
#define MYDELAY_H

#include "defs.h"

#ifdef  __cplusplus
extern "C" {
#endif

VOID delay1us( VOID );
VOID delay1ms( VOID );
VOID delay1s ( VOID );

#ifdef  __cplusplus
}
#endif

#endif  /* MYDELAY_H */

led.c

#include "led.h"
#include <p18cxxx.h>


/*----------------------------------------------------------------------------*/

#define LED_ON  1
#define LED_OFF 0
#define GPIO_LED_TRIS       TRISAbits.TRISA6
#define GIPO_LED_PORT_OUT   LATAbits.LATA6
#define GIPO_LED_PORT_IN    PORTAbits.RA6

/*----------------------------------------------------------------------------*/
enum
{
    ledStateOff = 0,
    ledStateOn
};

PRIVATE INT8 ledState_;


/*----------------------------------------------------------------------------*/

VOID ledAliveInit( VOID )
{
    GPIO_LED_TRIS = DIGITAL_OUTPUT;   
    ledAliveOff();   
}

/*----------------------------------------------------------------------------*/

VOID ledAliveOn( VOID )
{
    GIPO_LED_PORT_OUT = LED_ON;
    ledState_ = ledStateOn;
}

/*----------------------------------------------------------------------------*/

VOID ledAliveOff( VOID )
{
    GIPO_LED_PORT_OUT = LED_OFF;
    ledState_ = ledStateOff;
}

/*----------------------------------------------------------------------------*/

VOID ledAliveToogle( VOID )
{
    if ( ledStateOn == ledState_ )
    {
        ledAliveOff();
    }
    else if( ledStateOff == ledState_ )
    {
        ledAliveOn();
    }
}

mydelay.c

#include "mydelay.h"
#include "processor.h"

#include <p18cxxx.h>
#include <delays.h>

#define DELAYS_FOR_1S       1000UL
#define DELAYS_FOR_1MS      1000UL
#define DELAYS_FOR_1US      (CLOCK/1000000UL)

/*----------------------------------------------------------------------------*/

VOID delay1s( VOID )
{
    UINT16 i;

    for( i = 0; i < DELAYS_FOR_1S; ++i )
    {
        delay1ms();
    }
}

/*----------------------------------------------------------------------------*/

VOID delay1ms( VOID )
{
    UINT16 i;

    for( i = 0; i < DELAYS_FOR_1MS; ++i )
    {
        delay1us();
    }
}

/*----------------------------------------------------------------------------*/

VOID delay1us( VOID )
{
    UINT16 i;

    for( i = 0; i < DELAYS_FOR_1US; ++i )
    {
        Nop();
    }
}

config.h

#ifndef CONFIG_H
#define CONFIG_H

#ifdef  __cplusplus
extern "C" {
#endif

#define _NO_USE_EXTENDED_MODE

#ifdef _TEST_CLOCK_FW
#define _CLOCK_OUT_ON_RA6
#define _NO_USE_WDT    
#endif

#ifdef _TEST_LED_BLINK_FW
#define _NO_USE_WDT
#endif    

#ifdef _TEST_UART
#define _NO_USE_WDT
#endif    

#ifdef _TEST_DELAY
#define _NO_USE_WDT
#define _NO_CLOCK_OUT_ON_RA6
#endif    

#ifdef  __cplusplus
}
#endif

#endif  /* CONFIG_H */
\$\endgroup\$
2
  • \$\begingroup\$ please include the relevant part of your code in the question. \$\endgroup\$
    – Mike
    Sep 24 '19 at 8:30
  • \$\begingroup\$ OK. Added source code relevant to question. \$\endgroup\$ Sep 24 '19 at 17:09
2
\$\begingroup\$

The delayus() function is flawed. It won't take one instruction to go round the loop, but multiple instructions. In disassembly a call to delayus() is two instructions. Delayus() itself is about 14 instructions with about nine of them being executed 48 times. That's about 450 instructions just for the delay us function - and that is with optimizations enabled.

call to delayus

ECED        CALL 0xFFDA, 0    
F07F        NOP   

delay1us

0E00        MOVLW 0x0   
6E02        MOVWF 0x2, ACCESS
0E00        MOVLW 0x0
6E01        MOVWF __pcstackCOMRAM, ACCESS
5002        MOVF 0x2, W, ACCESS
E108        BNZ 0xFFF6
0E30        MOVLW 0x30
5C01        SUBWF __pcstackCOMRAM, W, ACCESS
B0D8        BTFSC STATUS, 0, ACCESS
0012        RETURN 0
F000        NOP
4A01        INFSNZ __pcstackCOMRAM, F, ACCESS
2A02        INCF 0x2, F, ACCESS
D7F6        BRA 0xFFE2
0012        RETURN 0

If you are wanting better timing, I'd use the PICs timer function. If you are wanting to output a accurate clock I'd use the PICs PWM function. If you really want to use a delay() function, look into PLIB (Peripheral library)

I also don't understand how you are getting 12MHz instruction clock if CPUDIV is 2 and PLLdiv is 2 by looking at the datasheet, I'd expect a 24MHz microcontroller clock frequency. You've also tuned off the centre frequency (OSCTUNEbits.TUN) making it difficult to know exact operating frequency, I'd just set it to 0.

If it's definitely a 12MHz instruction clock, follow what Mike suggested instead of your delay1us function

#define delay1us() Nop(); Nop(); Nop(); Nop(); Nop(); Nop(); Nop(); Nop(); Nop(); Nop(); Nop(); Nop()
\$\endgroup\$
6
  • \$\begingroup\$ You said I turned off centre frequency. But in fact, I did turned it on, right? Would you turn it off? Why? \$\endgroup\$ Sep 26 '19 at 13:26
  • \$\begingroup\$ It is perhaps off the point but I'd only tune the frequency for calibration purposes - for example if you are running some time critical operations for environmental reasons. This particular application is far away from that. If you are wanting to measure a particular frequency and know the expected frequency, keep it on centre i.e. 0. \$\endgroup\$
    – pm101
    Sep 26 '19 at 13:32
  • \$\begingroup\$ Doesn't PLIB functions depend on the actual clock value? \$\endgroup\$ Sep 26 '19 at 20:51
  • 1
    \$\begingroup\$ If you really want a quick and dirty solution use a macro with 12 Nop()for the delay \$\endgroup\$
    – Mike
    Sep 27 '19 at 8:25
  • \$\begingroup\$ I'm implementing this stuff using the timer. Thanks for the tip! Regarding the clock, two notes: 1.) CPUDIV is 1, not 2 and 2.) from datasheet (page 36) the PLL expects a 4MHz clock as input, which is why PLLDIV is 2, since I'm using the internal oscillator clocked at 8MHz. The output of the PLL goes to CPU Divider and since CPDIV = OSC1, it gets divided by 1. So, we have 48MHz entering in OSCCON<1:0> and in CLOCKO enabled modes, it gets divided by 4 before going to RA. This is why I expected a 12MHz output on RA6. This PIC seems to be a different beast, at least from the others I know. \$\endgroup\$ Oct 1 '19 at 18:42
0
\$\begingroup\$

Use Timer to generate periodic interrupt of 1 usec, then use counters for millisecond, seconds this will be more accurate and will not block codes, below is a code to generate millisecond timer with 8Mhz clock

#define MILLISEC_MULTIPLIER 0.001 //1 milliseconds
#define TIMER3_PRESCALER 4
#define MILLISEC_PERIOD (0xffff - (int)(MILLISEC_MULTIPLIER * OSC_FREQ/TIMER3_PRESCALER))
void TIMER3_Initialize(void){
    //generate milli seconds timer
    TMR3H = MILLISEC_PERIOD >> 8;
    TMR3 = MILLISEC_PERIOD;
    //clear interrupt flag
    PIR2bits.TMR3IF = 0;
    //enable timer3 interrupt
    PIE2bits.TMR3IE = 1;
    T3CON = 0x63;
}

void Timer3_Isr(void){
    TMR3IF = 0;
    //reload period to timer3 register
    TMR3H = MILLISEC_PERIOD >> 8;
    TMR3 = MILLISEC_PERIOD;
    //update millisecond timer register
    if(millisecond_timer_register == 0xffff){
        millisecond_timer_register = 0;
    }else{
        millisecond_timer_register++;
    }
    //millisecond_timer_register++;
}

If you controller support timer autoload features then you can use that to avoid small drift in the pulse generation due to period reload instructions

\$\endgroup\$
3
  • \$\begingroup\$ Which compiler are you using for this? \$\endgroup\$ Oct 1 '19 at 19:21
  • \$\begingroup\$ @daniel.franzini, MPLAB5.20 \$\endgroup\$
    – Raj
    Oct 2 '19 at 2:52
  • \$\begingroup\$ XC8 i'd guess . \$\endgroup\$
    – pm101
    Oct 2 '19 at 9:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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