CLI and feedback enhancements #461
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
February 19, 2025 16:08
added 13 commits
March 11, 2025 07:29
|
Thank you for your patches.
|
nxpfrankli
reviewed
Mar 11, 2025
| build |
There was a problem hiding this comment.
Create sperate patch for .gitignore file
nxpfrankli
reviewed
Mar 11, 2025
| @@ -62,6 +62,7 @@ static map<string, shared_ptr<FileBuffer>> g_filebuffer_map; | |||
| static mutex g_mutex_map; | |||
| static bool g_small_memory = true; | |||
|
|
|||
| // [what is the point/value of this value?] | |||
| #define MAGIC_PATH '>' | |||
There was a problem hiding this comment.
Mark avoid dead loop when search parent path
nxpfrankli
reviewed
Mar 11, 2025
| @@ -127,6 +128,9 @@ int DataBuffer::ref_other_buffer(std::shared_ptr<FileBuffer> p, size_t offset, s | |||
| return 0; | |||
| }; | |||
|
|
|||
| /** | |||
| * @brie Base class for FS things [whatever 'FS' means/is]. | |||
| */ | |||
nxpfrankli
reviewed
Mar 11, 2025
| * Passing a bool like dir is bad style since it only controls flow. | ||
| */ | ||
| virtual int split(const string &path, string *dir_part, string *name_part, bool dir=false) | ||
| { |
There was a problem hiding this comment.
use seperate patch to change outbackfile-> dir_part, outfilename -> filename_part
nxpfrankli
reviewed
Mar 11, 2025
| return 0; | ||
| } | ||
|
|
||
| protected: | ||
| FSBasic() {} // enforce that this class is abstract base; only creatable via a superclass | ||
| const char * m_ext = nullptr; |
<
6855
div class="js-comments-holder" >
There was a problem hiding this comment.
This one should be sperate patch
nxpfrankli
reviewed
Mar 11, 2025
| return -1; | ||
| } | ||
|
|
||
| if (m_use_bmap && m_bmap_filename.size() && !check_file_exist(m_bmap_filename)) { | ||
| set_last_err_string("FB: bmap file not found"); | ||
| set_last_err_string("FB: bmap file not found: " + m_bmap_filename); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
avoid two line should sperate patch, show improve error message
nxpfrankli
reviewed
Mar 11, 2025
| @@ -574,7 +574,7 @@ int CmdUsbCtx::look_for_match_device(const char *pro) | |||
|
|
|||
| uuu_notify nt; | |||
| nt.type = nt.NOTIFY_WAIT_FOR; | |||
| nt.str = (char*)"Wait for Known USB"; | |||
| nt.str = (char*)"DWait for Known USB"; | |||
| call_notify(nt); | |||
nxpfrankli
reviewed
Mar 11, 2025
| #endif | ||
|
|
||
| } |
There was a problem hiding this comment.
this part should be sperate patch
nxpfrankli
reviewed
Mar 11, 2025
uuu/buildincmd.cpp
Outdated
| @@ -237,7 +237,7 @@ void BuiltInScriptMap::ShowCmds(FILE * const file) const | |||
| fprintf(file, "|"); | |||
| } | |||
| } | |||
| fprintf(file, ">"); | |||
| //fprintf(file, ">"); | |||
| } | |||
nxpfrankli
reviewed
Mar 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a big change so you may be hesitant to merge it. None the less and FWIW the tool is very useful, but I found the UX lacking including CLI, help info and user feedback. I reworked these aspects and as it was a significant change and the CLI code lacked structure (lacked componentization, uuu.cpp way too big, ...), I organized the code somewhat.
I started from NXP repo commit da3cd53 (16-May-2024, Fix PID 0159 device name from imx93 to imx91). Via backmerge, all files auto-merged except for uuu.cpp which had too many changes. To support review of the new CLI without deleting the old one, I renamed my uuu.cpp to cli.h so that uuu.cpp is mostly the same as it was. Can choose which CLI to use in main(). It's a half-measure but I didn't want to just delete the existing CLI code. I did update the old CLI to work with the other changes throughout.
There were about 20 commits after 16-may-2024 up to today (11-mar-2024). Since all files automerged other than uuu.cpp, only commits that modified uuu.cpp require manual merging. These are:
f764c01 fixes #443: -udev stdout should replace 70-uuu.rules rather than be appended to it
Resolution: Changes made manually in cli.h
5d77a61 Improve uuu help context
Resolution: No chagne required since new CLI code includes new help info
6c2141e Add dynamic
MAX_PROGRESS_WIDTHglobal variable to handle long lines (#435)Resolution: **Skipped since couldn't understand intent of change. FYI This change would go into HorizontalScrollingFormatter.h; not cli.h
09fe178 remove ref global varible g_verbose in bmap.cpp
Resolution: Changes made manually in cli.h
c031b2a move global g_bmap_mode into libuuu
Resolution: Changes made manually in cli.h
CHANGES
o Theme: Command based CLI
- New CLI that is command-based; instead of switches that control high-level modes
- Segregation between install operations and other operations. Most options only apply to install. Makes it clearer what options go with what operations.
- install [OPTIONS] auto-detect-file
- install [OPTIONS] -s script-file ARGS
- install [OPTIONS] PRO: ARGS
- ls-devices: replaces -lsusb
- ls-builtin [BUILTIN]: moved built-in list out of -h
- cat-builtin BUILTIN: replaces -bshow
- help CONTEXT: organizes help info more consistently
- Renamed -d to --continuous: 'deamon' is too gargony
- Renamed -s to -i|--interactive: 'shell' is too gargony
- Renamed -m to --filter-path: more readable
- Renamed -ms to --filter-serial: more readable
- All multi-char options require two dashes: more consistent with other tools
o Theme: Enhance overwrite-based feedback
- Fixed horizontal scrolling of active command; it was only scrolling one or two chars left
- Show final status as "Successful" instead of "Done" since is more useful to know succeeded; not just done
o Theme: Enhance append-based feedback
- Show waiting for device message after validation so that it's not showing when never actually waiting for a device
- NOTIFY_CMD_INFO messages came out without context and % complete partially overwrote it; looked bad
- Added color and spacing to enhance readability
o Theme: Enhance CLI and script processing UX
- Overhauled error handling and reporting for CLI and script processing
- Provide 8000 limited feedback for bad input instead of outputting pages of info which is off-putting and confusing. Show limited help unless user asks for details.
- Edited help content for clarity and organized for uniformity
o Theme: Simplify logic and enhance code quality
- For overwriting feedback, leave the cursor at the end of the status area instead of trying to move it below the area on exit
- Factored out much of uuu.cpp into new files since it was too large and consisting of rather unrelated logic
- Renamed types that mentioned built-in scripts to be more generic since they are used for custom scripts as well as built-in script
- Add logger class and singleton to centralize logging logic
- Removed "using std" to make code easier to move between .cpp and .h files
- Changed feedback output to stderr so that commands that output info that one might want to capture does not include the info (unless they also capture stderr). In particular, 'help udev' (old -udev) outputs info that one wants in a file, but they don't want the app title or the instructions which both now go to stderr.
- Added header comments for functions that I learned what they did
o Theme: Support doxygen
- Enable doxygen output by adding @file to source files
- Enable doxygen output by adding Doxyfile; didn't understand how the existing Doxyfile.in file works
o Theme: Tried to fix auto-complete
- Auto-complete disabled for Linux since fails for input like: uuu f.uuu foo
- Auto-complete disabled for Windows since crashes for input line: uuu -autocomplete
o Theme: devops enhancements
- Minor changes to support building outside CI build system