MTP Responder Contribution

[hubbe]

While working on TeensySaber, I felt a need to access the storage without having to to pull out the sd card and put it in an sd card reader.
Since the promised MTP responder seems to be taking a while, I wrote one myself.
It requires applying a small patch to the teensy core libraries to work, but I hope Paul will incorporate this code into the teensy distribution to make it easy for everybody to use.
All code attached below, enjoy.

[hubbe]

Oh, I forgot: To try it out you also need to add entries to your boards.txt file to make "MTP Disk" show up in the "USB Type" menu.
The entries you need looks like this:

teensy36.menu.usb.mtp=MTP Disk
teensy36.menu.usb.mtp.build.usbtype=USB_MTPDISK
teensy36.menu.usb.mtp.fake_serial=teensy_gateway

(Replace 36 with 35, 31, 30 and LC as needed.)

[Frank B]

Wow, that looks very interesting.
Do you intend to make a library from that ?

[PaulStoffregen]

Will look at this soon! Thanks.

[hubbe]

Wow, that looks very interesting.
Do you intend to make a library from that ?

It's intended to be a library. Not sure how you make one though.
Also not sure if makes sense for me to do it or someone else (Paul?) as I'm probably not going to put a lot of effort into this in the future.
(It already works well enough for my purposes.)

[yoonghm]

I did this:

1. Updated the two file as in https://github.com/PaulStoffregen/cores/pull/230
2. Updated menu and built the code with USB_MTPDISK

Below are my observation

1. A lot of warning on "warning: dereferencing type-punned pointer will break strict-aliasing rules" on de-referencing on CONTAINER
2. A 32 GB microSD wass shown as 1.00 GB microSD with 512 MB free.
3. The microSD was shown empty but it was not.
4. Could not write anything into the microSD

Did I miss any step?

[tni]

[*] The microSD was shown empty but it was not.

You probably need to change the SPI pin. The sketch is set up for an external card slot.

I could write to the SD, but large file transfers hang with the MTP device being unresponsive (Teensy 3.6 SDIO slot, Windows 7).

[hubbe]

1. I'm not sure what the right way to work around those warnings is.
2. The SD library doesn't expose the size and free space of the SD card, so currently the code always says the card is 1Gb with 512Mb free.
3. Make sure the SD.begin() call matches the whatever pin you use as SD_EN (BUILTIN_SDCARD if you're using teensy 3.5/3.6)
4. (See 3)

I could write to the SD, but large file transfers hang with the MTP device being unresponsive (Teensy 3.6 SDIO slot, Windows 7).

This sounds suspiciously like what happened before I implemented the "Get Device Status" control message. (0x67A1).
I guess windows 7 does something slightly different than windows 10... Like maybe it uses 0x6721 instead?
I don't have time to test it myself right now as I'm preparing to go on vacation. (Also, I'm not sure I have a working
Win7 machine anymore...)

[MichaelMC]

With a few changes I got this working on a T36 with the built-in SD slot.
Replaced
#include "sd.h"
with
#include "SdFat.h"
SdFatSdioEX SD;

Increased the .name field of the Record struct from 14 to 64 bytes, and then replaced
strcpy(r.name, child.name); ~line 203
with
child.getName(r.name, 64);

change the 12 in (~line 336):
if (strlen(filename) > 12) return 0;
to 62

Remove the 10 from: SD.begin(10)


After this it all looks and works faultlessly (apart from volume space reporting). I'm seeing transfer speeds of around 1Megabyte per second, which is inline with the port type. There's no lag or stuttering of any kind when accessing or copying small or large (using Win7).

Nice work.

[MichaelMC]

For correct size and free space replace size() and free() (~line 100) with:

  uint64_t size(){
  	 return (uint64_t)512 * (uint64_t)SD.clusterCount() * (uint64_t)SD.vol()->blocksPerCluster();
  }
  
  uint64_t free() {
	 uint64_t volFree = SD.vol()->freeClusterCount();
  	 uint64_t ssize = (uint64_t)512 * volFree * (uint64_t)SD.vol()->blocksPerCluster();
  	 return ssize;
  }

and replace: ~line 377
uint32_t size = f_.size();
with
uint64_t size = f_.size();

Total size and free space/used space are now reporting correctly for me with an 8gb card.

[hubbe]

Nice, I'll have to take a look at SdFat.h, seems better than SD.h.

[MichaelMC]

To fix the aliasing problem:

Replace ~line 621
#define CONTAINER ((struct MTPContainer*)(receive_buffer->buf))

with

inline MTPContainer *contains (usb_packet_t *receive_buffer){
	return (MTPContainer*)(receive_buffer->buf);
}
#define CONTAINER contains(receive_buffer)

[tni]

This sounds suspiciously like what happened before I implemented the "Get Device Status" control message. (0x67A1).
I guess windows 7 does something slightly different than windows 10... Like maybe it uses 0x6721 instead?

I tried adding 0x6721 (the response would be the same, right?) - doesn't help.

Nice, I'll have to take a look at SdFat.h, seems better than SD.h.

It's much faster. With Teensy 3.6, SdFat-beta can do 19MB/s (not over FS USB of course).

[yoonghm]

This is wonderful. MichealMC@ could you post the complete code of your changes? Thanks

[hubbe]

To fix the aliasing problem:

Replace ~line 621
#define CONTAINER ((struct MTPContainer*)(receive_buffer->buf))

with

inline MTPContainer *contains (usb_packet_t *receive_buffer){
	return (MTPContainer*)(receive_buffer->buf);
}
#define CONTAINER contains(receive_buffer)

Can you explain why this is better than just using a define? Or is it just hiding the aliasing so that the compiler can't see it?

[yoonghm]

If you cast a pointer to a different kind of pointer and then dereference it, then your program has executed an undefined behavior.

The in-line function returns a pointer to a real MTPContainer object instead of relying on casting. Some solutions offered in stackoverflow uses memcpy() to copy the data into a real object, but this process is slower. The solution here is superb.

[hubbe]

If you cast a pointer to a different kind of pointer and then dereference it, then your program has executed an undefined behavior.

The in-line function returns a pointer to a real MTPContainer object instead of relying on casting. Some solutions offered in stackoverflow uses memcpy() to copy the data into a real object, but this process is slower. The solution here is superb.

If I'm reading the standard right, this is still undefined behavior, the compiler just doesn't complain about it anymore.
As far as I can tell, casting to a struct and then using is no longer supported in C++-11, so there goes 39 years of backwards compatibility....
Anyways, it looks like unions is the new hotness, which is silly, because it ends up doing the same thing.

[tni]

The aliasing is a time bomb. E.g.:

struct A {
    int s;
    int i;
};

struct B {
    int s;
    char buffer[4];
};

bool test(A& a, B& b) {
    b.buffer[0] = 1;
    a.i = 0;
    return a.i == b.buffer[0];
}

GCC 5.4 leaves the "a.i == b.buffer[0]" check in place. GCC 6.3 concludes that "a" and "b" cannot alias and optimizes the check away and produces the wrong result.

If I'm reading the standard right, this is still undefined behavior, the compiler just doesn't complain about it anymore.

Yes.

As far as I can tell, casting to a struct and then using is no longer supported in C++-11, so there goes 39 years of backwards compatibility....

It wasn't legal in the first place. This is not something new with C++-11.

Anyways, it looks like unions is the new hotness, which is silly, because it ends up doing the same thing.

Using a union for type punning is not legal in C++ (it is in C99 / C11).

GCC, CLang and ICC all support "__attribute((__may_alias__))", which gets you the correct behavior. E.g.:

 
struct A {
    int s;
    int i;
} __attribute((__may_alias__));

[yoonghm]

I have converted the file into library. The library could be downloaded from

https://github.com/yoonghm/Teensy_3..../Libraries/MTP

This library requires the following changes
* SdFat libraries - https://github.com/greiman/SdFat-beta/tree/master/SdFat
* <Arduino_path>/hardware/teensy/avr/cores/teensy3/usb_desc.c
* <Arduino_path>/hardware/teensy/avr/cores/teensy3/usb_dev.c
* <Arduino_path>/hardware/boards.txt

[Po Ting]

hello, gonna try this soon!
but wondering already,

is it possible to run my other program, after waiting for a USB response in, maybe 2 seconds?
or switch to a MTP device if plugged.

or is the mtpd.loop() a must to be called continuously for the MTP to work?

thanks

[hubbe]

The best way to run something else is to put it in mtp_yield();

[PaulStoffregen]

I've uploaded Teensyduino 1.37-beta1, with the USB patch and "MTP Disk (Experimental)" in the Tools > USB Type menu.

https://forum.pjrc.com/threads/44546...no-1-37-Beta-1

Please give 1.37-beta1 a try and let me know how it works for you?

[murpia]

Many thanks for all the great contributions! Here's what I did to get a working setup for MTP in case anyone needs guidance:

Install Teensyduino 1.37 Beta #1 over the top of 1.36 (Arduino IDE 1.8.2).
Copy folder 'MTP' from here https://github.com/yoonghm/Teensy_3..../Libraries/MTP into my local library folder.
Copy folder 'SdFat' from here https://github.com/greiman/SdFat-beta/tree/master/SdFat into my local library folder.
In Arduino IDE navigate to File->Examples->(Examples from custom libraries) MTP->MTP_blinky.ino.
In Arduino IDE navigate to Tools and ensure you have 'Board: Teensy 3.6', 'USB Type: MTP Experimental' (and I used 180MHz, Fastest)

Regards, Ian

[hubbe]

I've uploaded Teensyduino 1.37-beta1, with the USB patch and "MTP Disk (Experimental)" in the Tools > USB Type menu.

https://forum.pjrc.com/threads/44546...no-1-37-Beta-1

Please give 1.37-beta1 a try and let me know how it works for you?

Sorry for the delay.
I'm trying this out, and the MTP part seems to work, but the serial monitor seems to stop working.
I don't think the problem is on my side, because even if I don't enable the MTP responder in my code, the serial monitor still doesn't work.
I also tried switching to "MIDI", and then the serial monitor does work. I haven't figured out what the problem is yet, but I'll keep digging.

[murpia]

See here for how to create a composite MTP and CDC device, which sounds to me like what you need:

https://forum.pjrc.com/threads/44569...l=1#post145192

Regards, Ian