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

Full Usage Example fails on Feather M4 Express #21

Closed
meverett opened this issue Aug 15, 2019 · 20 comments · Fixed by adafruit/SdFat#5
Closed

Full Usage Example fails on Feather M4 Express #21

meverett opened this issue Aug 15, 2019 · 20 comments · Fixed by adafruit/SdFat#5
Assignees
Labels

Comments

@meverett
Copy link

meverett commented Aug 15, 2019

On all versions tested from 3.0.0 to 3.1.1 The "Full Usage Example" is failing on a Feather M4 Express. It's failing on line 88 of the .ino and entering the error conditional there.

When this happens I'm getting some instability in 3.1.1 as well, for example the Serial.println on line 89 would not show up in the serial console sometimes. And other times when I set the LED on pin 13 high before/after line 89 it would not illuminate. When this would happen the board would also appear unresponsive the the Arduino IDE could not longer connect to the port or reset it automatically for uploading a new sketch. At this point only option is to double click reset and upload a new sketch via that route. Maybe this is expected from while(1) in the error condition there but I feel like the answer is "no", otherwise why use this as an error catching technique? Either way I'm getting breaking hangs in this part of the code.

A good bit of the library functionality does work, but I've not yet had a chance to go through and test everything.

Useful Info/Assumptions:

  • I tried testing with 3.0.0 -> 3.1.1; same behavior on all
  • I used the format example from this library to format the flash before running the full usage examples sketch
  • I'm assuming this library requires the Adafruit fork of SdFat as the original library wouldn't even build without some changes to SdFatConfig.h, so that's what I used
  • Prior to breaking changes in 3.0.0 I had not yet had trouble working with the QSPI flash on the M4; this is new behavior
  • I'm using Arduino IDE 1.8.9 on Windows 10
  • Arduino SAMD Boards 1.8.3
  • Adafruit SAMD Boards 1.5.3

Here's what I get printed out on the serial console until it hangs:

Adafruit SPI Flash FatFs Full Usage Example
Flash chip JEDEC ID: 0xC84015
Mounted filesystem!
Creating deep folder structure...
Error, couldn't create deep directory structure!

Initially I wasn't even getting that last error line - the stability when it enters the inside of that error checking conditional seems unpredictable for some reason. In some other example .ino files I was testing I was also not getting Serial output but did verify the code was running by setting pin 13 high. However in the full usage example file it definitely dies inside of that error condition trying to create deep directory structure.

I'm going to test more functionality further and I'll add additional comments if I found more related information. Since fatfs.exists() is used in the example code without issue I'm going to assume the error is related to !fatfs.mkdir("/test/foo/bar") - as it seems testing the existing of and create directories without nesting (/test) isn't an issue and it seems to fail when trying to create nested files/folders.

@hathach hathach self-assigned this Aug 15, 2019
@meverett
Copy link
Author

meverett commented Aug 15, 2019

I put CircuitPython on the M4 via .UF2 to format the flash via OS as it looked like that helped with prior closed issues that I thought might be related. A successful OS flash did not appear to help anything.

Also on the exposed USB drive when running CircuitPython showed /test directory was created, also a boot_out.txt which looks like it's from CircuitPython. Before I formatted it, it had the same contents, but also "foo" but as a file and not as a directory, but as a file (according to Windows Explorer). These seems to indicate that when trying to create "/test/foo/bar" it can create 1 directory "/test" and then the next part "/foo" either fails or gets created as a file and fails.

@meverett
Copy link
Author

The stability of Serial output also seems really bad for some reason when running the examples provided with this library. Some of them seem to run fine in terms of the code, but Serial output just stops and not everything gets printed. As far as I know this behavior is particular to the examples in this library.

@Macrosii
Copy link

I was playing with this last night. Tested on M4 ItsyBitsy and Grand Central. Both had circuit python loaded. . Win7, latest IDE, this lib and Fork of SDfat

The code Locks up on line 88
if ( !fatfs.exists("/test/foo/bar") && !fatfs.mkdir("/test/foo/bar")) {
and never executes the following Serial.print.
Looks like an issue with nested directories, as if i change to
if ( !fatfs.exists("/bar") && !fatfs.mkdir("/bar")) {
It then works (it fails later, but one thing at a time).

@S2Doc
Copy link

S2Doc commented Aug 16, 2019

I'm having the exact same problem. Feather M4 Express

@S2Doc
Copy link

S2Doc commented Aug 16, 2019

Running the same demo on PlatformIO, the problem seems to occur here in Adafruit_FlashCache.cpp, beginning at line 98:

    uint32_t dst_off = cache_addr - addr;
    uint32_t src_off = 0;

    if ( dst_off < 0 )
    {
      src_off = -dst_off;
      dst_off = 0;
    }

    int cache_bytes = min(FLASH_CACHE_SIZE-src_off, count - dst_off);

    // start to cached
    if ( dst_off ) fl->readBuffer(addr, buffer, dst_off);

    // cached
    memcpy(buffer + dst_off, cache_buf + src_off, cache_bytes);

dst_off is an unsigned integer but, when run at the offending portion of the demo, it underflows becoming an inappropriate value. A few lines down, the comparison in the IF statement is never true, so the code to adjust the offsets never gets executed. This causes memcopy() to be called with values outside of existing memory, leading to the crash.

@hathach Would it be possible to fix this?

@Macrosii
Copy link

If i comment out create deep directory, then the code proceeds to create
"/test/test.txt"
Then performs 5 writes to the file. After the last write, writeFile.println(123, HEX);, the code never returns from the writeFile.close();
Does anyone experience the same ?

@Macrosii
Copy link

FYI
I formatted the Flash using the "SdFat_format.ino" (non circuit python) example. I still get the same errors as above!

@jk6011
Copy link

jk6011 commented Aug 20, 2019

Additional observations hopefully to help @hathach track down the problem(s):

  1. Similar to the problems reported for the "Adafruit Feather M4 Express", the "SdFat_full_usage" example also is not working with the "Adafruit Feather nRF52840 Express".

  2. The problem seems related to trying to re-open a file for read after it was already read and then closed.
    For example (assuming: "FatFileSystem fatfs;" and "File readFile;"):

    A. The "SdFat_ReadWrite" example works as written; however, if you copy and paste to the end of the example file another copy of the "read" code that follows the "// re-open the file for reading" line, in order to generate a second read of the same written data, the example fails.

    B. If you only open a file for reading (e.g., readFile = fatfs.open("text.txt")), but do not actually read from it, you can close and re-open the file as many times as you'd like without error. But once you read from the opened file (e.g., using readFile.read() or readFile.readStringUntil('\n')) and close it, you can't re-open it again for reading.

    Question: Is it possible that reading from the file and then closing it inadvertently erases the file?

    C. If you open a file for reading, you can use readFile.seek(0) on the open file to read from it from the beginning as many times as you'd like; however, once the file is closed, it can't be re-opened again for reading.

    D. Using readFile.available() does not appear to cause a problem as does readFile.read().

    E. You can only open the file for read during the same program execution of "SdFat_ReadWrite" where the data was written. That is, if you comment out the read portion of the example code, and only run the write portion of the code to write data to the flash device, and then you uncomment out the read portion, and you comment out the write portion and run the program again, the read will fail.

  3. I had the same problematic results as above when using the "Adafruit Feather nRF52832" with an additional external GD25Q16C chip that I wired separately to the '832 SPI (whereas, the '840 uses QSPI with the on-board GD25Q16C chip).

Additional comments:

  1. Related to meverett's initial comment about instability of Serial.println, I notice that if the program crashes (e.g., for trying to re-open a file for read), that none of the Serial.println() statements in the code that appear prior to the line that causes the crashes will display in the Serial Monitor unless you add "delay(1);" after each Serial.println() statement that appears prior to the line that crashes.

  2. My Tools>Port: COM port selection seems to change at random after uploads. Furthermore, the upload progress display at the bottom of the IDE typically says, "Upgrading target on COMxy", where xy is different from the COM port number I selected in Tools>Port:.

  3. Possibly related to 2 above, about 1 in 10 times I upload, I get the following error: "Couldn't find a Board on the selected port. Check that you have the correct port selected."

  4. I can't close the Serial Monitor and re-open it without performing a new upload.

  5. I have what I believe are the latest of each of: Arduino IDE (v1.8.9), BSP (v0.12.0), Bootloaders (v0.2.11 for both nRF52840 and '832), all the Adafruit Board Drivers for Windows 7, SdFat Adafruit forked library (v1.2.1 from 7/27/19), and Adafruit_SPIFlash library (v3.1.1).

@hathach, please help us!

@hathach
Copy link
Member

hathach commented Aug 20, 2019

Sorry guys, I am currently in the middle of something else. I will look at this in the next week. Meanwhile please make sure you git clone the master branch to test with. There is a fix for flash cache read a couple of week ago that isn't released yet for following bug

https://github.com/adafruit/Adafruit_SPIFlash/blob/master/src/Adafruit_FlashCache.cpp#L98

Running the same demo on PlatformIO, the problem seems to occur here in Adafruit_FlashCache.cpp, beginning at line 98:

    uint32_t dst_off = cache_addr - addr;
    uint32_t src_off = 0;

    if ( dst_off < 0 )
    {
      src_off = -dst_off;
      dst_off = 0;
    }

    int cache_bytes = min(FLASH_CACHE_SIZE-src_off, count - dst_off);

    // start to cached
    if ( dst_off ) fl->readBuffer(addr, buffer, dst_off);

    // cached
    memcpy(buffer + dst_off, cache_buf + src_off, cache_bytes);

dst_off is an unsigned integer but, when run at the offending portion of the demo, it underflows becoming an inappropriate value. A few lines down, the comparison in the IF statement is never true, so the code to adjust the offsets never gets executed. This causes memcopy() to be called with values outside of existing memory, leading to the crash.

@hathach Would it be possible to fix this?

@jk6011
Copy link

jk6011 commented Aug 20, 2019

Thanks, @hathach, for your quick reply. Much appreciated. However, I forgot to mention in my prior post that I had already included your latest Adafruit_SPIFlash master branch (from Aug 5, 2019, 10:12PM PDT) before I listed the problems I mentioned above. Thank you in advance, and we eagerly await your continued help with this issue.

@hathach
Copy link
Member

hathach commented Aug 20, 2019

OK thanks for the update, I will try to reproduce the issue and catch up in the next week.

@yyachim
Copy link

yyachim commented Aug 21, 2019

@hathach Thanks for the link to the upcoming updates for Adafruit_FlashCache.cpp. I was wondering if, in that version, it would still be possible to have out-of-range values passed to 'memcpy' due to the arithmetic for cache_bytes from line 107 (and passed to 'readBuffer' on line 117) due to 'count' being unsigned?

`

int32_t cache_bytes = min((int32_t) (FLASH_CACHE_SIZE-src_off), (int32_t) (count - dst_off)); // line 107

// start to cached
if ( dst_off ) fl->readBuffer(addr, buffer, dst_off);

// cached
memcpy(buffer + dst_off, cache_buf + src_off, cache_bytes);

// cached to end
int copied = dst_off + cache_bytes;
if ( copied < count ) fl->readBuffer(addr + copied, buffer + copied, count - copied); // line 117

`

@hathach hathach added the Bug label Sep 3, 2019
@hathach hathach added this to To do in Next Release Sep 3, 2019
@hathach hathach moved this from To do to In progress in Next Release Sep 3, 2019
@hathach hathach removed this from In progress in Next Release Sep 3, 2019
@hathach hathach added this to To do in Next Release via automation Sep 3, 2019
@hathach hathach moved this from To do to In progress in Next Release Sep 3, 2019
@hathach
Copy link
Member

hathach commented Sep 3, 2019

after doing lots of tests with lots of print for flash caching and sdfat, I couldn't find any issue with those module. Also the example work fine with Feather nRF52840, there is probably issue with QSPI driver implementation on samd51. To be continue ....

@hathach
Copy link
Member

hathach commented Sep 4, 2019

update: spend lots of time to carry more tests, it seems to be related to how the FAT is formatted. The sketch works well when formatted using PC (ubuntu 18.04), but failed if format with example sketch using fatfs. Probably related to 1 vs 2 FAT. Maybe it is not fully covered yet. Continue to test.....

@jk6011
Copy link

jk6011 commented Sep 4, 2019

hathach, thanks for the updates. We're rooting for you to resolve this issue!

Regarding your post yesterday that the issue might be with the QSPI driver, please note that in item 3 in my post above on August 19, I mentioned I had the same problematic results when using the "Adafruit Feather nRF52832" with an additional external GD25Q16C chip that I wired separately to the '832 SPI (whereas, the '840 uses QSPI with the on-board GD25Q16C chip). So I feel the problem might go beyond the QSPI driver. Possibly the problem is formatting with fatfs as you suggested in your post today.

@hathach
Copy link
Member

hathach commented Sep 5, 2019

SdFat assume filesystem always have 2 FAT (which is true for SD Card), but optional for other type of storage (in our case external flash). We did you modification to add 1-FAT support to SdFat but not complete. the SdFat PR will hopefully nail it.

Once 2 above PRs are merge, please give it a test again and close the issue if it is fixed. Thanks

PS: troubleshooting this issue is too problematic ~ . ~

Next Release automation moved this from In progress to Done Sep 5, 2019
@jk6011
Copy link

jk6011 commented Sep 7, 2019

Thanks, @hathach! From my initial tests, the issues I mentioned above appear to be resolved. Great work, and we appreciate your time and effort!

@hathach
Copy link
Member

hathach commented Sep 9, 2019

thank for confirming, we will release the fix for both library asap

@jk6011
Copy link

jk6011 commented Sep 16, 2019

@hathach, while the issue above is still somewhat fresh in your mind, I want to let you know that there might still be a small bug when using SPI with an external flash chip; although, using QSPI with flash seems fine now. When I wire up an external GD25Q16 flash chip to the SPI of an nRF52832, SdFat_ReadWrite prints out two copies of "testing 1, 2, 3." (Note that only one copy of "testing 1, 2, 3." is correctly printed when using the on-board GD25Q16 and QSPI of the nRF52840.)

When using the SPI on the nRF52832 with an external GD25Q16 flash chip, it's also odd that SdFat_full_usage correctly prints only one copy of "Hello world!" as long as it is written to a directory other than the root, such as written to /test/test.txt. However, if you edit SdFat_full_usage to write "Hello world!" to the root directory, i.e., to write to text.txt, rather than to write to /test/test.txt, then two copies will again be printed when using the SPI (whereas, only one copy is correctly printed when using QSPI on the nRF52840).

As I mentioned, the QSPI seems to work fine now with the nRF52840. You previously thought there might have been an issue with the QSPI driver implementation. Is it possible that you recently made some fixes to the QSPI driver that should equivalently be made to the SPI driver?

Thanks in advance for your help with this!

@hathach
Copy link
Member

hathach commented Sep 16, 2019

The code haven't tested with nrf52832 so there is no surprised that it didn't work well. The reason is I don't have an board with nrf52832 with GD25Q16 flash chip to test with since Adafruit didn't make one. If you could figure it out, feel free to submit a PR for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Next Release
  
Done
Development

Successfully merging a pull request may close this issue.

6 participants