-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(push): add an option to specify targets #775
Conversation
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.
Looks good but I do want to make sure that you can still use this without these args and get the old behavior
push
Outdated
def _transfer_firmware(host, repo_path, scp, ssh, sensors): | ||
def _build_fw(zip_path, apps_path, targets): | ||
if targets: | ||
regex_list = [re.compile(f"{target}" + r"(.*).hex") for target in targets] |
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 rear panel produces .bin
files, you'll need to include those 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.
can this be combined into this?
f"{target}(.*).hex"
the regex should be valid
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.
We should define a list of targets and validate them against the user input, something like this.
TARGETS = [
"head",
"gantry-x",
"gantry-y",
"gripper",
"pipettes-single",
"pipettes-multi",
"pipettes-96",
"hepa-uv",
"rear-panel",
]
for target in args.targets:
if target not in TARGETS:
print(f"Unknown target {target}", file=sys.stderr)
continue
# do work here
We actually do something similar in the scripts/subsystem_versions.py
script
if targets: | ||
regex_list = [re.compile(f"{target}" + r"(.*).hex") for target in targets] | ||
with ZipFile(zip_path, 'w') as zf: | ||
for fname in os.listdir(apps_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.
Same as the above comment, the scripts/subsystem_versions.py
script validates the filenames, it could be useful to check that.
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.
Changes look good to me, I left a few comments.
We should validate the --target
args against a known list of targets.
Other than that the change works, I tested it locally!
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.
nice work!
Overview
These changes to push allow us to specify targets in the form of presets, and only those targets will be built and copied onto the robot. To allow this, a new
update_shortsha
function grabs the existing manifest file from the robot, and updates the sha of the specified targets.You can specify just one or multiple targets, for example:
./push 10.10.10.10 --targets pipettes-single pipettes-multi gantry-x
../push 10.10.10.10 --targets pipettes-single
If this argument isn't used, the script will run the same as it did before, meaning it will build and update all firmware presets.
Logically, it behaves independently of the
sensors
argument, meaning you can use thefirmware-g4-sensors
build preset, and either send the full build or just the hex files for whichever nodes.Changelog
Test Plan
For all nodes:
can_control
sensors
argument still works with and without targetstargets
isn't usedReview Requests
update_shortsha
different from where I am?