Skip to main content
Topic: PIC programming adapter problem (Read 22394 times) previous topic - next topic

Re: PIC programming adapter problem

Reply #15
Could you attach the patch file, rather than attaching the content? So we can keep the formating consistent (tabs are translated to spaces, etc etc)

Re: PIC programming adapter problem

Reply #16
Here is the patch file (#ifdef changed to DEBUG).

Re: PIC programming adapter problem

Reply #17
Thanks :) .... commited

Re: PIC programming adapter problem

Reply #18
Sorry, was busy a lot today with Make. Nice thing that it got committed. Yay, we have another PIC in the library! :)

PS: I have two more that I'll add soon but ssshhhh :)

Re: PIC programming adapter problem

Reply #19
I have some clean-ups comming

It would be nice if someone could add pic16 protocol code :)

Re: PIC programming adapter problem

Reply #20
Am I reading pic18.c correct , and writing configuration bits for 18Fxxx is not supported yet?

Re: PIC programming adapter problem

Reply #21
[quote author="robots"]It would be nice if someone could add pic16 protocol code :)[/quote]
Ooops. I assumed that we have that. I had a 16f and a 12f in mind for adding the list. I can start looking at that once I finish all the stuff I had to postpone because of Make. And yes, that includes university stuff too. :S

[quote author="udif"]Am I reading pic18.c correct , and writing configuration bits for 18Fxxx is not supported yet?[/quote]
I was able to flash a PIC18f without any problems but that I have to check. Config bits should be there, that was the problem with the major haul. Are you looking the pic18.c file inside software folder? The ones inside software folder are the ones most up to date.

Re: PIC programming adapter problem

Reply #22
[quote author="tayken"][quote author="udif"]Am I reading pic18.c correct , and writing configuration bits for 18Fxxx is not supported yet?[/quote]
I was able to flash a PIC18f without any problems but that I have to check. Config bits should be there, that was the problem with the major haul. Are you looking the pic18.c file inside software folder? The ones inside software folder are the ones most up to date.[/quote]

Yes, I was looking into the one under 'software'.
I'm basing my conclusions on http://http://ww1.microchip.com/downloads/en/DeviceDoc/39622L.pdf

The reading part is OK, since reading configuration registers is identical to reading flash memory.
Quoting section 4.1 from the document above:
Quote
This technique will work to read any memory in the 000000h to 3FFFFFh address space, so it also applies to the reading of the ID and Configuration registers.

The writing part in pic.c treats flash memory the same way as configuration bits:
Code: [Select]
int PIC_WriteMemory(struct picprog_t *p, struct memory_t *mem)
{
        struct pic_chip_t *pic = PIC_GetChip(p->chip_idx);
        struct pic_family_t *fam = PIC_GetFamily(pic->family);
        struct proto_ops_t *proto = PIC_GetProtoOps(p->chip_idx);

        struct mem_page_t * page;

        proto->EnterICSP(p, fam->icsp_type);

        page = MEM_GetFirstPage(mem);
        while (page != NULL) {
                printf("Writing page %04lx... n", (unsigned long)page->base);

                // TODO: check address against pic memory map! EEPROM and Fuses might need separate handling
                proto->Write(p, page->base, page->data, page->size);
                page = MEM_GetNextPage(page);
        }

        proto->ExitICSP(p, fam->icsp_type);

}
As you can see, this code doesn'r care if the current page is flash memory or configuration registers.
However, configuration memory is written differently than flash memory.
Flash memory writing procedure is documented in section 3.2 of the spec above, while configuration register writing procedure is described in section 3.6
The differences are minor (a slightly different setup for the config registers, and each byte is written separately).
I plan adding it this week, since I have a blank PIC, and the bus pirate is my only PIC programmer.

There are two lines in the current code (pic18.c) that simply doesn't appear in the 2550 programming document (Table 3.5):
Lines 201-202:
Code: [Select]
        iface->PIC416Write(0x00, 0x6AA6);
        iface->PIC416Write(0x00, 0x88A6);
Line180:
Code: [Select]
        iface->PIC416Write(0x00, 0x84A6); //enable page writes

Are these really required? perhaps they were left here out of trial and error and never removed?

Re: PIC programming adapter problem

Reply #23
[quote author="udif"]
As you can see, this code doesn'r care if the current page is flash memory or configuration registers.
However, configuration memory is written differently than flash memory.

... snip ...

Are these really required? perhaps they were left here out of trial and error and never removed?[/quote]

Doesn't care is not the correct phasing. Not implemented is better :). Also the protocol methods are not prepared for fuses write. I will have expand the protocol structure.
I also have another changes in mind, that should ease maintenance of the application

I believe that Ian was the one who came up with the pic18 programming routines. I just copied/pasted them into this software.

Re: PIC programming adapter problem

Reply #24
I have just committed bunch of changed to the PiratePicProg repository. Mainly cleanups, and reorganization of the programming protocols.

I think the programming protocols should be extended with ReadFuse, WriteFuse and ReadEEPROM, WriteEEPROM functions.
Problem is with devices that share same space for flash and fuses. In that case solution number 2 should be used: Handle fuses and EEPROM in existing Read/Write functions.

Any suggestions?

Re: PIC programming adapter problem

Reply #25
I took a look at your latest commit.

I'm all for what you refer to as solution 2.
I don't think you should add special functions for EEPROM and flash. I think that the main code should keep calling the generic Read/Write functions, and let the device/family-specific code handle the different memory types.
After all, as far as the HEX file is concerned, configuration writes is simply data at 0x300000. There is no reason to expose all the memory types at the generic layers.

For example, the 18F read/Write code should detect that writes to 0x300000 are configuration writes and treat them as such.

Since the current API calls for transferring a pointer and a length for the memory to be read/written, then either the family/device-specific code needs to handle that (i.e. if you get a 32 byte write request to configuration space, you need to break it into byte writes yourself), or the Read/Write function will return the actual number of bytes read/written , and the calling code will need to adjust.

You can have separate read/write routines for EEPROM/flash/config/whatever, but I think they should be kept static, and be called from the main read/write functions according to the tblptr.

Re: PIC programming adapter problem

Reply #26
Here is my first take at writing configuration bits for the 18F2553 (And probably other 18Fxxx devices).
It is still not working 100%, but I want to give it an early review by others, who may spot the remaining bugs.

At the moment it seems to write only SOME of the configuration words, or perhaps some of these are write only and can't be read back (I haven't read the whole configuration registers descriptions yet). The first two bytes and the last 6 always seems to be OK, but the middle 6 always seems to be partially corrupted (only 1-2 bytes are OK, and one of the values is in the wrong address).
My test file was the diolan bootrom from the USB IR Toy repository, which I hand-edited to use a 8MHz xtal instead of the original 20Mhz (0x300000: 0x24 -> 0x21, and fixed the checksum).

I also removed some unused functions from pic24.c and pic16.c (ReadFlash and WriteFlash) because I removed those fields from proto_ops_t. (Dead code can always be resurrected from version control - that's what its there for).

I will probably end hooking up my Open Logic Sniffer to see what's really coming out of the Bus Pirate (now we only need to code an ICSP mode to the OLS software).

comments are welcomed!

Re: PIC programming adapter problem

Reply #27
I have quickly looked at the patch, and there are quite few problems:

1. Static functions are static for a reason, and not to be exposed in *.h files.

2. Write_mem_table and read_mem_table use the same type, Why not have them typedef-ed ?  Why do you have array of function pointers? (functions are pointers), you would have no & in the array and calling function from array would look like write_mem(xx) instead of (*write_mem)(xxx).

3. Functions PIC18_Write and PIC18_Read are too generic. I think we should go with my proposal 1. Have this generic function in pic.c and call different PICxx_writeEeprom from the selected protocol.

4. There is something wrong with the for-cycle in Write_fuse. 3/4 of the code are the same. :-)

These are of course non-functional bugs, just to keep the program in maintainable state.

Re: PIC programming adapter problem

Reply #28
[quote author="robots"]I have quickly looked at the patch, and there are quite few problems:

1. Static functions are static for a reason, and not to be exposed in *.h files.
[/quote]
You're right ofcourse. I moved those declarations to the beginning of pic18.c but that was on my laptop, and I forgot to sync it to my main machine.

[quote author="robots"]
2. Write_mem_table and read_mem_table use the same type, Why not have them typedef-ed ?  Why do you have array of function pointers? (functions are pointers), you would have no & in the array and calling function from array would look like write_mem(xx) instead of (*write_mem)(xxx).
[/quote]
typedefs are a good idea, I'll fix that later today.
As for arrays of functions - As far as I know, you can't have an array of functions in C, just array of function pointers.
http://c-faq.com/~scs/cclass/krnotes/sx8f.html
Quote
(The only possibilities that C doesn't support are functions returning arrays, and arrays of functions, and functions returning functions.)
I even tried that with GCC. The following code works:
Code: [Select]
#include "stdio.h"

void a(void) { printf("an");};
void b(void) { printf("bn");};
void c(void) { printf("cn");};
void d(void) { printf("dn");};

void (*arr[])(void) = {&a, &b, &c, &d};

int main(int argc, char *argv[])
{
int i;

  for (i = 0; i < sizeof(arr)/sizeof(arr[0]); i++) {
    (*arr[i])();
  }
  return 0;
}
The code below, however, doesn't work, producing the error:
Quote
b.c:8: error: declaration of 'arr' as array of functions
Code: [Select]
#include "stdio.h"

void a(void) { printf("an");};
void b(void) { printf("bn");};
void c(void) { printf("cn");};
void d(void) { printf("dn");};

void (arr[])(void) = {a, b, c, d};

int main(int argc, char *argv[])
{
int i;

  for (i = 0; i < sizeof(arr)/sizeof(arr[0]); i++) {
    (arr[i])();
  }
  return 0;
}
[quote author="robots"]3. Functions PIC18_Write and PIC18_Read are too generic. I think we should go with my proposal 1. Have this generic function in pic.c and call different PICxx_writeEeprom from the selected protocol.
[/quote]
I think this is a matter of personal style. The function in pic.c is iterating over the linked list of memory elements, calling the family-specific write function, which in turn decodes the memory address to decide what type of memory it is, and call the corresponding write function within that family. This was you isolate the fact that PIC18 has 3 different types of memory writes from the main program.
[quote author="robots"]
4. There is something wrong with the for-cycle in Write_fuse. 3/4 of the code are the same. :-)
[/quote]I assume you are bothered by the main loop in PIC18_Write_Fuse. Do you like this loop better?
Code: [Select]
	for(ctr = 0; ctr < length; ctr ++) {
if ((ctr & 1) == 0)
DataByte = (((uint8_t *)Data)[ctr + 1] << 8) + ((uint8_t *)Data)[ctr];
iface->PIC416Write(0x00, 0x0E00 | ctr);
iface->PIC416Write(0x00, 0x6EF6);
iface->PIC416Write(0x0F, DataByte);
iface->PIC416Write(0x40, 0x0000); // see PIC18_Write_Flash below
iface->flush();
}
[quote author="robots"]
These are of course non-functional bugs, just to keep the program in maintainable state.[/quote]
Generally speaking, when I contribute code to an existing project, I adapt to that project's coding style. Some of the choices I made were clearly no my personal coding style (The 3 line DataByte calculation, uppercase in variables, etc.).
Personally, for example, I would have avoided gcc extensions such as this:
Quote
   char tmp[21] = { [0 ... 20] = 0x00 };

Re: PIC programming adapter problem

Reply #29
2.
I was referring to something like this ....
Code: [Select]
#include "stdio.h"

typedef void (*fnc)(void);

void a(void) { printf("an");};
void b(void) { printf("bn");};
void c(void) { printf("cn");};
void d(void) { printf("dn");};

fnc func[] = {a,b,c,d};

int main(int argc, char *argv[])
{
int i;

  for (i = 0; i < sizeof(func)/sizeof(func[0]); i++) {
func[i]();
}
return 0;
}

3.
You are probably right about this.

4.
Designated inits are great addition to GCC, and I hope some day VC compiler will have them as well. These are used in the whole program (see pic_chip structure, etc). More here: http://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

Adaptation is good and required. But copy/pasting (even existing) code, without thinking about is not. Uppercase variables are not my choice either and were added when I didn't look:-). Feel free to change them to lower case. I try to follow the "kernel coding style", but I fail at the function naming. Lot of my functions have mixed-case names.

I like the new cycle better, it doesn't duplicate the internal code. (but i doubt it correctness, i need to lookup the manual :-) )

Same could probably go for "setup" function that prepares Flash write. FUSE write setup (almost identical thing) is not in a separate function. How about merging these two setups into the same function?