Refactor Tbots.py a little bit - #3905
Conversation
|
@GrayHoang ready for review? |
nycrat
left a comment
nycrat
left a comment
There was a problem hiding this comment.
Looking good so far. I think that tbots.py is now going in the right direction!
|
Me when I lie |
|
THere are some improvements that could be made to the history thing, like adding a bit more description but I don't wanna :((((((( (its some very pleasant tuple manipulation :DDDD) |
|
I made the changes. Please test and give me some feedback, but barring that this is ready to merge @nycrat @StarrryNight |
| description="Flash the powerboard firmware (powerloop_main)", | ||
| ), | ||
| # Maps to the deploy_powerboard.yml playbook but additionally compiles | ||
| # powerloop_main with the DEBUG_POWERLOOP flag, swapping in bare setup()/loop() |
There was a problem hiding this comment.
Is this is same as the disable services from above? is this two separate paths to the same setup, or just a shortcut so you dont have to select disable each time?
There was a problem hiding this comment.
no, disable_powerloop prevents the Pi from trying to communicate with the powerboard.
This one flashes dummy code onto the powerboard that pretty much doesn't do anything, so that elec people or testers can flash their own code on.
|
|
||
| if not past_cmd: | ||
| return | ||
| print(f"\n{'=' * 33} Running: {'=' * 38}\n\n{past_cmd}\n\n{'=' * 81}\n") |
There was a problem hiding this comment.
just some arbitrarily chosen numbers to create nice dividers in the CLI
There was a problem hiding this comment.
yea i agree its kind of weird to have random constants but we got bigger issues to worry about here 💀
| ).unsafe_ask() | ||
| cmd_title += launch_label | ||
|
|
||
| if launch_mode == InteractiveCli.LaunchMode.SIM: |
There was a problem hiding this comment.
a little unclear what this whole if block is doing, maybe some parts can be pulled into its own function
There was a problem hiding this comment.
the block is creating a questionary selection prompt and retrieving the results. While it is possible to move this to its own function I don't really think it would be meaningful, and would just mean that a future developer would have to run around looking at a bunch of different functions calling each other to understand the hierarchy.
| for label in sim_option_labels: | ||
| cmd_title += label | ||
|
|
||
| for opt in sim_options: |
There was a problem hiding this comment.
same sentiment here, this could probably be its own named fn
There was a problem hiding this comment.
this code is also just creating a questionary prompt and handling the result, adding labels and arguments and generating the command.
| choices=InteractiveCli.PLAYBOOK_CHOICES, | ||
| style=InteractiveCli.INTERACTIVE_STYLE, | ||
| ).unsafe_ask() | ||
| cmd_title += flash_cmd_label |
There was a problem hiding this comment.
i feel like im seeing a lot of these label updates, is it the best way to keep separately updating in each section?
There was a problem hiding this comment.
the problem is that questionary doesn't support callbacks afaik, so there's not really any way to enforce behaviour after a question is answered. I'm pretty sure questionary is just a tool to help create those selection prompts and retrieve some values, but doesn't really support implementing behaviour. So unfortunately, these sorts of updates are the best way, I think.
|
the bazelrc change might not need to be used? idk I always find myself needing to limit the number of jobs so I don't get screwed over |
adrianchan787
left a comment
adrianchan787
left a comment
There was a problem hiding this comment.
Two small things but looks good, also tested tbots.py a bit and seems to work
5 participants
Description
I hate
tbots.pyThis only sort of fixes the problems we are having, it effectively introduces some boilerplate to prevent having to hardcode flags and values into the decision tree for the interactive CLI. It also localizes all the interactive CLI stuff to a class rather than have it distributed across two separate files.
Testing Done
tbots.pyruns fine after changes (I think? No new bugs introduced??)Resolved Issues
Resolves #3876
Resolves #3885
Resolves #3871
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue