-
Notifications
You must be signed in to change notification settings - Fork 37
Support reset and flash for ARM MPS2 board. #160
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
Conversation
@bridadan can you review the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this PR up! Good initial pass, now I think we have to do a bit of clean up. Please see my comments below.
# type = 'CopyMethod' | ||
# stable = True | ||
# capabilities = ['shutil', 'default'] | ||
# required_parameters = ['image_path', 'destination_disk'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the commented code here please?
from host_test_plugins import HostTestPluginBase | ||
|
||
|
||
class HostTestPluginCopyMethod_MPS2(HostTestPluginBase): | ||
# MPS2 specific flashing / binary setup funcitons | ||
""" Generic flashing method for mbed-enabled devices (by copy) | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets fix this comment, seems like the older comment was better (seeing how it is a specific flashing function for the MPS2)
file.write(line), | ||
except IOError: | ||
return False | ||
def generic_mbed_copy(self, image_path, destination_disk): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is basically a private function, lets name it something not called generic_mbed_copy
since it isn't actually generic, but mps2 specific.
result = True | ||
destination_path = os.path.join(destination_disk,"mbed.bin") | ||
try: | ||
copy(image_path, destination_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we tested with this copy method, however this uses shutil
to do the copying. On Linux, this isn't a "blocking" write if I remember correctly. Ideally it should be.
The default method uses cp
followed by sync
. We should most likely be using the same method here as well before this gets merged to master. (For reference, here's the default copy method: https://github.com/ARMmbed/htrun/blob/master/mbed_host_tests/host_tests_plugins/module_copy_shell.py#L76)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bridadan I just tested on Linux, this copy method is blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real question is: is there magic sauce in the windows copy command that shutil.copy does not do?
@@ -67,12 +69,18 @@ def execute(self, capability, *args, **kwargs): | |||
@return Capability call return value | |||
""" | |||
result = False | |||
|
|||
disk = kwargs.get('disk', None) | |||
target_id = kwargs.get('target_id', None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add the same error checking that's in the copy plugin for these parameters here.
reboot_fh = open(reboot_file_path,"w") | ||
reboot_fh.close() | ||
time.sleep(3) # sufficient delay for device to boot up | ||
mount_res, destination_disk = self.check_mount_point_ready(disk, target_id=target_id,timeout=pooling_timeout) | ||
elif capability == 'shutdown.txt': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead get rid of the rest of these "capabilities" since they aren't doing anything. (be sure to remove them at the top of this file as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caught a few more things, please see below
copy_command = 'cp' if os.name == 'posix' else 'copy' | ||
result = self.run_command([copy_command, image_path, destination_path], shell=True) | ||
if result and os.name == 'posix': | ||
result = result and self.run_command('sync','-f',destination_path, shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a funny quirk with the sync
command. On Mac (which show up as posix
), the sync
command doesn't have the -f
option. It's only present on Linux as far as we can tell. Can you make this part have the same behavior as the current copy plugin? https://github.com/ARMmbed/htrun/blob/master/mbed_host_tests/host_tests_plugins/module_copy_shell.py#L84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And really also need to take into account @theotherjimmy's changes as well: https://github.com/ARMmbed/htrun/pull/157/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If copy fails we don't need a sync https://github.com/ARMmbed/htrun/pull/157/files always does a sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you don't need the result and
bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we don't need results and.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its sync is part of mac, running sync -f doesn't complain. for that matter running sync also passes with error code 0.
shutdown.txt - shutdown from run mode. | ||
reset.txt - reset FPGA during run mode. | ||
shutdown.txt - not implemented. | ||
reset.txt - not implemented. | ||
""" | ||
|
||
# Plugin interface | ||
name = 'HostTestPluginResetMethod_MPS2' | ||
type = 'ResetMethod' | ||
capabilities = ['reboot.txt', 'shutdown.txt', 'reset.txt'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our previous conversation, would you mind removing shutdown.txt
and reset.txt
? Currently there's nothing htrun that would make use of these capabilities.
# TODO: Implement touch file for reboot | ||
pass | ||
|
||
reboot_file_path = os.path.join(destination_disk,capability) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick for the next few lines here, could you please add a space between function parameters and commas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
capabilities = ['mps2'] | ||
required_parameters = ['image_path', 'destination_disk'] | ||
result = True | ||
destination_path = os.path.join(destination_disk,"mbed.bin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space between commas and parameters here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
1) use default shutil copy method. 2) sync filesystem after copy. 3) have consistent naming.
04197d5
to
70ab7af
Compare
Have updated changes to incorporate the previous review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
@mazimkhan I believe this is ready to go. The CM3DS team will need this to support customers, is there anything this needs from your point of view? |
Merging and releasing. |
No description provided.