Skip to content
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

✨Add list_files function #615 #615

Open
wants to merge 19 commits into
base: develop-pros-4
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions include/pros/misc.h
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,55 @@ double battery_get_capacity(void);
*/
int32_t usd_is_installed(void);

/**
* Lists the files in a directory specified by the path
* Puts the list of file names (NOT DIRECTORIES) into the buffer seperated by newlines
*
* This function uses the following values of errno when an error state is
* reached:
*
* EIO - Hard error occured in the low level disk I/O layer
* EINVAL - file or directory is invalid, or length is invalid
* EBUSY - THe physical drinve cannot work
* ENOENT - cannot find the path or file
* EINVAL - the path name format is invalid
* EACCES - Access denied or directory full
* EEXIST - Access denied
* EROFS - SD card is write protected
* ENXIO - drive number is invalid or not a FAT32 drive
* ENOBUFS - drive has no work area
* ETIMEDOUT - Operation timed out while accessing the volume
* ENFILE - too many open files
*
* \param path
* The path to the directory to list files in
* \param buffer
* The buffer to put the file names into
* \param len
* The length of the buffer
*
* \note use a path of "\" to list the files in the main directory NOT "/usd/"
* DO NOT PREPEND YOUR PATHS WITH "/usd/"
*
* \return 1 on success or PROS_ERR on failure setting errno
*
* \b Example
* \code
* void opcontrol() {
* char* test = (char*) malloc(128);
* pros::c::usd_list_files_raw("/", test, 128);
* pros::delay(200);
* printf("%s\n", test); //Prints the file names in the root directory seperated by newlines
* pros::delay(100);
* pros::c::usd_list_files_raw("/test", test, 128);
* pros::delay(200);
* printf("%s\n", test); //Prints the names of files in the folder named test seperated by newlines
* pros::delay(100);
* }
* \endcode
*/
int32_t usd_list_files_raw(const char* path, char* buffer, int32_t len);

/******************************************************************************/
/** Date and Time **/
/******************************************************************************/
Expand Down
101 changes: 101 additions & 0 deletions include/pros/misc.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <cstdint>
#include <string>
#include <vector>

namespace pros {
inline namespace v5 {
Expand Down Expand Up @@ -525,6 +526,106 @@ namespace usd {
* \endcode
*/
std::int32_t is_installed(void);

/**
* Lists the files in a directory specified by the path
* Puts the list of file names (NOT DIRECTORIES) into the buffer seperated by newlines
*
* This function uses the following values of errno when an error state is
* reached:
*
* EIO - Hard error occured in the low level disk I/O layer
* EINVAL - file or directory is invalid, or length is invalid
* EBUSY - THe physical drinve cannot work
* ENOENT - cannot find the path or file
* EINVAL - the path name format is invalid
* EACCES - Access denied or directory full
* EEXIST - Access denied
* EROFS - SD card is write protected
* ENXIO - drive number is invalid or not a FAT32 drive
* ENOBUFS - drive has no work area
* ENFILE - too many open files
*
* \param path
* The path to the directory to list files in
* \param buffer
* The buffer to put the file names into
* \param len
* The length of the buffer
*
* \note use a path of "\" to list the files in the main directory NOT "/usd/"
* DO NOT PREPEND YOUR PATHS WITH "/usd/"
*
* \return 1 on success or PROS_ERR on failure, setting errno
*
* \b Example
* \code
* void opcontrol() {
* char* test = (char*) malloc(128);
* pros::usd::list_files_raw("/", test, 128);
* pros::delay(200);
* printf("%s\n", test); //Prints the file names in the root directory seperated by newlines
* pros::delay(100);
* pros::list_files_raw("/test", test, 128);
* pros::delay(200);
* printf("%s\n", test); //Prints the names of files in the folder named test seperated by newlines
* pros::delay(100);
* }
* \endcode
*/
std::int32_t list_files_raw(const char* path, char* buffer, int32_t len);

/**
* Lists the files in a directory specified by the path
* Puts the list of file paths (NOT DIRECTORIES) into a vector of std::string
*
* This function uses the following values of errno when an error state is
* reached:
*
* EIO - Hard error occured in the low level disk I/O layer
* EINVAL - file or directory is invalid, or length is invalid
* EBUSY - THe physical drinve cannot work
* ENOENT - cannot find the path or file
* EINVAL - the path name format is invalid
* EACCES - Access denied or directory full
* EEXIST - Access denied
* EROFS - SD card is write protected
* ENXIO - drive number is invalid or not a FAT32 drive
* ENOBUFS - drive has no work area
* ETIMEDOUT - Operation timed out while accessing the volume
* ENFILE - too many open files
*
* \param path
* The path to the directory to list files in
*
* \return vector of std::string of file names, if error occurs, returns vector containing
* two elements, first element is "ERROR" and second element is the error message
*
* \b Example
* \code
* void opcontrol() {
* // Will return vector containing file paths of files in root directory
* std::vector<std::string> files = pros::usd::list_files("/test");
* pros::delay(200);
* // Given vector of std::string file paths, print each file path
* // Note that if there is an error, the vector will contain two elements,
* // first element is "ERROR" and second element is the error message
*
* // Check if error occurred
* if (file_list.front().start_with("ERROR")) {
* // deal with error
* }
* else {
* // file list returned is valid
* // Print each file
* for (std::string& file : files) {
* std::cout << file << std::endl;
* }
* }
* }
* \endcode
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely the docs should explain the params as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added param in header for funcs in commit 3d41ca6 and 4247fb1

std::vector<std::string> list_files(const char* path);
} // namespace usd

} // namespace pros
Expand Down
12 changes: 12 additions & 0 deletions src/devices/vdml_usd.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,15 @@
int32_t usd_is_installed(void) {
return vexFileDriveStatus(0);
}
static const int FRESULTMAP[] = {0, EIO, EINVAL, EBUSY, ENOENT, ENOENT, EINVAL, EACCES, // FR_DENIED
EEXIST, EINVAL, EROFS, ENXIO, ENOBUFS, ENXIO, EIO, EACCES, // FR_LOCKED
ENOBUFS, ENFILE, EINVAL};

int32_t usd_list_files_raw(const char* path, char* buffer, int32_t len) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be handled by vexFileDirectoryGet already, but can we add nullchecks for these parameters?

FRESULT result = vexFileDirectoryGet(path, buffer, len);
if (result != F_OK) {
errno = FRESULTMAP[result];
return PROS_ERR;
}
return PROS_SUCCESS;
}
85 changes: 85 additions & 0 deletions src/devices/vdml_usd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,90 @@ std::int32_t is_installed(void) {
return usd_is_installed();
}

std::int32_t list_files_raw(const char* path, char* buffer, int32_t len) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for our C++ API please use std::string and .c_str to call the internal C funcs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure all of the std::strings are also references pls*

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::string_view

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::string_view

I understand that's more C++onic but for the sake of our users I think a std::string would be a little more approachable... saves everyone a google search.

Copy link
Contributor

@djava djava Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😖 wasted allocation for every call with a string literal for path, which I assume is the typical case.

Copy link
Contributor

@djava djava Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure all of the std::strings are also references pls*

If you choose to use std::string instead of string_view do make sure its const refs, as mutable refs will cause other issues (won't accept temporaries i.e. string literals that convert to std::string)

return usd_list_files_raw(path, buffer, len);
}

std::vector<std::string> list_files(const char* path) {
std::vector<std::string> files = {};
// malloc buffer to store file names
size_t buffer_size = 10000;
char *buffer = (char *) malloc(buffer_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the C++ new char[buffer_size];instead of C's malloc (and later deleteinstead of `free).

It's a bit less scary type-wise and very widely considered better stylistically for C++.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually even better would be std::unique_ptr<char[]> buffer = std::make_shared<char[]>(buffer_size)and then passing buffer.get() to the C function. Then the deletehappens automatically when you exit the function scope.

if (buffer == NULL) {
// try again smaller buffer to see if that works
buffer_size = 500;
buffer = (char *) malloc(buffer_size);
if (buffer == NULL) {
// if still fails, return vector containing error state
// set errno to ENOMEM
errno = ENOMEM;
files.push_back("ERROR");
files.push_back("not enough memory to get file names");
return files;
}
}

// Check path user passed in
std::string_view path_sv(path);
constexpr std::string_view usd_prefix {"usd"};
const size_t usd_prefix_idx = path_sv.find(usd_prefix);
if (usd_prefix_idx == 0 || usd_prefix_idx == 1) {
// Deal with when user prepends path with usd
// as either "usd/..." or "/usd/..."
path_sv.remove_prefix(usd_prefix.length() + usd_prefix_idx);
}

// set path to path_sv.data()
path = path_sv.data();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path_sv is already pointing to *path. It's a read-only "view" over a string that exists elsewhere in memory, hence the name.


// Call the C function
int32_t success = usd_list_files_raw(path, buffer, buffer_size);
// Check if call successful, if error return vector containing error state
if (success == PROS_ERR) {
files.push_back("ERROR");
// Check errno to see which error state occurred
// push back error state to files vector as std::string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm I'm definitely not convinced that this error handling strategy is a good approach. Maybe an out-param for the vector is better so you have some other way to return errors? That's more inline with how pros usually does things anyway. (returning errors via return type)

if (errno == EINVAL || errno == ENOENT) {
// errors related to path not found
files.push_back("path not found");
} else {
// other error stats related to file io
files.push_back("file i/o error");
}
return files;
}

// Parse buffer given call successful, split by '/n'
std::string_view str(buffer);

// delimter_pos is the position of the delimiter '\n'
// index of which character to start substr from
// file_name used to store each file name
size_t delimiter_pos = 0;
size_t index = 0;
std::string file_name;

// Loop until delimiter '\n' can not be found anymore
while ((delimiter_pos = str.find('\n', index)) != std::string::npos) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really wanted to optimize this even more you could write a sentinel function (or whatever people call it I'm tired atm) that grabs all the delimter indexes, and then another loop that stores all the delimter positions in a data structure such as a vector, and then another loop that iterates through the data structure. This changes the O(N * M) behavior of this to O(N + M) where N is the size of the string and M is the number of delimeters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where you got O(N*M). If I'm reading correctly, you're seeing it as a full loop through all N characters each time we loop? That's not true, though, as index changes where the iteration starts each time (starting further along the string on each iteration), and each iteration stops as soon as it locates '\n'. Therefore, this is only O(N).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where you got O(N*M). If I'm reading correctly, you're seeing it as a full loop through all N characters each time we loop? That's not true, though, as index changes where the iteration starts each time (starting further along the string on each iteration), and each iteration stops as soon as it locates '\n'. Therefore, this is only O(N).

ah you're right, I didn't notice the index parameter last night.

// file_name is the string from the beginning of str to the first '\n', excluding '\n'
file_name = std::string(str.data() + index, delimiter_pos - index);

files.push_back(file_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move file_name into the vector instead of copying it. This will reuse the already allocated heap buffer instead of copying over a new one, resulting in each file name being copied once instead of twice or more.

files.emplace_back(std::move(file_name));

Alternatively, file_name could be a std::string_view that you emplace_back. Since files::value_type is std::string, that will result in the file name being copied (for the first time). Actually I think I like this solution better than the move anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented this change in commit 3d41ca6. Made file_name a std::string_view.

// Increment index to start substr from
index = delimiter_pos + 1;

// If index is greater than or equal to str length, break
if (index >= str.length()) {
break;
}
}

// Free buffer
free(buffer);

// Return vector of file names
return files;
}

} // namespace usd
} // namespace pros