ARMmbed / DAPLink

https://daplink.io
Apache License 2.0
2.34k stars 979 forks source link

possible issue for Drag and Drop feature on Mac OS #745

Open oxanagig opened 3 years ago

oxanagig commented 3 years ago

The drag and drop code logic seems to rely on that the bin/hex file is transferred to contiguous sectors.

static void file_data_handler(uint32_t sector, const uint8_t *buf, uint32_t num_of_sectors) { stream_type_t stream; uint32_t size;

// this is the key for starting a file write - we dont care what file types are sent
//  just look for something unique (NVIC table, hex, srec, etc) until root dir is updated
if (!file_transfer_state.stream_started) {
    // look for file types we can program
    stream = stream_start_identify((uint8_t *)buf, VFS_SECTOR_SIZE * num_of_sectors);

    if (STREAM_TYPE_NONE != stream) {
        transfer_stream_open(stream, sector);
    }
}

if (file_transfer_state.stream_started) {
    // Ignore sectors coming before this file
    if (sector < file_transfer_state.start_sector) {
        return;
    }

    // sectors must be in order
    if (**sector != file_transfer_state.file_next_sector**) {
        vfs_mngr_printf("vfs_manager file_data_handler sector=%i\r\n", sector);

        if (sector < file_transfer_state.file_next_sector) {
            vfs_mngr_printf("    sector out of order! lowest ooo = %i\r\n",
                            file_transfer_state.last_ooo_sector);

            if (VFS_INVALID_SECTOR == file_transfer_state.last_ooo_sector) {
                file_transfer_state.last_ooo_sector = sector;
            }

            file_transfer_state.last_ooo_sector =
                MIN(file_transfer_state.last_ooo_sector, sector);
        } else {
            vfs_mngr_printf("    sector not part of file transfer\r\n");
        }

        vfs_mngr_printf("    discarding data - size transferred=0x%x, data=%x,%x,%x,%x,...\r\n",
                        file_transfer_state.size_transferred, buf[0], buf[1], buf[2], buf[3]);
        return;
    }

    // This sector could be part of the file so record it
    size = VFS_SECTOR_SIZE * num_of_sectors;
    file_transfer_state.size_transferred += size;
    file_transfer_state.file_next_sector = sector + num_of_sectors;

    // If stream processing is done then discard the data
    if (file_transfer_state.stream_finished) {
        vfs_mngr_printf("vfs_manager file_data_handler\r\n    sector=%i, size=%i\r\n", sector, size);
        vfs_mngr_printf("    discarding data - size transferred=0x%x, data=%x,%x,%x,%x,...\r\n",
                        file_transfer_state.size_transferred, buf[0], buf[1], buf[2], buf[3]);
        transfer_update_state(ERROR_SUCCESS);
        return;
    }

    transfer_stream_data(sector, buf, size);
}

}

I found that this is true for Windows, but on Mac, the OS can put one file in non-contiguous sectors, and this will cause trouble for the logic implemented above.

Any thought on this?

0xc0170 commented 3 years ago

Have you been able to reproduce this and hit an error? We have been testing on Mac in our CI as well and many of us are actually using Mac.

oxanagig commented 3 years ago

To be honest, the reason I ask the question is that I am developing the drag-and-drop feature for my own project, and encountered the issue, then I stumble on this project and want to understand how it is implemented.

I am drag and drop a 36Kbytes bin file on Mac OS 10.15, this is what happened

Write Block: 27, size:8 // Mac OS related stuff Write Block: 1, size:1 // FAT area Write Block: 2, size:1 // Directory Entry area Write Block: 27, size:8 // Mac OS related stuff Write Block: 2, size:1 // FAT area Write Block: 35, size:64 // Write first 32kbytes of the bin file Write Block: 1, size:1 // FAT area Write Block: 27, size:8 // Mac OS related stuff Write Block: 2, size:1 // Directory Entry area Write Block: 27, size:8 // Mac OS related stuff Write Block: 2, size:1 // Directory Entry area Write Block: 27, size:8 // Mac OS related stuff Write Block: 99, size:8 // Write the last 4kbytes of the bin file Read Block: 35, size:64 // read back the first 32kbytes Write Block: 2, size:1 // Directory Entry area Read Block: 99, size:8 // read back the last 4K bytes

The same action on Windows 10: Write Block: 2, size:1 // Directory Entry area Write Block: 1, size:1 // FAT area Write Block: 2, size:1 // Directory Entry area Write Block: 1, size:1 // FAT area Write Block: 2, size:1 // Directory Entry area Write Block: 1, size:1 // FAT area Write Block: 35, size:72 // write the 36k bytes Write Block: 2, size:1 // Directory Entry area Write Block: 2, size:1 // FAT area

flit commented 3 years ago

It looks like the file is still written in order even if the blocks are non-contiguous. Unfortunately I'm not knowledgable about the virtual FS code in DAPLink (it was written by someone no longer working on the project) to know how it handles this, but I'm curious enough to look into it once I get a chance.

Btw, concerns about this kind of issue and, worse, out of order writes are what prompted Microsoft to define the UF2 format. Adding UF2 support to DAPLink would be nice, but it's not particularly high priority as very few development tools support it.

oxanagig commented 3 years ago

@flit Yes, the file is still written in order, and this is completely compliant with the FAT file system, as the FAT is a linked list so that the storage doesn't need to be in contiguous sectors/blocks.

image

In theory, the can be solved by checking the FAT block as the OS seems to always update the FAT before writing to the data sectors.


The UF2 seems to be well designed for MSD bootloader, as each block(512 bytes) has meta-data to identify the data.

oxanagig commented 3 years ago

I just finished reading the UF2 format, it stated in the blog:https://makecode.com/blog/one-chip-to-flash-them-all

"When the OS writes a HEX file, DAPLink needs to discard writes to the FAT or directory table, as well as writes of the meta-data files. It may need to deal with out-of-order writes. All these details mean that DAPLink is quite complex and also sometimes needs to be updated when a new OS release changes the way in which it handles FAT. This also is the reason that some MSC bootloaders for various chips only support given operating systems under some specific conditions." 😂