sparkfun / OpenLog

Open Source Hardware Datalogger
https://www.sparkfun.com/products/9530
Other
547 stars 215 forks source link

problem command Write file offset #168

Closed marclabouille closed 9 years ago

marclabouille commented 10 years ago

Hi, i have tested the V3.3 for command write file offset, i want modify data on text, but when i enter the new data, the two Carriage return (exit write mode) killed the next data and i have a new line on my text The V3.13 don't have this problem, i don't see the problem on code.

nseidle commented 10 years ago

Additional info from Marclabouille:

I’m using Openlog about 1 years for application write and read data’s ( phone book) for industrials phone system. It’s a very good product. I have downloaded the firmware Openlog V3.3 . ino and program Openlog with Atmel tools. Programming is OK and openlog don’t work with the commands Write file offset. That a means is, when I change 1 or 2 characters on a text the next characters are killed by the CR CR. For example I enter:

new test.txt $0d write test.txt $0d <123456789 $0d read test.txt 123456789 If I want replace 23 by 69 I enter: write test.txt 1 <69 $0d $0d read test.txt 169 6789

It’s very strange. The twice CR killed the next characters.

nseidle commented 10 years ago

I don't immediately know what would cause this. I need to dig back through the command structure to see how we handle CRs in command mode.

nseidle commented 10 years ago

Finally digging into this issue:

The write command is performing as designed. It requires two CRs to exit the write mode. The first one is getting recorded and is causing the overwrite and line break (CR) that you are seeing. There are some ways to get around this but I haven't arrived at a clean solution yet. I am still working on it.

nseidle commented 10 years ago

I think the correct way is to record to the file from the desired position until an escape character is received. This is a bit tricky. The best way would be to re-use the append_file code but that function doesn't support a file location. Perhaps I can add that support and then have the "write" command call the append_file function.

jcdammeyer commented 9 years ago

I followed the same example as above and this is what I discovered. I wrote out: 1234567890 CR I then wrote three ^Z and verified by sending an ls to show that I was in command mode and the file had been written. I then removed the 16GB MicroSD card and looked at it with a file editor on my workstation. In hex mode I see: 31323334 35363738 39300A1A 1A1A The CR character 0x0D has been replaced with a LF 0x0A and then the 3 ^Z characters are written to the file. I believe the 0x0A shouldn't be there since I sent an 0x0D. Yes I know Linux uses a LF for an EOL character but then Apple uses a CR and Microsoft uses the pair in the order of CR and then LF It's even worse when I use the write command to insert characters ABC at position 3. 123ABC

A In hex this is 31323341 42430A0D 410A0D1A 1A 1A

By any description I'd say this code is broken.
First I don't think the 3 ^Z should be in the file. That's a holdover from CP/M and DOS. The FCB now knows where the EOF is and the ^Z isn't needed. And if it's a courtesy to insert it then there should really only be one. Also that ^Z is the EOF termination and shouldn't be part of the file. If I append to the file the first written character should be on top of that ^Z.

Second. If I send a CR I expect to see it in the file. If I send a LF I expect to see it too. And if I send both then obviously both. When I create the exact same file on my Windows system I see the CR LF pair at the end of the file.

The write command has a really odd issue. I can see two characters to terminate the write. But to then insert one LF CR pair when EOL character for append is an LF? And then to write over more of the file with the same character as the first of the write string? I'd guess there's some sort of buffer wrap problem here?

It's a nice little module and I've designed it into a PC board so I want to use them for the next couple of years. But I think these two issues should be fixed. Thanks

jcdammeyer commented 9 years ago

OK. Had to look in the source code to determine what exactly was going on.

  1. There's a bug (with a comment) in the append_file function. //If n is greater than 3 there's a chance here where we receive three esc chars // and then reset the variable: 26 26 26 A T + would not escape correctly if(escape_chars_received == setting_max_escape_character) break; Later in the code workingFile.write(localBuffer, n); //Record the buffer to the card

The count 'n' includes the 3 ^Z plus any characters following them.
if(escape_chars_received == setting_max_escape_character) { n = checkedSpot -3; // don't save the escape characters or anything past them if ((int)n<0) n = 0; // Make sure n didn't wrap. break; } Now when writing the local buffer just check again to make sure n isn't 0 in case the 3 ^Z characters were the only ones in the buffer.

  1. The write command stores fixed size records except for the LF CR ordering/insertion and extra characters but the description of what it's doing is also perhaps a bit vague. The write command appears to be set up to work with a record size file structure and the read location+length command. For example: new datafile.txt
    creates a new empty file.

write datafile.txt 0 1234567890ABCDE CR CR

results in a file that is 20 bytes long. That's a 5 byte overhead. A read with hex output shows what else is written 31 32 33 34 35 36 37 38 39 30 61 62 63 64 65 A D 31 A D

Knowing that each write tags 5 extra bytes makes it possible to write to the position=size of the file to append to it. Write to any multiple of 20 in the above example to randomly write to it.
Read any multiple of 20 size=15 to get the saved data.

However, as in the append_file it's the deliberate closing of the file by writing the double CR characters that screws things up. Start writing at 20 without the CR CR and just a CR to terminate a line inserts just a LF CR pair. The end of the file is mucked up with the extra 3 characters.

An alternative approach is to write N-2 characters terminated with a CR which results in the LF CR insertion. If you want to append to the end of file start writing at file size-3. That writes over the garbage left by the previous CR CR pair and now you have a fixed record size. But even that screws up if you write 15 characters into the middle of the file and terminate the write with the CR CR sequence. If the record size is exactly 20 bytes where you write 18 expecting the LF CR pair at the end then the termination will write a garbage character and an extra LF CR over the beginning of the next.

So I'd say the write command is also badly broken.

nseidle commented 9 years ago

I'm just scratching the surface of this issue. There are multiple things going on. Let's start with some test cases:

new test.txt
append test.txt
1234567890<CR>^Z^Z^Z           //(should exit out)
read test.txt 0 100 2                    //(show file from 0, 100 chars, in hex)
output: 31 32 33 ... 39 30 D 1A 1A 1A          //0x0D is CR, 0x1A is ^Z

This output seems to be correct. I am using teraterm v4.87.

"The CR character 0x0D has been replaced with a LF 0x0A and then the 3 ^Z characters are written to the file."

This is perhaps a limit of your terminal program? I cannot replicate this problem.

"It's even worse when I use the write command to insert characters ABC at position 3."

Forgive me but there is too much going on in this issue to keep it straight. Please start another issue with specific test cases that I can test from a terminal. I really like your ideas about ways to avoid recording escape characters but there are some technical challenges to figure out.

nseidle commented 9 years ago

I believe most of the issues revolve around the write command. If we return to the original issue:

rm test.txt
new test.txt
write test.txt
123456789<enter><enter>
read test.txt 0 100 2
31 32 33 34 35 ... 39 A D       //A is NL, D is CR

It is odd that Openlog is recording a Newline and a carriage return

write test.txt 1
ABC<enter><enter>
read test.txt 0 100 2
31 41 42 43 A D 37 38 39 A D

This looks sort of correct. The NL and CR are being inserted the same as the previous write command. I believe the original poster wants the write command to not insert the 0x0A and 0x0D into the stream. This can be done but it will be tricky to not write two CRs but to write one CR followed by other text.

jcdammeyer commented 9 years ago

It’s been a few months since I looked at this. I did a work-around. I’ve almost used up the first 50 units. I made 100 carrier boards for them so I’ll get getting another 50 or so.

Here’s the thing. In DOS and before that in CP/M-80, a ^Z marked the end of a text file. If you opened the file and did a seek to the end to append to the file the next written character was written over the ^Z and the new text was terminated with the ^Z when the file was closed. You could have three ^Z characters there. It doesn’t matter.

So a file might look like: Hello World! It’s a nice day outside. Do you want to Play? ^Z^Z^Z

Now if I want to append to the file the Line “No it’s cold outside.” The file would look like this.

Hello World! It’s a nice day outside. Do you want to Play? No it’s cold outside. ^Z^Z^Z

IIRC, My testing showed the file looked like this. Hello World! It’s a nice day outside. Do you want to Play? ^Z^Z^Z No it’s cold outside. ^Z^Z^Z

So instead I’d have to seek to the EOF minus 3 characters to get rid of the ^Z.. But then I’m not appending and the writing doesn’t terminate with the ^Z. I think that’s what was happening.

But as I said, that’s a long time ago and the code for it is currently locked down and since it’s for a Homeland Security client I’m not allowed to just change it without the client doing extensive testing.

Thanks John

From: Nathan Seidle [mailto:notifications@github.com] Sent: August-20-15 9:05 AM To: sparkfun/OpenLog Cc: jcdammeyer Subject: Re: [OpenLog] problem command Write file offset (#168)

I'm just scratching the surface of this issue. There are multiple things going on. Let's start with some test cases: new test.txt append test.txt 1234567890^Z^Z^Z //(should exit out) read test.txt 0 100 2 //(show file from 0, 100 chars, in hex) output: 31 32 33 ... 39 30 D 1A 1A 1A //0x0D is CR, 0x1A is ^Z This output seems to be correct. I am using teraterm v4.87. "The CR character 0x0D has been replaced with a LF 0x0A and then the 3 ^Z characters are written to the file." This is perhaps a limit of your terminal program? I cannot replicate this problem. "It's even worse when I use the write command to insert characters ABC at position 3." Forgive me but there is too much going on in this issue to keep it straight. Please start another issue with specific test cases that I can test from a terminal. I really like your ideas about ways to avoid recording escape characters but there are some technical challenges to figure out. — Reply to this email directly or view it on GitHub https://github.com/sparkfun/OpenLog/issues/168#issuecomment-133061497 .Image removed by sender.

jcdammeyer commented 9 years ago

Also to test what is written it's best to use write or append the data and close the file. Then pull the MicroSD card ands mount it in your PC and load it with an editor that can look at the binary values. That's when I discovered there were problems. Writing and reading to the SD card in my application is done with a Delphi application and UART. Not a terminal emulation program from the desktop.

jcdammeyer commented 9 years ago

netclksd-backsideI just checked the application code. The solution was to break the file into smaller pieces. I have one file per minute. So I'd rm the old one. Append the new one and close it when done. That solved the multiple ^Z issue since I never append to an existing file. I have an #ifdef in the code for APPEND verses WRITE and APPEND is enabled.
Simple solution for problem code. The attached photo shows the board it's mounted on. At the far back (not installed) is a WizNet Ethernet module. On the bottom side of the board is a coin cell and RTC. I have one version that runs ACN-E1.31 DMX-512A messaging on Ethernet and the plan was to also put show software on the SD card. This board was done in a hurry because the client forgot to ask for it. Then suddenly there was a deadline that had to be met. A replacement

nseidle commented 9 years ago

So very cool! Thanks for posting the photo. Sounds like you've got some nice work arounds in place.

To be clear, I'm not using an emulator - I am using a serial terminal to send commands directly to OpenLog. Teraterm is just a free/easy to use one that allows distinct CR/NL control.

The ^Z command is intended to drop the user to the command prompt, not to designate the end of a file (sorry for the confusion). The break command (^Z) on OpenLog is configurable to any ASCII character and was picked arbitrarily to be ^Z by me as the default.

The write command relies on the user entering it from the command shell. This means it uses the read_line function which itself relies on searching for \r to signify the end of user input.

if(dataLen < (sizeof(buffer) - 1)) tempFile.write("\n\r", 2); //If we didn't fill up the buffer then user must have sent NL. Append new line and return

We could change the two bytes that are written to the file when using write command but I am worried about breaking compatibility with current users.

I'm going to close this issue but open a new one that discusses methods to avoid recording the three escape characters from the log. I that feature would be more valuable than modifying how the write method works.

JKPina commented 7 years ago

Hi guys, I got #168 issue but I couldn't figure out if it was finally solved, I dived into the openlog firmware version 4, but checked that the piece of code referring to that issue was commented, so: is it a fixed issue or (even better) is a correct way in the uController code to avoid putting a <CR> in the file?

thanks in advance ;)

jcdammeyer commented 7 years ago

I don't know if it was ever fixed. I ordered (and received) another 50 to finish off this production run. Because the board these plug into uses an FTDI device that is only rated 0-70C and a -40C device isn't available in the package I'm using we will need to revise the main board for the next production run. The OpenLog crystal is also only 0-70C and we don't want to rework it with a new crystal.

That's a good enough excuse to fix some of the little issues and recreate the companion board to have a PIC32 for Ethernet, Battery backed up RTC and SD card support. As I said above. I worked around the issue and that interface code is stable.

JKPina commented 7 years ago

I finally found that the best way to avoid escape characters ( <SUB> or <CR><LF> is just a particular escape character) in the end of file is just update the firmware to v4. as @nseidle said in another thread #196

cheers,