vtable problem

Status
Not open for further replies.

WMXZ

Well-known member
I have the following problem
this code (a minimal, single file version of the MTP-responder)
Code:
#include "SD.h"

class mSD_Base
{ 
public:
  mSD_Base(FS *sd) {sdx=sd;}

  File sd_open(uint32_t store, const char *filename, uint32_t mode) 
  { if(!cs || (cs[store]<256)) return ((SDClass *)&sdx[store])->open(filename,mode); 
    else return 0;
  }
private:
  FS *sdx;
  const int *cs = 0;
};

class MTPStorageInterface {
public:
  virtual void OpenFile(char *filename, uint32_t mode = FILE_READ)=0;
};

class MTPStorage_SD : public MTPStorageInterface, mSD_Base { 
public:
  MTPStorage_SD(FS *fs): mSD_Base(fs) {}

  void OpenFile(char *filename, uint32_t mode = FILE_READ) {sd_open(0,filename,mode);}
};

class MTPD {
public:
  explicit MTPD(MTPStorageInterface* storage): storage_(storage) {}

  void open(char *filename){storage_->OpenFile(filename);}

private:
  MTPStorageInterface* storage_;
};

void setup() {
  // put your setup code here, to run once:

  FS sdx[1];
  MTPStorage_SD storage(sdx);
  MTPD       mtpd(&storage);

  mtpd.open((char *)"test");
}

void loop() {
  // put your main code here, to run repeatedly:

}

generates this Link error
Code:
C:\Users\zimmer\Documents\Arduino\TestVtable/TestVtable.ino:47: undefined reference to `vtable for FS'

Not sure how to solve this problem
 
Can't reproduce this - can you provide the details needed to compile this - software version, teensy model, etc.
 
Can't reproduce this - can you provide the details needed to compile this - software version, teensy model, etc.

Simply copy all into Arduino IDE and compile (You need the latest TD beta with new SD library and FS.h) I have it on T4.1
Anyhow, It seems that the Cast
Code:
((SDClass *)&sdx[store])
is the culprit.

If you have not followed the recent SD discussion, FS.h is an interface class and SD is the implementation for SDFat-beta file system

I will try to reduce again (get rid of SD.h)
 
So,
the following program works
Code:
class FS
{
public:
  FS() {}
  virtual int open(const char *filename, uint8_t mode = 0);
};

class SDClass : public FS
{
public:
  SDClass() { }
  int open(const char *filename, uint8_t mode = 0) {
    Serial.print(filename); Serial.print(" ");
    Serial.println(mode);
    return 1;    
  }
};

class mSD_Base { 
public:
  mSD_Base(FS *sd): sdx(sd) {}

  int sd_open(uint32_t store, const char *filename, uint32_t mode) 
  { if(!cs || (cs[store]<256)) return ((SDClass *)&sdx[store])->open(filename,mode); 
    else return 0;
  }
private:
  FS *sdx;
  const int *cs = 0;
};

class MTPStorageInterface {
public:
  virtual int OpenFile(char *filename, uint32_t mode = 0)=0;
};

class MTPStorage_SD : public MTPStorageInterface, mSD_Base { 
public:
  MTPStorage_SD(FS *fs): mSD_Base(fs) {}

  int OpenFile(char *filename, uint32_t mode) {return sd_open(0,filename,mode);}
};


class MTPD {
public:
  explicit MTPD(MTPStorageInterface* storage): storage_(storage) {}

  int open(char *filename){return storage_->OpenFile(filename);}

private:
  MTPStorageInterface* storage_;
};

void setup() {
  // put your setup code here, to run once:
  while(!Serial);
  FS sdx[1];
  MTPStorage_SD storage(sdx);
  MTPD       mtpd(&storage);

  Serial.println(mtpd.open((char *)"test"));
}

void loop() {
  // put your main code here, to run repeatedly:

}

So there must be something in the implementation of SD that is generating this error
Will dig in further
getting now difficult to find missing implementation
 

The idea is to use everywhere the FS interface class and in the, what I call mSD_Base class, something like
Code:
if(cs[ii]==0) return ((SDClass*) &sdx[ii].open(...);
else if (cs[ii]==256) return ((LittleFS_RAM*) &sdx[ii].open(...);
else if (cs[ii]==257) return ((LittleFS_QSPI*) &sdx[ii].open(...);
etc.
In the Configuration function, there is then no need to know SDCLass, LittleFS_RAM etc, only the connection to chip select number
 
The idea is to use everywhere the FS interface class and in the, what I call mSD_Base class, something like
Code:
if(cs[ii]==0) return ((SDClass*) &sdx[ii].open(...);
else if (cs[ii]==256) return ((LittleFS_RAM*) &sdx[ii].open(...);
else if (cs[ii]==257) return ((LittleFS_QSPI*) &sdx[ii].open(...);
etc.
In the Configuration function, there is then no need to know SDCLass, LittleFS_RAM etc, only the connection to chip select number

Agree - would make it alot cleaner

Just tried your test sketch and it seems to work here too. Pointed you to that post just by way of added info.

EDIT: if you include SPIFlash you are dealing again with CS pins and SPI port. So I acuatlly added in another var to you setNumbers function: Storage type so if 0 == sd, 1 == RAM, 2 == SPIFlash and 3 == QSPI. Only way i could figure out to differentiate between cs pins for SD cards and SPIFLash.
 
This is partially (maybe mostly) my fault. The FS class is very much beta at this point and still needs work....

With that in mind, I believe the main problem is this line:

Code:
  FS sdx[1];

FS is meant to be an abstract base class. But I didn't (yet) properly define it that way, so this code compiles when it really should give you an "invalid abstract type" error.

Even as FS is defined so far, I hope you can see creating instances of FS makes no sense. It's not a tangible class that actually does anything with any type of media. It's just an abstract base class which the actual media-accessing classes like SDClass, LittleFS_RAM, LittleFS_SPIFlash, LittleFS_QSPIFlash and LittleFS_Program inherit, so programs like your's can access them all in a uniform and hardware-independent way.

Change that line to this and your program compiles. :)

Code:
  SDClass sdx[1];

You approach of an array of FS pointers is correct. You just need to get the addresses to initialize the array from real / tangible instances like SDClass, LittleFS_SPIFlash, etc...


While technically not an error (at least not a compile-time error), the type cast on this line may be problematic.

Code:
        return ((SDClass *)&sdx[store])->open(filename, mode);

Better to leave just use it as FS, as that's what the FS class is meant to do.

Code:
        return sdx[store].open(filename, mode);
 
In the Configuration function, there is then no need to know SDCLass, LittleFS_RAM etc, only the connection to chip select number

You should not need the chip select pin. The FS derived class stores all the details it needs to access the hardware. If it even uses a chip select pin, that info is inside the media-accessing instance. All you should need is a FS pointer to that instance.

I hope you will consider a public API which allows an Arduino style something like this:

Code:
#include <SD.h>
#include <LittleFS.h>
#include <MTP.h>

LittleFS_Program myfs;

void setup() {
  SD.begin(csPin);
  if (SD) {
    MTPD.addFilesystem(SD);
  }
  myfs.begin(520000);
  if (myfs) {
    MTPD.addFilesystem(myfs)
  }

Perhaps the code could look something like this?

Code:
#include <FS.h>

#define MTPD_MAX_FILESYSEMS  20

class MTPDclass {
  public:
    MTPDclass() {
      fsCount = 0;
    }
    void addFilesystem(FS &filesystem) {
      if (fsCount < MTPD_MAX_FILESYSEMS) {
        sdx[fsCount++] = &filesystem;
      }
    }
  private:
    int fsCount;
    FS *sdx[MTPD_MAX_FILESYSEMS];
};

extern MTPDclass MTPD;

You should not need to include SD.h or LittleFS.h. Only the user's sketch code in Arduino needs those. The actual media-accessing instances come from the top-level code the user writes. You should only need to get FS pointers to those instances, without any need to code for the specific media types. The FS & File classes are meant to allow hardware-independent access to file on any media.

But of course this stuff is all still beta and a moving target. I've probably made mistakes & oversights, which I'll try to fix as the issues come up. Please understand I'm only human and just 1 guy. But I really do want to make this work well.
 
Last edited:
You should not need the chip select pin. The FS derived class stores all the details it needs to access the hardware. If it even uses a chip select pin, that info is inside the media-accessing instance. All you should need is a FS pointer to that instance.

I hope you will consider an public API something like this:

Code:
void setup() {
  SD.begin(csPin);
  if (SD) {
    MTPD.addFilesystem(SD);
  }

Perhaps the code could look something like this?

Code:
#include <FS.h>

#define MTPD_MAX_FILESYSEMS  20

class MTPDclass {
  public:
    MTPDclass() {
      fsCount = 0;
    }
    void addFilesystem(FS &filesystem) {
      if (fsCount < MTPD_MAX_FILESYSEMS) {
        sdx[fsCount++] = &filesystem;
      }
    }
  private:
    int fsCount;
    FS *sdx[MTPD_MAX_FILESYSEMS];
};

extern MTPDclass MTPD;

You should not need to include SD.h or LittleFS.h. Only the user's sketch code in Arduino needs those. The actual media-accessing instances come from the top-level code the user writes. You should only need to get FS pointers to those instances, without any need to code for the specific media types. The FS & File classes are meant to allow hardware-independent access to file on any media.

But of course this stuff is all still beta and a moving target. I've probably made mistakes & oversights, which I'll try to fix as the issues come up. Please understand I'm only human and just 1 guy. But I really do want to make this work well.

Thanks Paul,
I will try to follow your suggestions
Originally I wanted to avoid a MAXFILESYTEMS declaration, but as pointers it does not uses too much space.
 
I believe you should use an array of pointers to the FS-derived instances, rather that just a single pointer to an array.

(edit - crossposted with your reply....)

In other words, instead of this:

Code:
private:
  FS *sdx;

I would recommend using this:

Code:
private:
  FS *sdx[MTPD_MAX_FILESYSEMS];

The first way requires all the media-accessing instances to be neatly packed in memory as an array. That's not very practical, since they could be many different types provided by different libraries. The actual instances are likely to be different sizes.

By using an array of pointers, the user can create the media-accessing instances separately, and you can provide a very easy Arduino-like API for the user to "begin" or "add" or "attach" them as needed, without requiring the user to think about how the memory is arranged or allocated. By using an array of pointers inside your library, each filesystem is guaranteed to use exactly 4 bytes for the pointer. Each can be located anywhere in memory and be whatever size its class actually implements beyond the FS interface.
 
As this progresses, I'm imagining I will add functions to the FS class and code inside SD/SdFat and LittleFS specifically to support MTP. Eventually there will need to be some way for MTP to detect when the filesystem has changed and give notice to the USB host. How that will all work, I do not know at this time.

Just want to say MTP is important and the FS class can and will expand to meet MTP needs.
 
So,
I adapted the MTP code to the suggested separation between FS-agnostic interface and the main code, where user adds FS of interests.
Not sure if I did it exactly as suggested, but it works.
mtp_test was used to test (and debug the code)
github is updated
 
@WMXZ
This is probably going to be a evolving over time. But nice job - was lost on how to do it.
 
Status
Not open for further replies.
Back
Top