Fix #175: use PIC specific replacement values for comparison of missing values - [merged] #405

Closed DavidGriffith closed 11 months ago

In GitLab by @michael.dreher42 on Jul 26, 2020, 14:47

Merges verify -> master

Only the verify of PIC micro controllers should be affected. For testing use files who's size is smaller or larger than the chip memory

@radiomanV: please review is_pic() and get_pic_word_width()

In GitLab by @radiomanV on Jul 27, 2020, 03:36

please review is_pic() and get_pic_word_width()

These are ok. But the issue #175 is still present. For example PIC16F84A:

minipro -d pic16f84@dip18 
Found TL866II+ 04.2.116 (0x274)
Name: PIC16F84@DIP18
Memory: 1024 Words + 64 Bytes
Package: DIP18
ICSP: ICP002.JPG
Protocol: 0x18
Read buffer size: 128 Bytes
Write buffer size: 32 Bytes

This controller has two address spaces: the code memory (2048 bytes) and the data section (64 bytes). So far so good.
If i issue a blank check:

minipro -p pic16f84@dip18 -b
Found TL866II+ 04.2.116 (0x274)
Reading Code...  0.07Sec  OK
Verification failed at address 0x0000: File=0xFFFF, Device=0x3FFF
Reading Data...  0.01Sec  OK
Data memory section is blank.

Not good. Lets write some random bytes to the code section:

head -c 2048 </dev/urandom |./minipro -p pic16f84@dip18 -w - -c code
Found TL866II+ 04.2.116 (0x274)
Erasing... 0.25Sec OK
Writing Code...  20.60Sec  OK
Reading Code...  0.07Sec  OK
Verification failed at address 0x0000: File=0x632F, Device=0x232F

So we have a problem with the source data mask:

int compare_word_memory(uint16_t replacement_value,
      uint8_t little_endian,
      uint8_t *s1, uint8_t *s2, size_t size1, size_t size2,
      uint16_t *c1, uint16_t *c2)

if the replacement_value is a mask then both values (file and device) should be masked before compare:

    if (v1 != v2) {
      *c1 = v1;
      *c2 = v2;
      return i;
    }
//should be:
    v1 &= replacement_value;
    v2 &= replacement_value;
    if (v1 != v2) {
      *c1 = v1;
      *c2 = v2;
      return i;
    }

This should fix all issues when blank value differs.

Also apart from PIC code section memory all others sections should not be tested in compare_word_memory.
Lets fill the eeprom (DATA) section of our PIC16F84A:

head -c 64 </dev/urandom |./minipro -p pic16f84@dip18 -w- -c data
Found TL866II+ 04.2.116 (0x274)
Erasing... 0.25Sec OK
Writing Data...  1.29Sec  OK
Reading Data...  0.01Sec  OK
Verification OK

Verification OK is not ok because we use compare_word_memory in DATA section when the correct compare should be 8 bit compare_memory function. So in write_page_file and verify_page_file this:

uint16_t compare_mask = get_compare_mask(handle);
//should be
uint16_t compare_mask = type == MP_CODE ?  get_compare_mask(handle) : 0;

This way only PIC code memory section is compared in compare_word_memory if necessary.

In GitLab by @michael.dreher42 on Jul 27, 2020, 04:21

if the replacement_value is a mask then both values (file and device) should be masked before compare

I had that before, but I think it is not a good idea. When the file has a 0xff but the chip can only write a 0x3f, then an error should be raised. The file is then just wrong and the verify should fail.

The replacement value is only used for bytes which are missing.

uint16_t compare_mask = type == MP_CODE ? get_compare_mask(handle) : 0;

Thanks, that shall fix the problem with the data memory, I put that in get_compare_mask(). Not sure if it fixed all mentioned problems.

In GitLab by @radiomanV on Jul 27, 2020, 06:01

The file is then just wrong and the verify should fail.

Then the issue #175 never was an issue.
The hex or binary file can be generated or dumped by another tool which put FF as blank value. In our particular case when the PIC word length is 14 what should we do with bit 14 and 15? Which are their default value? 0 or 1? what we should compare? 3FFF with FFFF?

We cannot force a blank value of 3FFF with bit 15=0 and 14=0 or 1 or anything because those bits doesn't exists.
We don't have an integer data type of 14 bit and then we use a standard 16 bit one with D0-D13 being valid data and D14-D15 bits don't care.

If we set those bits to zero(3F) then we must force the source data bit 15 and 14 to zero. We can set them to 1 in both source and chip buffer for the same reason.
We must give a default value on both source and chip data buffer for the unused bits before compare or compare at the bit level but this is lame and time consuming.

In GitLab by @michael.dreher42 on Jul 27, 2020, 12:21

@radiomanV: ok, you convinced me :-)

I only tested with binary files which are shorter than the chip memory (with '-s'), there only the replacement values at the end are needed.

For Intel Hex format, values can be missing in the middle and those are filled with memset(0xff) so those need also to be considered. The easiest solution is the masked comparison like your suggestion, filling the buffer with 14-bit replacement values could be another option.

When files contain bits which cannot be written (e.g. 14 and 15) those are now silently ignored.

In GitLab by @radiomanV on Jul 27, 2020, 14:20

My intention was never to convince you but to show you my point :) A hex file with blank values of FFFF is not a wrong file. If we consider this a wrong file then the 3FFF can be considered wrong too and we should reject it at the load stage and don't even bother to write anything. But this is a shoot in the foot :)

Also another issue is the configuration bytes verify. Some tools set the unused configuration bits to 1 while other tools set them to 0. But this is more complicated because we need the configuration bits mask for every supported microcontroller (only Avr and Pic are supported at this moment).

Also Hex files generated by the Microchip compilers contains the configuration bytes or eeprom data in the same hex but at different address spaces (like 0x30000 or 0x2007 or so).

Initially when such type of hex file was feeded to my hex parser the parse action was ok, but failed at the write stage. This was the issue #159. I just disabled this behavior in the hex parser. The default behavior was to read the whole hex file and to copy upto chip size buffer. If we have more data over the chip size we only update the size variable and return this new size to the caller. This was disabled in !153.

I still want to introduce what i said in #159. For the moment we have the blank value for all supported PIC controllers. But we should introduce new info for these controllers: the config bytes address (for Microchip), the config bytes mask (for both Atmel and Microchip) and the addresses for eeprom and user data sections if these are available for that chip.
Where should we introduce those new informations?
My first guess is the device_t structure.

In GitLab by @michael.dreher42 on Jul 27, 2020, 22:26

Also another issue is the configuration bytes verify. Some tools set the unused configuration bits to 1 while other tools set them to 0

From a standpoint of 'fuse' an unused fuse shall always be left in an untouched state and this means in the '1' state which is also the value you will read back. This is how you normally handle reserved values which makes it easier for extensions. When tools use '0' as default, they didn't understand this strategy and should be considered broken.

Where should we introduce those new informations?

When introducing new information in device_t we need to clarify a merge strategy that means how to update device_t from the DLL while keeping the locally added information. This means it should be kept in another file and not directly edited in the header infoic*devices.h. The extract tool can then merge the two sources.

Does Xgpro have the information? If yes, is it probably already contained in device_t, maybe indirectly like the PIC instruction width or as part of the "config" information. Rules which extract data from existing device_t data can be maintained much easier for merging than extending device_t with new fields.

the config bytes mask

When you look at https://www.engbedded.com/fusecalc/ for ATmega328P in the "Manual fuse bits configuration" section they set the 5 unused bits of the "Extended" fuse to 1.

I remember that avrdude also had the problem of not knowing the config byte mask and gave an error for unused bits which are set to 0. In avrdude.conf the following info can be found:

part
    id                  = "m328";
    desc                = "ATmega328";
    memory "efuse"
        size = 1;
        read = "0 1 0 1 0 0 0 0 0 0 0 0 1 0 0 0",
               "x x x x x x x x o o o o o o o o";

        write = "1 0 1 0 1 1 0 0 1 0 1 0 0 1 0 0",
                "x x x x x x x x x x x x x i i i";
    ;

The 'write' command contains information about the "config byte mask" as "iii", so that could be used as a source.

In GitLab by @radiomanV on Jul 28, 2020, 24:54

From a standpoint of 'fuse' an unused fuse shall always be left in an untouched state and this means in the '1' state which is also the value you will read back.

Exactly. This is the way a fuse should be. An untouched fuse =1, blown fuse =0. There was always a confusion especially in GUI tools when a checked fuse was considered '1' (ponyprog and other ancient tools).

Does Xgpro have the information?

Yes. But you have to dig deep in the debugger to extract this information. I managed to extract some binary structures like pin mask for bad pin contact but the amount of work to find the exact offset is discouraging.
Look at this:

{
    .name = "ATMEGA8@DIP28",
    .protocol_id = 0x1d,
    .variant = 0x43,
    .read_buffer_size =  0x100,
    .write_buffer_size = 0x40,
    .code_memory_size = 0x2000,
    .data_memory_size = 0x200,
    .data_memory2_size = 0x00,
    .chip_id = 0x1e9307,
    .chip_id_bytes_count = 0x03,
    .opts1 = 0x04,
    .opts2 = 0x0040,
    .opts3 = 0x0001,
    .opts4 = 0x182630,
    .opts5 = 0x0400,
    .opts6 = 0x0000,
    .opts7 = 0x0081,
    .opts8 = 0x0c25,
    .package_details = 0x1c000700,
    .config = avr2_fuses
}

This member .config = avr2_fuses gives us this:

fuse_decl_t avr2_fuses[] = {
    {.num_fuses = 2,
     .num_locks = 1,
     .num_uids = 0,
     .item_size = 1,
     .word = 0,
     .erase_num_fuses = 2,
     .rev_mask = 0,
     .fnames = (const char *[]){"fuses_lo", "fuses_hi"},
     .unames = NULL,
     .lnames = (const char *[]){"lock_byte"}}};

Introducing new members in device_t structure is not what i would like. But we can change the config member to point to a much valuable structure like .config = atmega8_conf. Then we can put all information in a new file say atmel.h for Atmel controllers, microchip.h for Microchip controllers pld.h for GAL and ATF devices. This way we can add specific informations for a particular chip; for ex. .rev_mask = 0 and .num_uids = 0 are not avr specific members.
This way we can remove those generic data structures. We declare a specific data structure at the top of these files and cast the .config member to whatever device type we need. Something like this:

//atmel.h
    .name = "atmega8_conf",
    .num_fuses = 2,
    .num_locks = 1,
    //....................
    .fuse_mask=xxxx;
   //other device specific members
}

The erase/write/read protocol is still generic for all supported chips and if i remember correctly there are some issues because of this. But if we have specific data structure for each type of chip we can write a specific code. Look at tl866iiplus_erase in tl866iiplus.c. Not so good.

DavidGriffith / minipro-import-test

Fix #175: use PIC specific replacement values for comparison of missing values - [merged] #405