LFS Telecommand to list files / directories [Issue #7] #216
Conversation
Replaced all previous code for FLASH driver for NAND flash module.
Updated littlefs_helper.c configuration to use NAND memory module configuration values
Made changes to flash to handle full page reading and writing, added flash telecommand that unblocks block locks.
Minor changes to flash read hex telecommand for testing, added docstring for FLASH_reset function and telecommand.
Removed all debugging printing lines to prepare for merging. Added new timeout variable for handling HAL_SPI transmits of 2176 bytes.
Removed the telecommand unblock_block_locks and added automatic execution during write functions, minor changes to other flash telecommands.
Fixed issue with improper page addressing where littlefs sent page address in bytes rather than the actual page address.
Removed unnecessary lines of code and added additional helpful comments based on PR comments
Current version of this telecommand doesn't store the directory information anywhere yet, only sends it through UART through debugging.
Added a telecommand for creating a directory and updated warning if trying to create a directory that already exists
Saksham-P
changed the title
|
The implementation for the listing directory telecommand works as intended, although I just want to make sure if the implementation is as optimized and efficient as it can be. |
|
@Saksham-P @KaleF07 Wondering if the comments made sense? |
Saksham was mostly the one working on this issue, some of my commits are from previous LFS issues. I will let him respond to the comments as I have not looked at the code yet |
Added Break statement within while loop if error reading a directory entry, and return the error after closing the directory
|
I updated the code, and didn't change the loop, but just checked if we read dir gave an error, we break instead of returning the error, and then return the error after closing the directory. |
| DEBUG_uart_print_str("Error reading directory contents.\n"); | ||
| return read_dir_result; | ||
| if (read_dir_result >= 0) { | ||
| DEBUG_uart_print_str("Successfully Listed Directory Contents.\n"); |
There was a problem hiding this comment.
Small thing here, this if statement seems redundant, as you could just put this debug_uart_print_str in the below else statement.
if (read_dir_result < 0) {
return read_dir_resust;
} else {
// debug statement
return 0;
}
All functions have some coverage of LOG_message, some don't have it entirely as they are needed to print useful information to the serial
|
I've replaced as many debug statements to LOG_message statements, there are still a few that I intentionally left in the code as they are used to send usefull information to the Serial, for example when listing the directories to the serial, it uses Debug_UART. I've also left the |
|
As per @NuclearTea 's request, I have tested this branch and the code seems to be working correctly. This lines up with my expectation as I have created a directory called lifecycle while working on another issue. Then running The first time, directory creation succeeds and is confirmed by calling Finally by calling This also seems correct as I have created deploy_antenna_on_boot_enabled.bool while working on another issue. I have also tried listing a directory which didn't exist: This Failed with error -2. Some Closing Thoughts
Great work this is super sweet! Hope this helps! |
There was a problem hiding this comment.
LGTM! @DeflateAwning wondering what you think. The code has been tested by @Makiv1 and has passed testing.
There was a problem hiding this comment.
This looks fantastic, thanks! Just a few tiny changes, but awesome overall.
In addition to the review comments, in the display, can we please suffix folders (but not files) with /. So, for example, it would look like this:
========================= Executing telecommand 'fs_list_directory'=========================
Name bytes
./
../
lifecycle/
some_file.txt 12
| int8_t read_dir_result = 1; | ||
| while (read_dir_result >= 0) | ||
| struct lfs_info info; | ||
| DEBUG_uart_print_str("Name \t bytes\n"); |
There was a problem hiding this comment.
This is gonna be awfully difficult to read when the satellite's in space ;)
Please use the logging function to display this info.
| count--; | ||
| } | ||
|
|
||
| DEBUG_uart_print_str(info.name); |
There was a problem hiding this comment.
Please switch all this to LOG_message
| DEBUG_uart_print_str(" bytes"); | ||
| } | ||
| DEBUG_uart_print_str("\n"); | ||
| } | ||
|
|
||
| int8_t close_dir_result = lfs_dir_close(&LFS_filesystem, &dir); | ||
| if (close_dir_result < 0) | ||
| { | ||
| DEBUG_uart_print_str("Error closing directory.\n"); |
There was a problem hiding this comment.
Please remove this DEBUG_uart_print when you add LOG_message calls
| if (read_dir_result < 0) { | ||
| return read_dir_result; | ||
| } else { | ||
| DEBUG_uart_print_str("Successfully Listed Directory Contents.\n"); |
There was a problem hiding this comment.
Please remove DEBUG_uart_print_str. Please try to find all instances in the new code you're adding here.
| int8_t format_result = lfs_format(&LFS_filesystem, &LFS_cfg); | ||
| if (format_result < 0) { | ||
| LOG_message(LOG_SYSTEM_LFS, LOG_SEVERITY_CRITICAL, LOG_all_sinks_except(LOG_SINK_FILE), "Error formatting FLASH memory!"); | ||
| return format_result; |
There was a problem hiding this comment.
Can you please convert this file's indentation to use spaces instead of tabs. There's a few places it's mixed in this repo, but in general, we prefer spaces. Ideally, never mix in the same file.
NuclearTea
added
the
blocker
DeflateAwning
added
pr-awaiting-changes
and removed
pr-please-review
Got rid of static configuration and debug imports Folders are now displayed with '/'
Update:
Concern?After pushing my changes, I realized that my local build doesn't give an error or warning when I am using
My current arm compiler is |
|
After merging/working on the version in |
Added Telecommand to create directory
Added Telecommand to list directory entries
.and..)