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

Add support for more uploading options #126

Closed
wants to merge 28 commits into from

Conversation

Skyluker4
Copy link

@Skyluker4 Skyluker4 commented Mar 26, 2020

Summary:

This revision allows for uploading custom binary files, which did not work before. It also adds more options for uploading and managing files on the V5. More .ini options are now customizable with the cli.

Motivation:

I wanted to be able to upload binary files which I could not before. I also wanted to change more options in the .ini files such as the IDE used.

Test Plan:

  • Upload a binary not in a project to the V5 with prosv5 upload [file]
  • Verify automated .ini generation by uploading program to V5 and reading to confirm it is correct
  • Change metadata options when uploading. For example, prosv5 upload --name TestName --description 'test program' --icon USER921x.bmp --ide VEXcode --ide-version 3.1.5 --program-version 1.1.0 --date [insert ISO date] --timezone '-5' --no-execute and make sure they all go to the V5 correctly
  • Upload a custom .ini file with prosv5 upload --ini-file [ini file]
  • Read a file with prosv5 v5 read-file [file]
  • Delete entire program with prosv5 v5 rm-program [port]

Copy link
Member

@edjubuh edjubuh left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the contribution! Have a few nits but overall looks good.



@v5.command(hidden=True)
Copy link
Member

Choose a reason for hiding this comment

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

I'll let @HotelCalifornia make call about un-hiding mut and friends but we had kept read_file and write_file hidden because we feel many users would expect to be able to read files written here inside their user programs.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I agree that read/write_file should stay hidden. Could go either way on mut and friends

pros/cli/v5_utils.py Outdated Show resolved Hide resolved
pros/cli/v5_utils.py Outdated Show resolved Hide resolved
Comment on lines +534 to +549
if silent:
for i in range(0, file_len, max_packet_size):
packet_size = max_packet_size
if i + max_packet_size > file_len:
packet_size = file_len - i
file.write(self.ft_read(addr + i, packet_size))
progress.update(packet_size)
logger(__name__).debug('Completed {} of {} bytes'.format(i + packet_size, file_len))
else:
with ui.progressbar(length=file_len, label='Downloading {}'.format(remote_file)) as progress:
for i in range(0, file_len, max_packet_size):
packet_size = max_packet_size
if i + max_packet_size > file_len:
packet_size = file_len - i
file.write(self.ft_read(addr + i, packet_size))
progress.update(packet_size)
logger(__name__).debug('Completed {} of {} bytes'.format(i + packet_size, file_len))
Copy link
Member

Choose a reason for hiding this comment

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

Is there a better way to get silent operation without needing to duplicate this logic? Perhaps we can propagate silent into pros.common.ui.progressbar or something similar.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think making the progressbar silent would be the way to go since that is the entire point of the progress bar. Maybe just put the code inside the for loop (minus the progress.update part) in a separate function and call it every time in with the for loop? That way there's no major change to the progress bar code and gets rid of code reuse and then progessbar.update can be called in the one that uses the progressbar.

Copy link
Contributor

Choose a reason for hiding this comment

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

something like this would probably work. does the progressbar show up if you never update it? I can't remember

# e.g.: self._write_file_packets(file, max_packet_size, progress.update)
def _write_file_packets(file, max_packet_size, progress_cb=None):
    file_len = len(file)
    for i in range(0, file_len, max_packet_size):
        packet_size = max_packet_size
        if i + max_packet_size > file_len:
            packet_size = file_len - i
        file.write(self.ft_read(addr + i, packet_size))
        if progress_cb is not None:
            progress_cb(packet_size)
        logger(__name__).debug(f'Completed {i + packet_size} of {file_len} bytes')

Copy link
Author

@Skyluker4 Skyluker4 Jun 28, 2020

Choose a reason for hiding this comment

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

The progressbar still shows up if you never update it - just stays at 0% -, but doesn't show if you just keep the with ui.progressbar(...) out of it. The function you made works with a few modifications: file_len has to be passed in since it's grabbed from the file metadata and addr needs to be a parameter as well.
This worked for me:

def _write_file_packets(self, file, file_len, max_packet_size, addr, progress_cb=None):
        for i in range(0, file_len, max_packet_size):
            packet_size = max_packet_size
            if i + max_packet_size > file_len:
                packet_size = file_len - i
            file.write(self.ft_read(addr + i, packet_size))
            if progress_cb is not None:
                progress_cb(packet_size)
            logger(__name__).debug(f'Completed {i + packet_size} of {file_len} bytes')

And inside of read_file:

if silent:
    self._write_file_packets(file, file_len, max_packet_size, addr)
else:
    with ui.progressbar(length=file_len, label='Downloading {}'.format(remote_file)) as progress:
        self._write_file_packets(file, file_len, max_packet_size, addr, progress.update)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, okay then. Was just toying around with the idea of duplicating as little as possible. One small nit: prefer f-string literals wherever possible

pros/cli/v5_utils.py Outdated Show resolved Hide resolved
pros/cli/v5_utils.py Outdated Show resolved Hide resolved
pros/cli/v5_utils.py Outdated Show resolved Hide resolved
Comment on lines +534 to +549
if silent:
for i in range(0, file_len, max_packet_size):
packet_size = max_packet_size
if i + max_packet_size > file_len:
packet_size = file_len - i
file.write(self.ft_read(addr + i, packet_size))
progress.update(packet_size)
logger(__name__).debug('Completed {} of {} bytes'.format(i + packet_size, file_len))
else:
with ui.progressbar(length=file_len, label='Downloading {}'.format(remote_file)) as progress:
for i in range(0, file_len, max_packet_size):
packet_size = max_packet_size
if i + max_packet_size > file_len:
packet_size = file_len - i
file.write(self.ft_read(addr + i, packet_size))
progress.update(packet_size)
logger(__name__).debug('Completed {} of {} bytes'.format(i + packet_size, file_len))
Copy link
Author

Choose a reason for hiding this comment

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

I don't think making the progressbar silent would be the way to go since that is the entire point of the progress bar. Maybe just put the code inside the for loop (minus the progress.update part) in a separate function and call it every time in with the for loop? That way there's no major change to the progress bar code and gets rid of code reuse and then progessbar.update can be called in the one that uses the progressbar.

@mooreBrendan mooreBrendan self-assigned this Dec 21, 2020
@Skyluker4
Copy link
Author

Any update on this?

@Skyluker4
Copy link
Author

@edjubuh @HotelCalifornia @mooreBrendan Is this pr dead?

@WillXuCodes
Copy link
Member

@edjubuh @HotelCalifornia @mooreBrendan Is this pr dead?

Not exactly, everybody's sort of busy with their own thing at the moment and I'm sure CLI development will pick up when the school year starts or whenever our internships end.

@BennyBot BennyBot mentioned this pull request Jan 31, 2022
4 tasks
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.

5 participants