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.

Sign up for GitHub

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

Jump to bottom

Only show one 'Loading' and one 'Verifying' progress bar when loading #130

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

Only show one 'Loading' and one 'Verifying' progress bar when loading #130

wants to merge 1 commit into from

Conversation

DavidEGrayson
Copy link
Contributor

This makes the output much shorter and more useful when loading a UF2 file that writes to many different memory ranges, which can happen if it contains a filesystem. Also, this PR removes the "OK" message that was printed after verification: if the verification finished and there is no error message, that means everything was OK.

This is similar to what I did in my older pull request #66 from 18 months ago for picotool version 1, but that pull request had a bigger scope and was never merged in.

If you want to test this, here is a simple Ruby script I used to generate a fragmented UF2 file for a Pico 2:

#!/usr/bin/env ruby
# coding: ASCII-8BIT

output_filename = ARGV.fetch(0)

UF2_FLAG_FAMILY_ID_PRESENT = 0x2000
RP2040_FAMILY_ID = 0xe48bff56
RP2350_ARM_S_FAMILY_ID = 0xe48bff59

FRAGMENT_COUNT = 32
FRAGMENT_SIZE = 4096

block_map = {}
FRAGMENT_COUNT.times do |i|
  (FRAGMENT_SIZE/256).times do |j|
    block_map[i * FRAGMENT_SIZE * 2 + j * 256] = "\xAA" * 256
  end
end

File.open(output_filename, 'wb') do |output|
  block_map.keys.sort.each_with_index do |offset, block_number|
    address = offset + 0x1000_0000
    block = block_map.fetch(offset)
    uf2_block = "UF2\n\x57\x51\x5D\x9E" +
      [UF2_FLAG_FAMILY_ID_PRESENT, address, block.size,
      block_number, block_map.size, RP2350_ARM_S_FAMILY_ID].pack('<LLLLLL') +
      block.ljust(476, "\x00") + "\x30\x6F\xB1\x0A"
    output.write uf2_block
  end
end

Picotool output before this pull request:

$ ./picotool.exe load -v ../fragment.uf2
Family ID 'rp2350-arm-s' can be downloaded in absolute space:
  00000000->02000000
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Loading into Flash: [==============================]  100%
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK
Verifying Flash:    [==============================]  100%
  OK

Picotool output after this pull request:

$ ./picotool.exe load -v ../fragment.uf2
Family ID 'rp2350-arm-s' can be downloaded in absolute space:
  00000000->02000000
Loading:   [==============================]  100%
Verifying: [==============================]  100%

@DavidEGrayson
Only show one 'Loading' and one 'Verifying' progress bar when loading.
f1b47b1 
This makes the output much shorter and more useful when
loading a UF2 file that writes to many different memory ranges,
which can happen if it contains a filesystem.

Also, don't bother printing "OK": if the verification finished
that means everything was OK.
lurch
lurch reviewed Sep 2, 2024
View reviewed changes
main.cpp
Comment on lines +4260 to +4261
for (auto r : ranges) { total_bytes += r.to - r.from; }
{
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth keeping the // new scope for progress bar comment, oherwise it might look like the { on the second line is associated with the for on the previous line 😉
Or alternatively, as there's now not multiple progress bars, perhaps this additional scope is no longer necessary? 🤔

lurch
lurch reviewed Sep 2, 2024
View reviewed changes
main.cpp Show resolved Hide resolved
will-v-pi
will-v-pi requested changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@will-v-pi will-v-pi left a comment

Choose a reason for hiding this comment

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

I'm happy with the general idea of this change, but I would want to keep the "OK" printout after verifying for clarity, and keep the "Loading into xxx" printout to specify whether it's loading into flash or SRAM. Therefore it will still need to split the memory ranges into different memory types, with a separate loading bar for each memory type - possibly by having get_coalesced_ranges return a vector<vector<range>> with a separate vector<range> for each memory type?

These changes should also be made to the verify command and the verify section of the save command, as those have similar progress bars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lurch lurch lurch left review comments

@will-v-pi will-v-pi will-v-pi requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DavidEGrayson @lurch @will-v-pi