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

[microTVM] Zephyr: refactor _find_openocd_serial_port #10346

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

gromero
Copy link
Contributor

@gromero gromero commented Feb 22, 2022

Refactor _find_openocd_serial_port() as a generic USB serial port
finder since other runners beyond openocd use it (e.g. jlink runner).

Also instead of using redundant hardcoded values in BOARD_USB_FIND_KW
dict, use idVendor and idProduct from boards.json. And don't use 'usb'
module to first find the serial number of the port and then pass it to
'serial' module to obtain the port path, instead search for the port
path directly via 'serial' module using the serial number (if provided)
or use idVendor and idProduct values taken from boards.json.

Signed-off-by: Gustavo Romero gustavo.romero@linaro.org

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

Thanks @gromero! looks good, just a minor change required.


return autodetected_openocd_serial


def _get_openocd_device_args(options):
return ["--serial", openocd_serial(options)]
Copy link
Member

Choose a reason for hiding this comment

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

openocd_serial is still used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mehrdadh Thanks for catching it! I'll promote _generic_find_serial_port() to a module method so it can be used in _get_openocd_device_args.

@mehrdadh
Copy link
Member

I think you need to rebase because of this PR: #10304

Refactor _find_openocd_serial_port() as a generic USB serial port
finder since other runners beyond openocd use it (e.g. jlink runner).

Also instead of using redundant hardcoded values in BOARD_USB_FIND_KW
dict, use idVendor and idProduct from boards.json. And don't use 'usb'
module to first find the serial number of the port and then pass it to
'serial' module to obtain the port path, instead search for the port
path directly via 'serial' module using the serial number (if provided)
or use idVendor and idProduct values taken from boards.json.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
@gromero gromero force-pushed the refactor_microtvm_serial_port_finder branch from 25dd703 to bd82437 Compare February 23, 2022 14:33
@gromero
Copy link
Contributor Author

gromero commented Feb 23, 2022

I think you need to rebase because of this PR: #10304

@mehrdadh I've pushed a rebased version. However diff shown by GH is total crap imo. So maybe you would need to pull the changes and check the modified file locally. I don't know why GH (and git apparently too) is doing such a horrible diff work. I've tried different diff algorithms and nothing really improved :S

Could you please test it against your hardware fleet? :)

@alanmacd
Copy link
Contributor

alanmacd commented Feb 23, 2022

I think you need to rebase because of this PR: #10304

Could you please test it against your hardware fleet? :)

@gromero I can test your changes on the hardware, we are just waiting for an unrelated fix to land, then I'll run your branch. Your rebase looks good to me from the perspective of merging in the #10304 changes.

@gromero
Copy link
Contributor Author

gromero commented Feb 23, 2022

I think you need to rebase because of this PR: #10304

Could you please test it against your hardware fleet? :)

@gromero I can test your changes on the hardware, we are just waiting for an unrelated fix to land, then I'll run your branch. Your rebase looks good to me from the perspective of merging in the #10304 changes.

@alanmacd Hi Alan. Thanks a lot for the review and for testing the change against the hw fleet there ;)

@alanmacd
Copy link
Contributor

I think you need to rebase because of this PR: #10304

Could you please test it against your hardware fleet? :)

@gromero I can test your changes on the hardware, we are just waiting for an unrelated fix to land, then I'll run your branch. Your rebase looks good to me from the perspective of merging in the #10304 changes.

@alanmacd Hi Alan. Thanks a lot for the review and for testing the change against the hw fleet there ;)

@gromero I ran you changes last night, the testing run looks good.

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

LGTM!

@mehrdadh
Copy link
Member

Thanks @alanmacd for testing on hardware fleet!

@gromero
Copy link
Contributor Author

gromero commented Feb 24, 2022

Thanks a lot @mehrdadh @alanmacd

@areusch would you mind to land this change, please?

@areusch areusch merged commit b56770b into apache:main Feb 28, 2022
@areusch
Copy link
Contributor

areusch commented Feb 28, 2022

thanks @gromero !

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
Refactor _find_openocd_serial_port() as a generic USB serial port
finder since other runners beyond openocd use it (e.g. jlink runner).

Also instead of using redundant hardcoded values in BOARD_USB_FIND_KW
dict, use idVendor and idProduct from boards.json. And don't use 'usb'
module to first find the serial number of the port and then pass it to
'serial' module to obtain the port path, instead search for the port
path directly via 'serial' module using the serial number (if provided)
or use idVendor and idProduct values taken from boards.json.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
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.

4 participants