bug fixes for USB V1.4.2 lib

General discussion relating to the library modules supplied with the compiler

Moderators: David Barker, Jerry Messina

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

bug fixes for USB V1.4.2 lib

Post by Jerry Messina » Thu Nov 21, 2013 2:50 pm

A particularly nasty bug has surfaced in the USB V142 code

In USBSystem.bas there are two helper functions to set/get ram memory indirectly by address using the FSR registers:

Code: Select all

inline sub SetRAMPtr(pAddress as FSR0, pValue as POSTINC0)
end sub

inline function GetRAMPtr(pAddress as FSR1) as POSTINC1
end function
When used alone, these routines appear(ed) to work as desired.

However, in the sub USBCtrlTrfRxService() I used them together in one statement:

Code: Select all

SetRAMPtr(pDst.bRam, GetRAMPtr(pSrc.bRam))
And this doesn't work as intended. If you look at the asm produced for that you see

Code: Select all

?I000291_F010_001657_P000032 ; L#MK SETRAMPTR(PDST.BRAM, GETRAMPTR(PSRC.BRAM))
    MOVFF M7_U16H,_FSR1L#F0_U16H
    MOVFF M7_U16,_FSR1L#F0_U16
    MOVFF POSTINC1,POSTINC0			;<<< COPY IS DONE BEFORE FSR0 IS SET!!
    MOVFF M9_U16H,_FSR0L#F0_U16H
    MOVFF M9_U16,_FSR0L#F0_U16
Not only does the resulting code end up copying the data incorrectly, it also writes a byte
to wherever FSR0 happens to be pointing to initially, trashing some (random) memory contents!

It turns out that I was too smart for my own good. If you look closely at the routine

Code: Select all

inline sub SetRAMPtr(pAddress as FSR0, pValue as POSTINC0)
end sub
you'll see a glaring mistake. The intent of that routine is to load the FSR0 register, and then use POSTINC0 to write the value.
That works as long as the parameters are evaluated left to right. But, when I used a function call as 'pValue', I forced the
compiler to evaluate that first!. So, 'SetRAMPtr()' as coded is seriously broken since it relies on a certain order of evaluation.

By replacing the sub with a macro, this can be fixed. A better implementation would be:

Code: Select all

macro SetRAMPtr(pAddress, pValue)
    FSR0 = pAddress
    POSTINC0 = pValue
end macro
Also, you should replace the routine USBCtrlTrfRxService() with the following:

Code: Select all

sub USBCtrlTrfRxService()
    // This code executes in the Data stage of a Control Write transfer
    dim byte_to_read as word		// count of bytes to copy

    // the two msb's of the count are in two lsb's of BDxSTAT (BC9, BC8)
    byte_to_read.byte1 = ep0Bo.Stat._byte and $03
    byte_to_read.byte0 = ep0Bo.Cnt

    // Accumulate total number of bytes read
    wCount = wCount + byte_to_read

	// copy bytes from src (FSR1) to dest (FSR0)
    pSrc.bRam = addressof(CtrlTrfData)
    FSR1 = pSrc.bRam
    FSR0 = pDst.bRam
    while (byte_to_read > 0)
        POSTINC0 = POSTINC1
        dec(byte_to_read)
    end while
	// and update buffer pointers
    pSrc.bRam = FSR1
    pDst.bRam = FSR0
end sub

Optional Changes:
The compiler changed somewhat back around V2.2.1.4 or so, and if you compile the existing code for interrupt
mode (#option USB_SERVICE = true), you'll see a number of warning messages such as
[Warning] usbsystem.bas: Inline procedure called from within context save block, disabling inline code generation
If you wish to get rid of these warnings, locate the following misc routines in USBSystem.bas:

Code: Select all

inline sub Nop()
    asm
        Nop
    end asm
end sub

inline function GetRAMPtr(pAddress as FSR1) as POSTINC1
end function

inline function ReadROM() as TABLAT
    asm
        TBLRD *+
    end asm
end function

inline function GetROMPtr(pAddress as TABLEPTR) as TABLAT
    ReadROM()
end function
And replace them with the code below

Code: Select all

macro nop()
    asm
        Nop
    end asm
end macro

function GetRAMPtr(pAddress as FSR1) as POSTINC1
end function

function ReadROM() as TABLAT
    asm
        TBLRD *+
    end asm
end function

function GetROMPtr(pAddress as TABLEPTR) as TABLAT
    asm
        TBLRD *+
    end asm
end function
That should get rid of the warnings


While you're making changes there's another goof... I seem to have left some debug code in the CDC interrupt handler
file USBCDC.bas:

Code: Select all

#if (USB_SERVICE)
interrupt USB_intr_handler(PRIORITY_LEVEL)
high(porta.0)		<< ****REMOVE THIS LINE****
    // check for USB intr request
    if ((PIR2Bits.USBIF = 1) and (PIE2Bits.USBIE = 1)) then
        PIR2Bits.USBIF = 0
        save(0, FSR0, FSR1, _service)
        _service()
        restore
    endif
low(porta.0)		<< ****REMOVE THIS LINE****
end interrupt
#endif

bitfogav
Registered User
Registered User
Posts: 169
Joined: Sat Oct 09, 2010 1:39 pm
Location: United Kingdom

Post by bitfogav » Thu Nov 21, 2013 3:36 pm

Thank you Jerry for the fixes.. will there be an updated zip file we can download?

User avatar
RangerBob
Posts: 152
Joined: Thu May 31, 2007 8:52 am
Location: Beds, UK

Post by RangerBob » Mon Nov 25, 2013 5:24 pm

Thanks Jerry,

Been using your USB libs extensively and only ever spotted the Interrupt one, but thought it was something I introduced! ;)

Rangerbob

bitfogav
Registered User
Registered User
Posts: 169
Joined: Sat Oct 09, 2010 1:39 pm
Location: United Kingdom

Re: bug fixes for USB V1.4.2 lib

Post by bitfogav » Wed May 28, 2014 12:06 pm

I just wanted to share that I have made a 64Bit Windows OS version of the HID bootloader application program.
HIDBootLoader64Bit.zip
(33.73 KiB) Downloaded 299 times

bitfogav
Registered User
Registered User
Posts: 169
Joined: Sat Oct 09, 2010 1:39 pm
Location: United Kingdom

Re: bug fixes for USB V1.4.2 lib

Post by bitfogav » Sat Jul 12, 2014 4:18 pm

I have been using the 18F46J53 with Jerry's ported SF version of microchips HID bootloader, I came across a problem where I was unable to reconnect to the bootloader after the user uploaded the main program code..

This has nothing to do with Jerry's ported code but the fact that once again microchip has the wrong data stored within microchips HID bootloader source, even the latest version of microchips HID bootloader is incorrect..

The fix:
Within the SF HID bootloader file HIDBootloader18F.bas I have replaced the following code, to bring the configuration words inline with the 18F46J53:

Code: Select all

#elseif _device in (18F26J53, 18F46J53)
    #define DEVICE_J_SERIES
    #define _EraseBlockSize_            = 1024      // 1024 byte erase block
    #define _StartPageToErase_          = 4         //application start page
    #define _MaxPageToEraseNoConfigs_   = 94        //Last page of flash without flash configuration words
    #define _MaxPageToEraseWithConfigs_ = 95        //Page 95 contains the flash configurations words
    #define _ProgramMemStopNoConfigs_   = $017C00   //**MUST BE WORD ALIGNED (EVEN) ADDRESS
    #define _ProgramMemStopWithConfigs_ = $017FF8   //**MUST BE WORD ALIGNED (EVEN) ADDRESS
    #define _ProgramBlockSize_          = $40       //64 byte flash block
    #define _ConfigWordsStartAddress_   = $017FF8   //$XXXF8 is CONFIG1L
    #define _ConfigWordsSectionLength_  = $08       //8 bytes worth of Configuration words
To

Code: Select all

#elseif _device in (18F26J53, 18F46J53)
    #define DEVICE_J_SERIES
    #define _EraseBlockSize_            = 1024      // 1024 byte erase block
    #define _StartPageToErase_          = 4         //application start page
    #define _MaxPageToEraseNoConfigs_   = 62        //Last page of flash without flash configuration words
    #define _MaxPageToEraseWithConfigs_ = 63        //Page 63 contains the flash configurations words
    #define _ProgramMemStopNoConfigs_   = $00FC00   //**MUST BE WORD ALIGNED (EVEN) ADDRESS
    #define _ProgramMemStopWithConfigs_ = $00FFF8   //**MUST BE WORD ALIGNED (EVEN) ADDRESS    
    #define _ProgramBlockSize_          = $40       //64 byte flash block
    #define _ConfigWordsStartAddress_   = $00FFF8   //$XXXF8 is CONFIG1L
    #define _ConfigWordsSectionLength_  = $08       //8 bytes worth of Configuration words
The 18F46J53 now works correctly..
Last edited by bitfogav on Sat Jul 12, 2014 6:09 pm, edited 1 time in total.

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

Re: bug fixes for USB V1.4.2 lib

Post by Jerry Messina » Sat Jul 12, 2014 6:09 pm

Thanks for the heads up!

Also, you can add a few others to the mix:

Code: Select all

#elseif _device in (18F27J53, 18F47J53)
    #define DEVICE_J_SERIES
    #define _EraseBlockSize_            = 1024      // 1024 byte erase block
    #define _StartPageToErase_          = 4         //application start page
    #define _MaxPageToEraseNoConfigs_   = 126       //Last page of flash without flash configuration words
    #define _MaxPageToEraseWithConfigs_ = 127       //Page 127 contains the flash configurations words
    #define _ProgramMemStopNoConfigs_   = $01FC00   //**MUST BE WORD ALIGNED (EVEN) ADDRESS
    #define _ProgramMemStopWithConfigs_ = $01FFF8   //**MUST BE WORD ALIGNED (EVEN) ADDRESS
    #define _ProgramBlockSize_          = $40       //64 byte flash block
    #define _ConfigWordsStartAddress_   = $01FFF8   //$XXXF8 is CONFIG1L
    #define _ConfigWordsSectionLength_  = $08       //8 bytes worth of Configuration words
I've been looking at incorporating the changes needed for the J94/J99 and 45K50 series, too. Hopefully I'll get a chance at some point soon...

bitfogav
Registered User
Registered User
Posts: 169
Joined: Sat Oct 09, 2010 1:39 pm
Location: United Kingdom

Re: bug fixes for USB V1.4.2 lib

Post by bitfogav » Sat Jul 12, 2014 6:20 pm

That's strange because the 18F47J53 seems to work fine with the bootloader.. :wink:

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

Re: bug fixes for USB V1.4.2 lib

Post by Jerry Messina » Sat Jul 12, 2014 6:40 pm

That's strange because the 18F47J53 seems to work fine with the bootloader..
Duh! It seems the 47J53 was already in there ok.

Nevermind...

Post Reply