SD file block adjustments

[AndyLindsay]

Regarding this program:

The user defined code currently emits fclose(fp); and the program emits this code:

// ------ Libraries and Definitions ------
#include "simpletools.h"

// ------ Global Variables and Objects ------
char  text_to_sd[64];
char  text_from_sd[64];



// ------ Main Program ------
int main() {
  sd_mount(22, 23, 24, 25);
  strcpy(text_to_sd, "Hello 123");			// Save string into variable text_to_sd.
  strcpy(text_from_sd, "");			// Save string into variable text_from_sd.
  FILE* fp = fopen("test2.txt","w");fwrite(text_to_sd, 1, 10, fp);fclose(fp);
  fp = fopen("test2.txt","r");fread(&text_from_sd, 1, 10, fp);print(text_from_sd);
  print("\r");
  fclose(fp);

}

1. The SD file open block displays an option of read-write, but uses fopen(..., "w"). I think it would be better to change the default "read-write" dropdown menu option to "write-only".
2. The SD file open and SD file blocks are missing carriage returns after each statement.
3. The FILE *fp statement occurs in main, which puts it out of scope in functions. I think it would be better to have it globally declared (but not initialized -i.e. initialized to zero).
4. The file did not successfully reopen as "r" after opening as "w" because it needs to be closed first. That's in the user defined blocks. Assuming that the FILE *fp pointer is global and initialized to zero, each SD file open block could: if(fp) fclose(fp);
5. It would be nice to have a SD file close block that emits if(fp) {fclose(fp); fp = 0;);

After the changes are made, the blocks could look like this:

...and the code generated would look like this:

// ------ Libraries and Definitions ------
#include "simpletools.h"

// ------ Global Variables and Objects ------
char  text_to_sd[64];
char  text_from_sd[64];

FILE* fp;

// ------ Main Program ------
int main() {
  sd_mount(22, 23, 24, 25);
  strcpy(text_to_sd, "Hello 123");			// Save string into variable text_to_sd.
  strcpy(text_from_sd, "");			// Save string into variable text_from_sd.
  if(fp) fclose(fp); 
  fp = fopen("test2.txt","w");
  fwrite(text_to_sd, 1, 10, fp);
  if(fp) fclose(fp); 
  fp = fopen("test2.txt","r");
  fread(&text_from_sd, 1, 10, fp);
  print(text_from_sd);
  print("\r");
  if(fp) {fclose(fp); fp = 0;};
}

[AndyLindsay]

@VonSzarvas, looks like maybe you've been using the blocks successfully with a different approach. Please review and comment if this conflicts with other usage cases. Thanks!

[VonSzarvas]

One issue with using a fixed width of bytes when writing is that the additional bytes will be written as null.
Later, when opening that file with a computer text editor, it may not show up at all (or as expected). Some text viewers may not handle the null well, especially without line endings.

First reaction... I'd prefer to see these functions in the blocks:

1. Now there is open-read, open-write, and open-read/write. The write starts at the beginning of the file, overwriting existing data. What about adding, open-append ? (Thought for implementing the read-write vs read only needed, Perhaps just an "append" tickbox on the existing "write" or "read-write" blocks.

2. Write bytes should stay as-is, but a new block "Write String" with an include line-ending tickbox which is ticked by default. Write String wouldn't need to ask the number of bytes, so nulls would not be padded/written to the file.

3. Bonus block, Read string. The block should probably have a dropdown for the delimeter with some common defaults (CR, CRLF, NULL), and a CUSTOM option which allows user to add a char block representing any delim they want. Read string would read one string into a variable. Customer sets up a loop to parse the whole file.

[AndyLindsay]

Thanks @VonSzarvas!

I especially like the append idea.

Adding to the list, another possible feature would be have the open blocks return their fopen call's status return value.

[zfi]

@AndyLindsay, reference your 5 items:

1. There are 6 modes in the fopen() from the C library. Are they all available in this implementation? The answer to this question will guide followup comments concerning the SD Open block. We need to be careful that we do not alter the block in a way that makes it incompatible with the previous iteration.

2. CR/LF issues are addressed in my local branch and will appear in the next PR I submit.

3. The file pointer is now global on my local branch.

// ------ Libraries and Definitions ------
#include "simpletools.h"

// ------ Global Variables and Objects ------
FILE *fp;
int item;
char  buffer[64];
char  content[64];

[ ... ]

4 & 5) The SD file block actually has a Close option in the drop-down list. I updated the code to check fp is not null before closing, as you recommended.

// ------ Main Program ------
int main() {
  sd_mount(0, 0, 0, 0);
  item = 10;
  strcpy(buffer, "1234567890");			// Save string into variable buffer.
  strcpy(content, "");			// Save string into variable content.
  fp = fopen("file.txt","w");
  fwrite(buffer, 1, ((int) strlen(buffer)), fp);
  if(fp) fclose(fp);
  fp = fopen("file.txt","r");
  fread(&content, 1, 10, fp); 
 print(content);
  print("\r");
    if(fp) fclose(fp);
}

[zfi]

@VonSzarvas, Your first item will depend on what file modes are available in the C compiler fopen() implementation. I don't know if we are getting the standard fopen() or a skinny version of it. Items 2 can easily be addressed by including a strlen(string_buffer) in the length setting. That will write the string to the card. You can add terminators with additional SD write blocks.

Item 3 is possible but needs to be vetted first. I would not want to develop the block unless there is a demand for it.

[zfi]

Many of the suggestions listed above have been implemented in v1.5.7, released on 1/16/2021. Please evaluate this issue against the new release and then let's determine if we can close this issue.

[AndyLindsay]

When close is chosen from the SD file block, a new SD file [close] block appears in addition to the SD file [write] block that was placed on the canvas.

I think it would be both easier to fix and better for the user if SD file close was its own block.

[zfi]

@AndyLindsay,in Solo 1.5.7, If I put an SD file block on the canvas and then select 'close' from the drop-down, the SD file write block mutates to an SD file close block and the value (10) block becomes orphaned. You description indicates that there would be two SD file blocks - write and close - after the 'close' type is selected. Can you verify that there are two SD file blocks after that operation?

We could create an SD file close block. There are also a number of other possible SD blocks to support more direct access to the file system on the card to support different data types.

[AndyLindsay]

@zfi, I just tried it again, and this time, the block successfully mutated, and only orphaned the 10 block. There is no second instance of the SD file write block.

[AndyLindsay]

@zfi, regarding "There are also a number of other possible SD blocks to support more direct access to the file system on the card to support different data types."

Yes, I've been trying to just focus on correcting what was there, but all the while thinking that it would be great to see some additional blocks.

For now though, I would just like to bump suggestion 1 that @VonSzarvas made earlier:

...The open-write starts at the beginning of the file, overwriting existing data. What about adding, open-append ? ...

SD file open "filename.txt" as [append] could emit fopen(filename.txt, "a");

[AndyLindsay]

I do think having SD file close as its own block would be good.

[zfi]

We can track the new close file block in #537.

[zfi]

Altering an existing block is likely going to break existing projects that contain the block. The SD File Open block has two modes, read and read/write. @VonSzarvas says there is a write-only mode but I don't see that in the code.

        .appendField(new Blockly.FieldDropdown([
          ['as read-only', 'r'],
          ['as read-write', 'w'],
        ]), 'MODE');

I should point out that the read-write mode is not properly implemented. Fopen("file", "w") should create a new file or truncate an existing file and only allow writes to the file. I have verified that this is in fact what it is doing. The correct file mode for read/write is "w+" if the intent is to open a file and position the pointer to the beginning of the file for read or write operations.

To address the request for an "append" mode, we could add one or more MODE combinations to the existing block by extending the MODE list without breaking any project that includes this block. All I really need is a definitive list of the modes we want to implement.

[zfi]

These are the official options:

Mode	Description
"r"	read: Open file for input operations. The file must exist."w"
"a"	append: Open file for output at the end of a file. Output operations always write data at the end of the file, expanding it. Repositioning operations (fseek, fsetpos, rewind) are ignored. The file is created if it does not exist.
"r+"	read/update: Open a file for update (both for input and output). The file must exist.
"w+"	write/update: Create an empty file and open it for update (both for input and output). If a file with the same name already exists its contents are discarded and the file is treated as a new empty file.
"a+"	append/update: Open a file for update (both for input and output) with all output operations writing data at the end of the file. Repositioning operations (fseek, fsetpos, rewind) affects the next input operations, but output operations move the position back to the end of file. The file is created if it does not exist.

With the mode specifiers above the file is open as a text file. In order to open a file as a binary file, a "b" character has to be included in the mode string. This additional "b" character can either be appended at the end of the string (thus making the following compound modes: "rb", "wb", "ab", "r+b", "w+b", "a+b") or be inserted between the letter and the "+" sign for the mixed modes ("rb+", "wb+", "ab+").

[AndyLindsay]

Thank you @zfi, yes, I've been pouring over that one for a bit now.

@zfi and @VonSzarvas, I am not done testing all the options in C yet, but could still use a reality check on the direction of the blocks.

I think that just having the current SD open file block could lead to confusion if "w" is used because it will overwrite an existing file.

This in mind, there should probably be separate SD create file block that creates an empty text (w) or binary (wb) file and then closes it.

Then, the SD open file could could open in one of the following modes:

• text-read-write (r+)
• text-append (a+)
• text-read-only (r)
• binary-read-write (r+b)
• binary-append (a+b)
• binary-read-only (rb)

I think the binary option could be really useful, but it might also lead to the temptation of mixing binary and text data in a text file. Any suggestions on how to make the binary option available and minimize the coding error risks would be great. For example, is there block smarts that can be incorporated to warn users when they try to mix modes? Would it be better to make separate blocks, some dedicated to text and others dedicated to binary?

P.S. Also, are the separate SD file create and SD file open blocks a viable solution for keeping users from inadvertently deleting their data?

[zfi]

P.S. Also, are the separate SD file create and SD file open blocks a viable solution for keeping users from inadvertently deleting their data?

File creation and truncation are inherent in the SD File Open block. Perhaps we need a new block that breaks out these options visually and then let the code generator emit the correct mode based on the user input. Something like:

(These are just prototypes, warts included at no extra charge)

[AndyLindsay]

I like that! (Warts and all.) Would this still be in addition to a SD create file, or do you think it might be better to take the 'open' and and make it a dropdown with and options? ...or some other approach?

[zfi]

I really don't see why we would need a block the simply creates an empty file. SD open file does that now. Map out an alternative if you don't think this will do.

I should also note that the block I proposed would be a new block and the existing SD open file block would be tagged as deprecated.

[AndyLindsay]

I really think that file creation and file opening/editing need to be separated for BlocklyProp programmers. Sure, C programmers have to learn that fopen doesn't really mean "open" in the file browser sense, and that it could also accidentally nuke all the info they had in an existing file. This is not a "hard lesson" we want to set up for beginning BP programmers.

That is why I am suggesting separate create, open, and close blocks. Under that approach, we would have:

SD file create [filename.txt] type <text, binary>
Emits: fopen("filename.txt", "w"); or fopen("filename.bin", "wb"); followed by fclose(fp); fp = 0;
NOTE: It would be nice if the .txt was changed to .bin when is selected from the dropdown. We would still want the user to be able to change the file extension for their target app.

SD file open "filename.txt/bin" Type <Text, Binary> Mode <Read/Write, Append, Read Only>
Emits: fopen("filename.txt", <"r+", "a+", "r">); or fopen("filename.bin", <"r+b", "a+b", "rb">)
Note: From the BP standpoint, I think that r+ a+ and r (and r+b a+b and rb for binary) are all that are needed to open and modify an existing file. This file may have been created by a PC, or it may have been created by SD file create in a previous step.

SD file close
Emits: if(fp) {fclose(fp); fp = 0;};

[AndyLindsay]

I can see that "Destructive Create" can convey the risk, but from the tutorial classroom standpoint, it's kind of hard to say: "The first step is to create a file. We do that using a block set to 'Destructive Create'." In the classroom setting, that could lead questions that really only have answers related to underlying C nuances.

Now, this is not ideal either, but I do still think it's safer in a tutorial to say: "If you haven't already put text.txt on the SD from your computer, you'll need to use SD file create "test.txt" Type . (Be careful, if you already had test.txt, this will overwrite it, and you will lose any data that was in it.) After creating the file, you can open it and add to it in read/write, add to the end with append, or get data from it with Read Only."