Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dap_vendor: Buffer MSD_Write packets into 512-byte blocks. #1093

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

microbit-carlos
Copy link
Contributor

@microbit-carlos microbit-carlos commented Feb 3, 2025

Currently flashing a Universal Hex in "segments" or "blocks" format via WebUSB does not work.

This PR sends to the file streamer blocks of the same size as the VFS sector size, which is what vfs_manager does and some stream formats expect, like Universal Hex and in the future UF2.

This could have been implemented inside file_stream instead, but this way it mirrors what the MSC side is doing, buffering the 64 byte packets into 512-byte sectors before passing it to vfs_manager, which then sends it to file_stream.

One constrain is that DAPLink doesn't know when the last DAP.js ID_DAP_MSD_Write packet is sent, so we need to flush whatever is left in the buffer on ID_DAP_MSD_Close. So the "close" response could contain an error writing the last buffer.

There is also a memory hit, as the buffer takes 512 bytes of RAM. We could reuse the usb_buffer from vfs_manager.c (which is passed to usbd_msc.c, so that's where the 64-byte MSC packets are buffer into VFS sectors), not if you prefer to keep them separated. To be fair, using WebUSB flashing + MSD drag&drop at the same time will fail anyway, so probably wouldn't need to worry much about having two different components accessing the same buffer.

Comment on lines 49 to 53
// TODO: It might be more memory efficient to re-use the usb_buffer from vfs_manager.c
static uint8_t __ALIGNED(4) file_stream_buffer[VFS_SECTOR_SIZE];
static uint16_t file_stream_buffer_pos = 0;
Copy link
Contributor Author

@microbit-carlos microbit-carlos Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in the PR, this is the buffer that could be removed and usb_buffer from vfs_manager.c used instead.

static uint32_t usb_buffer[VFS_SECTOR_SIZE / sizeof(uint32_t)];

extern U8 *USBD_MSC_BlockBuf;

static void build_filesystem()
{
// Update anything that could have changed file system state
file_transfer_state = default_transfer_state;
vfs_user_build_filesystem();
vfs_set_file_change_callback(file_change_handler);
// Set mass storage parameters
USBD_MSC_MemorySize = vfs_get_total_size();
USBD_MSC_BlockSize = VFS_SECTOR_SIZE;
USBD_MSC_BlockGroup = 1;
USBD_MSC_BlockCount = USBD_MSC_MemorySize / USBD_MSC_BlockSize;
USBD_MSC_BlockBuf = (uint8_t *)usb_buffer;
}

Left it as a TODO comment to come to an agreement before merging the PR.

@microbit-carlos microbit-carlos marked this pull request as draft February 3, 2025 11:44
To send to the file streamer blocks of size matching the VFS sector
size, which is expected for some stream formats like Universal Hex
Blocks.
@microbit-carlos
Copy link
Contributor Author

microbit-carlos commented Feb 14, 2025

When combining this PR + out of order PR + UF2, it fails building some ports due to insufficient .bss so I had to reuse the usb_buffer from vfs_manager.c in the end to save 512 bytes.

Force pushed, diff can be seen in:

@microbit-carlos microbit-carlos marked this pull request as ready for review February 14, 2025 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant