MTP Responder Contribution

I renamed a file. Looks like this in the explorer:
2020-11-20 20_45_42-Start.png

Edit: The wrong Character at the end is an ACK (0x06h), the rest of the filename is missing.
2020-11-20 21_03_45-Start.png
 
Last edited:
...at least here is a problem:
Code:
  bool MTPStorage_SD::rename(uint32_t handle, const char* name) 
  { char oldName[256];
    char newName[256];
    [B]char [COLOR=#ff0000]temp[64];[/COLOR][/B]

    uint16_t store = ConstructFilename(handle, oldName, 256);
    Serial.println(oldName);

    Record p1 = ReadIndexRecord(handle);
    strcpy(temp,p1.name);
    strcpy(p1.name,name);

Then, never ever use strcpy for these things if you don't want buffer-overflows.
It just reduces the wait-time for a crash ;)

Use strncpy()

They should be replaced in the code, everywhere.
 
We have strlcpy() on Teensy, which is the same as strncpy() but also takes care to null terminate in some cases where strncpy() doesn't.
 
Yes, a filename size of 64 was selected in original code https://github.com/yoonghm/MTP/blob/master/MTP.h
this is not the absolute filename that gets constructed for disk access only the filename inside a directory, which is limited to 256 characters.
Will introduce a check , but not sure what to do if someone insists on using very long filenames
 
Last edited:
No, but they were the reasons for the crashes I had. My MP3s tend to have very long names :) and they were on the SD-Card. It was enough that they existed. I guess something in the RAM got overriden.


On the other hand, all the rest of the Code (SdFat) uses 256. So why restrict here.

Esp uses a lot of malloc() for these things. A way to reduce Stack-usage.
In code like this, fragmentation is no issue, too, as all mem can get free()d immedeatly.

The strlcpy() Paul mentioned is good, too.
 
On the other hand, all the rest of the Code (SdFat) uses 256. So why restrict here.
For the whole filename including multiple directories.

Yes, I have seen people that put a lot of metadata into a filename

And I do not like these filenames

Anyhow, it is the record definition that limits the code, and can be changed.
 
I fear, it does not matter wether we like it or not.
(And no, i did not name the Mp3s this way ;)

Anyway, it does not hurt much to accept more than 64.
And it hurts even less to add secure functions like strlcpy.
 
Code:
  uint16_t MTPStorage_SD::ConstructFilename(int i, char* out, int len) // construct filename rexursively
  {
    Record tmp = ReadIndexRecord(i);
      
    if (tmp.parent==(unsigned)i) 
    { strcpy(out, "/");
      return tmp.store;
    }
    else 
    { ConstructFilename(tmp.parent, out, len);
      if (out[strlen(out)-1] != '/') strcat(out, "/");
      if(((strlen(out)+strlen(tmp.name)+1) < (unsigned) len)) strcat(out, tmp.name);
      return tmp.store;
    }
  }
This should use strlcat(). And i'm not sure if GCC is smart enough to re-use the computed length of strln(out) (in the else {..})
If not, it may be better to do the strlen(out) once and save it in a temp variable.
 
Just to jump in here for a minute.

As Frank and Paul said it would probably be safer in the long run would probably be better to use strlcpy().

As for filename length whatever gets decided probably should make sure they are all compatible between classes, SDFat, FS, LittleFS, and MTP. I would think.

As for USB interface was poking around usb_desc.h and .cpp and did notice that number of endpoints are different between the 2 for MTP. In desc.c its defined as 3 and in desc.h its defined as 4. Tried making them the same but.. as a test I changed it to 4 in the desc.c but then the Teensy as a device wouldn't show up. If I change it to 3 in desc.h I can copy those 2 jpgs to RAM, qspi, SD Card and the winbond flash on the breakout but it fails to copy to propshield flash and basically hangs MTP and I have to restart the T4.1
 
Just tried a T3.6 with MTP responder and LittleFS well strange things are happening. Basically have 2 SD cards and 2 SPIFlahsh attached (propshield and breakout).

1. All storage devices are found except when I try to use the SPI breakout it will not recognize the chip and Teensy does not show in Explorer. If I open device manager it shows as MTP USB device. Only works with the propshield.
2. If I copy 2 jpgs >300k to the propshield (which fails on the T4.1) it does copy it but if I cycle power the files I copied to the propshield are not there.
 
Just to jump in here for a minute.

As Frank and Paul said it would probably be safer in the long run would probably be better to use strlcpy().

As for filename length whatever gets decided probably should make sure they are all compatible between classes, SDFat, FS, LittleFS, and MTP. I would think.

As for USB interface was poking around usb_desc.h and .cpp and did notice that number of endpoints are different between the 2 for MTP. In desc.c its defined as 3 and in desc.h its defined as 4. Tried making them the same but.. as a test I changed it to 4 in the desc.c but then the Teensy as a device wouldn't show up. If I change it to 3 in desc.h I can copy those 2 jpgs to RAM, qspi, SD Card and the winbond flash on the breakout but it fails to copy to propshield flash and basically hangs MTP and I have to restart the T4.1


-No problem to use the strlxxx vestions
-No problem to use
Code:
  #define MAX_FILENAME_LEN 256
definition in SD.h
Need only to decide what to do if combination of file path components exceed this limit.
- On USB: maybe there is a mis-undestanding desc.c indicates the number of entrypoind and there are 3 RX,TX,Event, while desc.h indicates RX and TX use channel 4 and Event use channel 5
 
Wow, i'm pretty impressed by GCC.
I played a bit with godbolt, and indeed it is smart enough to re-use the result of strlen().
Even if you add a const string with strcat inbetween - it calls strlen only once.
However, if you use strlcat instead, it can't. Ok, that's because strlcat is no inbuilt gcc function.


(just wanted to mention that gcc is really clever sometimes)
(edit: p.s. and it does not call a function for strcat... wow)
 
I updated the MTP repository (strlxxx, MAX_FILENEME_LEN = 256; also features and limitations in Readme)
 
- On USB: maybe there is a mis-undestanding desc.c indicates the number of entrypoind and there are 3 RX,TX,Event, while desc.h indicates RX and TX use channel 4 and Event use channel 5

Thanks for the explanation. Never really looked at those 2 files before so something new for me, again.
 
I've added a PR that adds usage of the real Teensy serialnumber instead of the fixed #define MTP_SERNR "1234"
2020-11-21 15_17_43-Start.png
Was quite easy:
Code:
    char buf[11];        // serial
    for (size_t i=0; i<10; i++) buf[i] = usb_string_serial_number.wString[i];
    writestring(buf);
Should, in theory, work with all Teensy-models. Tested with T4.1 only.
This is all fun.. great field to play..
 
Last edited:
@Frank - Nice!

@all
I started playing some more with the why large files are failing on my one T4.1... So started adding some debug outputs in Storage.cpp to see if I get an idea.

Example in the Create:
Code:
  uint32_t MTPStorage_SD::Create(uint32_t storage, uint32_t parent,  bool folder, const char* filename)
  {
    uint32_t ret;
    #if DEBUG==1
      Serial.printf("SCR: %s\n",filename);
    #endif

    if (parent == 0xFFFFFFFFUL) parent = storage-1;
    Record p = ReadIndexRecord(parent);
    Record r;
[COLOR="#FF0000"]    if (strlen(filename) > 62) return 0;[/COLOR]
    strcpy(r.name, filename);
But also was wondering about that hard coded 62?

I also added to write:
Code:
  void MTPStorage_SD::write(const char* data, uint32_t bytes)
  {
    #if DEBUG==1
      Serial.printf("  SW: %u\n",bytes);
    #endif
And close:

Code:
  void MTPStorage_SD::close() 
  {
    #if DEBUG==1
      Serial.printf("SCL - start");
    #endif
    mtp_lock_storage(true);
    uint32_t size = (uint32_t) file_.size();
    file_.close();
    mtp_lock_storage(false);
    //
    // update record with file size
    Record r = ReadIndexRecord(open_file_);
    r.child = size;
    WriteIndexRecord(open_file_, r);
    open_file_ = 0xFFFFFFFEUL;
    #if DEBUG==1
      Serial.printf("SCL - complete\n");
    #endif
  }
Note: on close the first run I had missed the last \n...
I had a run that I tried two files. A smaller one, which worked and the one that failed.

The last parts of the output:
Code:
100c 20 1 70: 4 ffffffff ffffffff
SCR: IMG_1255.jpg
2001 24 3 70: 4 ffffffff b
100d 12 1 71: 0 3000 177b5
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 6069
SCL - startSCL - complete2001 12 3 71: 0 aa65d7bb 3fad00ff
9803 20 1 72: b dc02 3fad00ff
2001 20 3 72: b dc02 3fad00ff
9803 20 1 73: b dc01 3fad00ff
2001 20 3 73: b dc01 3fad00ff
9803 20 1 74: b dc07 3fad00ff
2001 20 3 74: b dc07 3fad00ff
9802 20 1 75: dc0b 3000 3fad00ff
2001 20 3 75: dc0b 3000 3fad00ff
9803 20 1 76: b dc0b 3fad00ff
2001 20 3 76: b dc0b 3fad00ff
9803 20 1 77: b dc41 3fad00ff
2001 20 3 77: b dc41 3fad00ff
9803 20 1 78: b dc44 3fad00ff
2001 20 3 78: b dc44 3fad00ff
9803 20 1 79: b dc03 3fad00ff
2001 20 3 79: b dc03 3fad00ff
9803 20 1 80: b dc04 3fad00ff
2001 20 3 80: b dc04 3fad00ff
1008 16 1 81: b dc04 3fad00ff
2001 16 3 81: b dc04 3fad00ff
1005 16 1 82: 4 dc04 3fad00ff
2001 16 3 82: 4 dc04 3fad00ff
100c 20 1 83: 4 ffffffff 3fad00ff
SCR: T4-Cardlike.jpg
2001 24 3 83: 4 ffffffff c
100d 12 1 84: 0 3000 5321e
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
  SW: 8192
Quick count is about 26 writes called here. * 8192 would be: 212992 bytes received from the 332kb file

Will see if anything look obvious, probably next to check to see if it is stuck in the write or ...
 
@Frank B - yes it is :) Fun..... I think

Anyway been doing a bit more experimenting with MTP and the different storage areas and what transfers fail, where and when. There are three files that were problem children 2 JPEGS and 1 specific PDF. I was able to transfer the files to all storage media except for the FLASH on the PROPSHIELD for some strange reason. When it failed to transfer I would loose the PROPSHIELD in the Windows Explorer (sometimes) but in all cases when the transfer error occurred MTP would fail to show files on any of the other meda.

Oh btw I also changed the clock on QSPI to 132 Mhz as a separate test and it worked at the higher clock as well.

If someone wants to give it a try. Here is the sketch I am using Once you set up your config you can select what storage areas you want with the defines at the top of the sketch. I will also attach the pdf i am using to test

Code:
#include "Arduino.h"

#include "SD.h"
#include "MTP.h"
#include "usb1_mtp.h"

#define HAVE_LITTLEFS 1 // set to zero if no LtttleFS is existing or to be used
#if HAVE_LITTLEFS==1
  #define USE_RAM 1
  #define USE_SPI 1
  #define USE_QSPI 1
  #define USE_HS_QSPI 1
#endif

/****  Start device specific change area  ****/

  // edit SPI to reflect your configuration (following is fot T4.1)
  #define SD_MOSI 11
  #define SD_MISO 12
  #define SD_SCK  13

  #define SPI_SPEED SD_SCK_MHZ(16)  // adjust to sd card 

// SDClasses
  const char *sd_str[]={"sdio"}; // edit to reflect your configuration
  const int cs[] = {BUILTIN_SDCARD}; // edit to reflect your configuration
  const int nsd = sizeof(cs)/sizeof(int);

SDClass sdx[nsd];

//LittleFS classes
#if HAVE_LITTLEFS==1
  #include "LittleFS.h"
  #if USE_RAM == 1
    const char *ram_str[]={"RAM0", "RAM1"};     // edit to reflect your configuration
    const int ram_size[] = {2'000'000, 4'000'000};
    const int ram_nsd = sizeof(ram_size)/sizeof(int);
    LittleFS_RAM ramfs[ram_nsd]; // needs to be declared if LittleFS is used in storage.h
  #endif
  
  #if USE_SPI == 1
    const char *spi_str[]={"PROPSHIELD", "WINBOND"};     // edit to reflect your configuration
    const int spi_cs[] = {6, 10}; // edit to reflect your configuration
    const int spi_nsd = sizeof(spi_cs)/sizeof(int);
    LittleFS_SPIFlash spifs[spi_nsd]; // needs to be declared if LittleFS is used in storage.h
  #endif
  
  #if USE_QSPI == 1
    const char *qspi_str[]={"QSPI0"};     // edit to reflect your configuration
    const int qspi_nsd = 1;
    LittleFS_QSPIFlash qspifs[qspi_nsd]; // needs to be declared if LittleFS is used in storage.h
  #endif
 
#endif

MTPStorage_SD storage;
MTPD       mtpd(&storage);


void storage_configure()
{
    #if defined SD_SCK
      SPI.setMOSI(SD_MOSI);
      SPI.setMISO(SD_MISO);
      SPI.setSCK(SD_SCK);
    #endif

    for(int ii=0; ii<nsd; ii++)
    { if(cs[ii] == BUILTIN_SDCARD)
      {
        if(!sdx[ii].sdfs.begin(SdioConfig(FIFO_SDIO))) {Serial.println("No storage"); while(1);};
        storage.addFilesystem(sdx[ii], sd_str[ii]);
      }
      else if(cs[ii]<BUILTIN_SDCARD)
      {
        pinMode(cs[ii],OUTPUT); digitalWriteFast(cs[ii],HIGH);
        if(!sdx[ii].sdfs.begin(SdSpiConfig(cs[ii], SHARED_SPI, SPI_SPEED))) {Serial.println("No storage"); while(1);}
        storage.addFilesystem(sdx[ii], sd_str[ii]);
      }
        uint64_t totalSize = sdx[ii].totalSize();
        uint64_t usedSize  = sdx[ii].usedSize();
        Serial.printf("Storage %d %d %s ",ii,cs[ii],sd_str[ii]); Serial.print(totalSize); Serial.print(" "); Serial.println(usedSize);
    }
    #if HAVE_LITTLEFS==1
      #if USE_RAM == 1
        for(int ii=0; ii<ram_nsd;ii++)
        {
          { if(!ramfs[ii].begin(ram_size[ii])) { Serial.println("No storage"); while(1);}
            storage.addFilesystem(ramfs[ii], ram_str[ii]);
          }
          uint64_t totalSize = ramfs[ii].totalSize();
          uint64_t usedSize  = ramfs[ii].usedSize();
          Serial.printf("Storage %d %s ",ii,ram_str[ii]); Serial.print(totalSize); Serial.print(" "); Serial.println(usedSize);
        }
      #endif

      #if USE_SPI == 1
        for(int ii=0; ii<spi_nsd;ii++) {
          pinMode(spi_cs[ii],OUTPUT); digitalWriteFast(cs[ii],HIGH);
          if(!spifs[ii].begin(spi_cs[ii], SPI)) {Serial.println("No storage"); while(1);}
          storage.addFilesystem(spifs[ii], spi_str[ii]);
          
        uint64_t totalSize = spifs[ii].totalSize();
        uint64_t usedSize  = spifs[ii].usedSize();
        Serial.printf("Storage %d %d %s ",ii,spi_cs[ii],spi_str[ii]); Serial.print(totalSize); Serial.print(" "); Serial.println(usedSize);
        }
      #endif

      #if USE_QSPI == 1
       for(int ii=0; ii<qspi_nsd;ii++) {
          if(!qspifs[ii].begin()) {Serial.println("No storage"); while(1);}
          storage.addFilesystem(qspifs[ii], qspi_str[ii]);
     
          uint64_t totalSize = qspifs[ii].totalSize();
          uint64_t usedSize  = qspifs[ii].usedSize();
          Serial.printf("Storage %d %s ",ii,qspi_str[ii]); Serial.print(totalSize); Serial.print(" "); Serial.println(usedSize);
        }
      #endif
      
    #endif
}
/****  End of device specific change area  ****/

  // Call back for file timestamps.  Only called for file create and sync(). needed by SDFat-beta
   #include "TimeLib.h"
  void dateTime(uint16_t* date, uint16_t* time, uint8_t* ms10) 
  { *date = FS_DATE(year(), month(), day());
    *time = FS_TIME(hour(), minute(), second());
    *ms10 = second() & 1 ? 100 : 0;
  }

void logg(uint32_t del, const char *txt)
{ static uint32_t to;
  if(millis()-to > del)
  {
    Serial.println(txt); 
    to=millis();
  }
}

void setup()
{ 
  while(!Serial); 
  Serial.println("MTP_test");
  
  // Set Time callback // needed for SDFat-beta
  FsDateTime::callback = dateTime;

  #if USE_HS_QSPI == 1
    //Reset clock to 132.9 Mhz
    CCM_CCGR7 |= CCM_CCGR7_FLEXSPI2(CCM_CCGR_OFF);
    CCM_CBCMR = (CCM_CBCMR & ~(CCM_CBCMR_FLEXSPI2_PODF_MASK | CCM_CBCMR_FLEXSPI2_CLK_SEL_MASK))
      | CCM_CBCMR_FLEXSPI2_PODF(4) | CCM_CBCMR_FLEXSPI2_CLK_SEL(2); 
    CCM_CCGR7 |= CCM_CCGR7_FLEXSPI2(CCM_CCGR_ON);
  #endif

  usb_mtp_configure();
  storage_configure();

  #if HAVE_LITTLEFS==1
  #if USE_RAM
  // store some files into disks (but only once)
  for(int ii=0; ii<10;ii++)
  { char filename[80];
    sprintf(filename,"test_%d.txt",ii);
    if(!ramfs[0].exists(filename))
    {
      File file=ramfs[0].open(filename,FILE_WRITE_BEGIN);
        file.println("This is a test line");
      file.close();
    }
  }
  #endif
  #endif
  const char *str = "test1.txt";
  if(sdx[0].exists(str)) sdx[0].remove(str);
  File file=sdx[0].open(str,FILE_WRITE_BEGIN);
      file.println("This is a test line");
  file.close();

  Serial.println("\n**** dir of sd[0] ****");
  sdx[0].sdfs.ls();
  Serial.println();

  Serial.println("Setup done");
  Serial.flush();
}

void loop()
{ 
  mtpd.loop();

  //logg(1000,"loop");
  //asm("wfi"); // may wait forever on T4.x
}
 

Attachments

  • Visio-driver.pdf
    1.4 MB · Views: 53
I've added a PR that adds usage of the real Teensy serialnumber instead of the fixed #define MTP_SERNR "1234"
View attachment 22560
Was quite easy:
Code:
    char buf[11];        // serial
    for (size_t i=0; i<10; i++) buf[i] = usb_string_serial_number.wString[i];
    writestring(buf);
Should, in theory, work with all Teensy-models. Tested with T4.1 only.
This is all fun.. great field to play..

I only replaced the original "?", but was planning to use the one you are mentioning, at a later time
 
SPICONFIG ????
On the off chance that the SPI Clock may need to increase I looked at what is setup in SerialFlash:
Code:
#define SPICONFIG   SPISettings(50000000, MSBFIRST, SPI_MODE0)

In LittleFS its setup as
Code:
#define SPICONFIG   SPISettings(30000000, MSBFIRST, SPI_MODE0)
So as an experiment I tried to increase the to 36Mhz. But still can not copy large files to the Propshield. Increased to 44Mhz and still can not transfer the large files to the Propshield flash. Out of curiosity I tried at 24 MHz as well.

EDIT: Since I also tested on a T3.6 wondering if its not an issue with the Propshield Flash and large files?
 
Code:
  uint32_t MTPStorage_SD::Create(uint32_t storage, uint32_t parent,  bool folder, const char* filename)
  {
    uint32_t ret;
    #if DEBUG==1
      Serial.printf("SCR: %s\n",filename);
    #endif

    if (parent == 0xFFFFFFFFUL) parent = storage-1;
    Record p = ReadIndexRecord(parent);
    Record r;
[COLOR="#FF0000"]    if (strlen(filename) > 62) return 0;[/COLOR]
    strcpy(r.name, filename);
But also was wondering about that hard coded 62?

The 62 is a residual of previous 64 byte limitation
delete this line and replace next line
with
Code:
 strlcpy(r.name, filename,MAX_FILENAME_LEN);
 
@WMXZ - Thanks, the hard coded one is not hurting me... Just thought I would mention as I saw it.

Now digging a little at the next level. (usb2) -

Added some digital Writes/toggles in a few places in usb1_mtp.c
33 - usb_mtp_configure
34 usb_mtp_wait

35 usb_mtp_recv
36 usb_mtp_send

39 toggle when wait errors out.

First pass: I did not see any error outs:
screenshot.jpg

The interesting thing so far is up till it failed each of the groups were something like: 16 reads in a cluster and then gap of time.
screenshot2.jpg

But at the end there was only one read/wait... Again may not mean anything, but...
 
@KurtE
yes, I was playing some time with this USB.
Without an USB analyser it is becoming extremely difficult (there are a lot of possibilities)
Maybe by comparing again the code with Paul's different implementations could help.
At the moment I fighting with copying directories from one disk to another. I'm close but not there yet.
 
@KurtE
Went back just to look at the copy behavior when it transfers to the BreakOut board, specifically, with sending data in groups. If I watch the progress bar during the transfer of the 1.5Mb PDF it looks like it transfers some data, pauses and then transfers again. This occurs may 4-5 times total until transfer is complete. Its not a smooth transfer like when transferring to SD Card or QSPI.
 
Nice to see it coming along and having great general function for four of you on multiple boards!

I saw success with my last post and then was otherwise engaged yesterday with a bit more ahead today.

Are you still using Seremu or have you made a transition to normal USB Serial? I saw notes on that ... but was only glancing at the many posts between other tasks.

Using Seremu after Serial goes true I put in a while( !Serial.available() ); that stalled until TyComm came online and sent a character. That was a quick test and I may have done a single mtp.loop() in that case to make sure the MTP code got started after all the setup for it was done. Then I didn't miss any Serial output.

Last night I did do a Device Mgr 'Portable Devices' : "Disable device" then "Enable device" and got a current content refresh on Teensy.

In that list I see 'Teensy' and "Teensy8A" - but only one powered teensy just now?
 
Back
Top