tools: Fix imports and add serial port arg for maintenance and updateSystemPakcage. #4
Conversation
toolkit/app-write-mram.py
Outdated
| @@ -139,7 +139,9 @@ def main(): | |||
| parser = argparse.ArgumentParser( | |||
| description="NVM Burner for Application TOC Package" | |||
| ) | |||
| parser.add_argument("-d", "--device", type=str, help="serial port device") | |||
| parser.add_argument( | |||
| "-p", "--port", type=str, help="Serial port device", required=True | |||
There was a problem hiding this comment.
This won't work, it conflicts with the -p pad argument.
Because this option is required (options are usually optional!), maybe just make it a positional required argument? Same with maintenance.py and updateSystemPackage.py.
There was a problem hiding this comment.
You're right I missed the -p for padding. We could just use --port or -P? I prefer explicit keyword args to positional args to match the other args style. Note -d is already commonly used for discover.
There was a problem hiding this comment.
But if you always have to specify it, why not make it positional and then you don't have to type --port each time? It's pretty clean to write maintenance.py /dev/ttyUSB0.
I prefer explicit keyword args to positional args to match the other args style.
The other arguments are all optional. IMO it's more inconsistent to add a required option, rather than adding a positional argument.
There was a problem hiding this comment.
But if you always have to specify it
You don't, it should have a default value like /dev/ttyACM0.
There was a problem hiding this comment.
OK, it's just that you added required=True here 😂
There was a problem hiding this comment.
Could use -P and --port. Or remove the shorthand -p from the pad option and reuse -p for port.
There was a problem hiding this comment.
Okay, updated. I wasn't sure if we should keep -p for tools that don't have it, so I just removed it from all tools.
There was a problem hiding this comment.
Could use -P and --port
No problem, I could revert that if you like.
There was a problem hiding this comment.
I think it's fine with just --port. This tool will anyway most likely be used from a makefile or script.
There was a problem hiding this comment.
Let's get this merged and then focus on the db issue.
iabdalkader
force-pushed
the
fix_tools
branch
2 times, most recently
from
dc40aec to
fcbe1e6
Compare
|
For consistency, adding |
iabdalkader
force-pushed
the
fix_tools
branch
2 times, most recently
from
36ff066 to
b7954ca
Compare
Signed-off-by: iabdalkader <[email protected]>
Add `--port` arg to `maintenance.py`, `updateSystemPackage.py` and `app-write-mram.py` Signed-off-by: iabdalkader <[email protected]>
No description provided.