GLCD and interrupts

General discussion relating to the library modules supplied with the compiler

Moderators: David Barker, Jerry Messina

Post Reply
tcdev
Posts: 15
Joined: Thu Jan 15, 2015 12:31 am

GLCD and interrupts

Post by tcdev » Fri Jan 23, 2015 12:21 pm

As mentioned in another post, I've inherited an existing design and have been charged with fixing it, despite having zero prior experience with either PIC18F or Swordfish BASIC. I am an experienced embedded software engineer, so I'd argue it's not a hopeless case. :mrgreen: Oh, and it has to be done yesterday!

Anyway, the existing code uses the GLCD library. The only interrupt the code used was the ISRRX.OnData interrupt. Serial traffic is minimal.

I've taken a sledge-hammer to the design and added a low-priority ISR that runs ADC acquisition triggered off TIMER1/CCP2 and also TIMER0 (10ms). The ISR appears to work and I'm able to change the frequency of acquisitions as expected via CCPR2H/L.

The problem is, the interrupts appear to be corrupting the display. At very low frequency (of interrupt), the worst appears to be either side of the display (page) shifting up and down via a single pixel row. As I increase the frequency, the corruption worsens and at anything near the frequency I require for decent touch screen operation, it's totally corrupt and completely unreadable. Disabling the interrupt results in a perfect display.

Now I can't see why I shouldn't be able to interrupt the GLCD routines. Looking at the bus timing diagrams for the LCD module there don't appear to be any signals/operations that couldn't - in theory - be extended indefinitely without corrupting the display. I should also mention that the ADC plays with RBPU# (same port as LCD control bus) but all control signals are outputs so that shouldn't be an issue. Regardless, removing the code that toggles RBPU# has no effect. So I'm at a loss to explain what could be causing this.

My question at the end of all this is, can anyone think of why the GLCD routines can't be interrupted? There's still the possibility that I'm doing something stupid of course, but if they can't be interrupted, then it's going to be a nightmare getting this working properly, and then shouldn't the ISRRX interrupt also cause occasional corruption?

EDIT: I should probably include some code snippets...

The options for the GLCD module:

Code: Select all

#Option GLCD_MODEL = KS0108            
#option GLCD_SCREEN_WIDTH = 128      
#option GLCD_SCREEN_HEIGHT = 64     
#Option GLCD_DATA = PORTD           
#Option GLCD_RS  = PORTB.2           
#Option GLCD_RW  = PORTB.1           
#Option GLCD_EN  = PORTB.0          
#Option GLCD_CS1 = PORTB.4           
#Option GLCD_CS2 = PORTB.3           
#Option GLCD_RST = PORTE.3           
#Option GLCD_ASPECT_RATIO = 100      
#Option GLCD_INIT_DELAY = 200        
The initialisation routine for the ISR:

Code: Select all

Public Sub Initialise()

    state = 0

#ifdef ADC_ENABLE_ADC
    #ifdef ADC_USE_CCP2
        // CCP2 configuration for the ADC
        T3CON = %00000000       // Timer1 is compare clock
        CCPR2H = $10            // CCPR2 is timer period
        CCPR2L = 0
        CCP2CON = %00001011     // Special Event Trigger (ADC)
        // TIMER1 configuration for the ADC
        T1CON = %10110101       // 16-bit,1:8 prescale
    #endif

    // ADC configuration
    Input(PEN)
    TRISA = %00001111   // AN3-0 on PORTA
    ADCON1 = %00001011  // CCP2,AVss-AVdd, AN3-0
    // start (dummy) read on AN3
    ADCON0 = %00001100  // AN3,disabled
    ADCON2 = %10111010  // Right,20Tad,Fosc/32
    ADCON0.0 = 1        // enabled
    // ADC interrupt
    ADIP = 0            // Low priority interrupt
    ADIF = 0            // Clear any pending ADC interrupt flag
    ADIE = 1            // Enable ADC interrupt
    #ifndef ADC_USE_CCP2
        ADCON0.1 = 1    // GO
    #endif
#endif // ADC_ENABLE_ADC

...
And finally, the ISR (TS_X1 etc are all on PORTA):

Code: Select all

Interrupt OnTS(ipLow)

#ifdef ADC_ENABLE_ADC
    // ADC interrupt?
    If ADIF = 1 Then
        Select state
            Case 0
                // stop AN3
                ADCON0.0 = 0        // disabled
                // read result
                TEMPP = ADRES
                If TEMPP < TouchThreshold Then
                    If PCOUNT < 4 Then
                        Inc(PCOUNT)
                    Else
                        XBUF(BUFI) = TEMPX
                        YBUF(BUFI) = TEMPY
                        BUFI = (BUFI + 1) And 3
                    EndIf
                Else 
                    PCOUNT = 0
                EndIf
                ToggleLed ()
                // config for next read
                Input(TS_X1)        // X- floating
                Input(TS_X2)        // X+ floating
                Output(TS_Y1)       // Y- LOW
                TS_Y1 = 0
                Output(TS_Y2)       // Y+ HIGH
                TS_Y2 = 1
                RBPU = 1            // Disable PULL-UPs
                // start read on AN1
                ADCON0 = %00000100  // AN1,disabled
                ADCON2 = %10111010  // Right,20Tad,Fosc/32
                ADCON0.0 = 1        // enabled
                #ifndef ADC_USE_CCP2
                    ADCON0.1 = 1    // GO
                #endif
                state = 1
            Case 1
                // stop AN1
                ADCON0.0 = 0        // disabled
                // read result
                TEMPX = ADRES
                // config for next read
                Input(TS_Y1)        // Y- floating
                Input(TS_Y2)        // Y+ floating
                Output(TS_X1)       // X- LOW
                TS_X1 = 0
                Output(TS_X2)       // X+ HIGH
                TS_X2 = 1
                RBPU = 1            // Disable PULL-UPs
                // start read on AN3
                ADCON0 = %00001100  // AN3,disabled
                ADCON2 = %10111010  // Right,20Tad,Fosc/32
                ADCON0.0 = 1        // enabled
                #ifndef ADC_USE_CCP2
                    ADCON0.1 = 1    // GO
                #endif
                state = 2
            Case 2
                // stop AN3
                ADCON0.0 = 0        // disabled
                // read result
                TEMPY = ADRES
                // config for next read
                Input(TS_X2)        // X+ floating                          
                Input(TS_Y1)        // Y- floating                                   
                Input(TS_Y2)        // Y+ ADC input
                Output(TS_X1)       // X- LOW
                TS_X1 = 0
                RBPU = 0            // Enable PULL-UPs
                // start read on AN3
                ADCON0 = %00001100  // AN3,disabled
                ADCON2 = %10111010  // Right,20Tad,Fosc/32
                ADCON0.0 = 1        // enabled
                #ifndef ADC_USE_CCP2
                    ADCON0.1 = 1    // GO
                #endif
                state = 0
        Else
            state = 0
        EndSelect
        // Clear ADC Interrupt Flag
        // - doco suggests this should be done after reading result
        ADIF = 0
    EndIf
#endif // ADC_ENABLE_ADC

...

Jerry Messina
Swordfish Developer
Posts: 1469
Joined: Fri Jan 30, 2009 6:27 pm
Location: US

Re: GLCD and interrupts

Post by Jerry Messina » Fri Jan 23, 2015 1:50 pm

It can take a little getting familiar with SF and interrupts, ram recycling, save/restoring context, etc.

Looking at the isr code, I'd be suspicious of one of the FSR registers getting corrupted since nothing is saved and the isr is using arrays.

For a quick test, try this:
- move all of the ADC code out of the ISR and into a subroutine
- add save/restore around the sub call

example:

Code: Select all

sub handleADC()
    Select state
        Case 0
            // stop AN3
            ADCON0.0 = 0        // disabled
            // read result
            TEMPP = ADRES
            If TEMPP < TouchThreshold Then
                If PCOUNT < 4 Then
                    Inc(PCOUNT)
                Else
                    XBUF(BUFI) = TEMPX
                    YBUF(BUFI) = TEMPY
                    BUFI = (BUFI + 1) And 3
                EndIf
            Else
                PCOUNT = 0
            EndIf
            ToggleLed ()
            // config for next read
            Input(TS_X1)        // X- floating
            Input(TS_X2)        // X+ floating
            Output(TS_Y1)       // Y- LOW
            TS_Y1 = 0
            Output(TS_Y2)       // Y+ HIGH
            TS_Y2 = 1
            RBPU = 1            // Disable PULL-UPs
            // start read on AN1
            ADCON0 = %00000100  // AN1,disabled
            ADCON2 = %10111010  // Right,20Tad,Fosc/32
            ADCON0.0 = 1        // enabled
            #ifndef ADC_USE_CCP2
                ADCON0.1 = 1    // GO
            #endif
            state = 1
        Case 1
            // stop AN1
            ADCON0.0 = 0        // disabled
            // read result
            TEMPX = ADRES
            // config for next read
            Input(TS_Y1)        // Y- floating
            Input(TS_Y2)        // Y+ floating
            Output(TS_X1)       // X- LOW
            TS_X1 = 0
            Output(TS_X2)       // X+ HIGH
            TS_X2 = 1
            RBPU = 1            // Disable PULL-UPs
            // start read on AN3
            ADCON0 = %00001100  // AN3,disabled
            ADCON2 = %10111010  // Right,20Tad,Fosc/32
            ADCON0.0 = 1        // enabled
            #ifndef ADC_USE_CCP2
                ADCON0.1 = 1    // GO
            #endif
            state = 2
        Case 2
            // stop AN3
            ADCON0.0 = 0        // disabled
            // read result
            TEMPY = ADRES
            // config for next read
            Input(TS_X2)        // X+ floating                         
            Input(TS_Y1)        // Y- floating                                   
            Input(TS_Y2)        // Y+ ADC input
            Output(TS_X1)       // X- LOW
            TS_X1 = 0
            RBPU = 0            // Enable PULL-UPs
            // start read on AN3
            ADCON0 = %00001100  // AN3,disabled
            ADCON2 = %10111010  // Right,20Tad,Fosc/32
            ADCON0.0 = 1        // enabled
            #ifndef ADC_USE_CCP2
                ADCON0.1 = 1    // GO
            #endif
            state = 0
        Else
            state = 0
    EndSelect
end sub


Interrupt OnTS(ipLow)
  #ifdef ADC_ENABLE_ADC
    // ADC interrupt?
    If ADIF = 1 Then
        save(0, handleADC)      // saves pretty much everything + resources used by handleADC
        handleADC()
        restore
        // Clear ADC Interrupt Flag
        ADIF = 0
    EndIf
  #endif // ADC_ENABLE_ADC
end interrupt
If that works, then you can start whittling down the 'save' to find out what's actually needed, just save that, and put it back together. Or, take a look at the asm output of the isr and see what's getting accessed that needs to be saved.


just FYI, for setting an output high or low you can also do that in a single statement:

Code: Select all

Output(TS_X1)       // X- LOW
TS_X1 = 0

// can be done as
low(TS_X1)
// or
high(TS_X1)
Low() and High() make the pin an output and set the output state

User avatar
octal
Registered User
Registered User
Posts: 586
Joined: Thu Jan 11, 2007 12:49 pm
Location: Paris IDF
Contact:

Re: GLCD and interrupts

Post by octal » Fri Jan 23, 2015 3:47 pm

While the modification suggested by Jerry may solve the problem, I would say that doing anything in any ISR is really the wrong way of doing things.
Any good and well designed application should be designed using either an RTOS, or a state machine.
- In the first case, RTOS offers all needed stuff to handle concurrency even between the ISR and non ISR tasks.
- In the case of state machine, it's better to have the interrupt do the extreme minimum work (and never call any routine), and to setup some flags or completely switch the state machine active state to something that's handled right after having left the ISR.

Jerry Messina
Swordfish Developer
Posts: 1469
Joined: Fri Jan 30, 2009 6:27 pm
Location: US

Re: GLCD and interrupts

Post by Jerry Messina » Sat Jan 24, 2015 12:55 pm

The isr LOOKS like it's long and involved, but if you take a closer look it's already arranged as a state machine and the code is actually pretty straight forward. I completely agree with octal on the point that in general you DO need to watch what it is you put in an isr. You can easily make a real mess for yourself if you're not very careful (btw, that's not directed to tcdev, just a general comment).

For kicks I compiled the code I posted above, and this is what I noticed (I assumed that all of the variables used were bytes, and if they're not then what follows may not apply)

The statements

Code: Select all

	XBUF(BUFI) = TEMPX
	YBUF(BUFI) = TEMPY
involve the use of FSR0 to do the array indexing

The statement

Code: Select all

	BUFI = (BUFI + 1) AND 3
involves using a 16-bit temp variable from the stack frame to hold the RHS of the expression

The sub ToggleLed() is an unknown, since it's not shown. If it does anything more than literally change the state of an IO pin, then all bets are off.

If you replace the statement

Code: Select all

	BUFI = (BUFI + 1) AND 3
with

Code: Select all

    BUFI = BUFI + 1
    BUFI = BUFI And 3
the compiler can optimize that such that it no longer requires the use of a 16-bit temp... only the module level BUFI variable. That actually ends up saving 10 bytes or so of code, and you no longer have to worry about saving the temp stack frame variable.

If you make that change, then all you should need to do is save the FSR0 register, so at a minimum you should have

Code: Select all

save(FSR0)
<handleADC call or inline code>
restore
There's typically a difference in what you need to do depending on whether the code in the isr is directly inline or in a subroutine.
If you call subroutines from the isr then you normally need to tell the compiler about that in the 'save(...)' statement.
If the code is inline (as you had originally), then usually you only need to worry about the FSR0/1/2 and PRODH/L registers.
The system registers 'save(0, ...)' normally come into play when using math functions or system library calls.

Usually most isr code tends to need at least a

Code: Select all

save(FSR0, FSR1, PRODH, PRODL)
statement on entry, but it's hard to make a blanket statement like that. When in doubt, take a look at the asm results after you compile. It's always enlightening.

tcdev
Posts: 15
Joined: Thu Jan 15, 2015 12:31 am

Re: GLCD and interrupts

Post by tcdev » Tue Jan 27, 2015 2:27 am

Jerry Messina wrote:The isr LOOKS like it's long and involved, but if you take a closer look it's already arranged as a state machine and the code is actually pretty straight forward. I completely agree with octal on the point that in general you DO need to watch what it is you put in an isr. You can easily make a real mess for yourself if you're not very careful (btw, that's not directed to tcdev, just a general comment).

(snip)
Firstly, very much appreciated that you've taken the time to play around with the code. My next step was to do more reading on the PIC and then start looking at the assembler that was generated for exactly this type of thing. Looks like you've done the hard work for me - thanks again!

As a general comment; I've worked on different processors from H8/500's to 32-bit ARMs and soft-core processors to Wintel (Device Drivers) and never in my wildest dreams would I have imagined that hitting a few registers to set up the next ADC would ever be considered "excessive" for an ISR. Totally agree with you and Octal on the point of good design, though as explained I don't have the luxury of re-architecting this particular piece of software. I believed I was being pretty conservative just triggering the ADC, buffering the result, and setting up the next conversion in the ISR. That would certainly have cut the mustard on pretty much any other processor I've ever worked on.

In any case, it would appear I've over-estimated the humble PIC18F and need to pay closer attention to the code produced. Shall take all your advice and code snippets on board and hopefully knock this thing over in the next few days. Then I never want to see it again! ;)

tcdev
Posts: 15
Joined: Thu Jan 15, 2015 12:31 am

Re: GLCD and interrupts

Post by tcdev » Tue Jan 27, 2015 11:51 am

Many thanks to Jerry and Octal; I've implemented your suggestions and it's all good!

FTR it seems only FSR0 was being used - once I removed the call to ToggleLED() - so a simple save(FSR0) did the trick! Also noted the High()/Low() optimisations.

You've certainly saved me some head scratching with a quick primer and straight to the point - much appreciated! :D

Post Reply