Skip to content

Fix #2750 #2763

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 13, 2019
Merged

Fix #2750 #2763

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
88e13a0
Fix #2750
judge2005 May 9, 2019
747ab2e
Sync with upstream
judge2005 May 11, 2019
b28c423
Fixes for pull comments
judge2005 May 11, 2019
009e0bb
latest modifications following comments from me-no-dev
judge2005 May 12, 2019
a677959
Move SPIFFSFSImpl to .cpp file
judge2005 May 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 16 additions & 17 deletions libraries/SPIFFS/src/SPIFFS.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,31 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include "vfs_api.h"

extern "C" {
#include <sys/unistd.h>
#include <sys/stat.h>
#include <dirent.h>
#include "esp_spiffs.h"
}

#include "SPIFFS.h"

using namespace fs;

SPIFFSFS::SPIFFSFS(FSImplPtr impl)
: FS(impl)
{}
SPIFFSImpl::SPIFFSImpl()
{
}

bool SPIFFSImpl::exists(const char* path)
{
File f = open(path, "r");
return (f == true) && !f.isDirectory();
}

SPIFFSFS::SPIFFSFS() : FS(FSImplPtr(new SPIFFSImpl()))
{

}

bool SPIFFSFS::begin(bool formatOnFail, const char * basePath, uint8_t maxOpenFiles)
{
Expand Down Expand Up @@ -98,16 +108,5 @@ size_t SPIFFSFS::usedBytes()
return used;
}

bool SPIFFSFS::exists(const char* path)
{
File f = open(path, "r");
return (f == true) && !f.isDirectory();
}

bool SPIFFSFS::exists(const String& path)
{
return exists(path.c_str());
}

SPIFFSFS SPIFFS;

SPIFFSFS SPIFFS = SPIFFSFS(FSImplPtr(new VFSImpl()));
19 changes: 15 additions & 4 deletions libraries/SPIFFS/src/SPIFFS.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,37 @@
#ifndef _SPIFFS_H_
#define _SPIFFS_H_

#pragma once
Copy link
Member

@me-no-dev me-no-dev May 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is redundant with the above. keep one or the other :)


#include "FS.h"
#include "FSImpl.h"
Copy link
Member

@me-no-dev me-no-dev May 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two includes should not be in the header, but in the CPP file instead. Nothing in the header depends on them :)

Copy link
Contributor Author

@judge2005 judge2005 May 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header does depend on them, but they are also included via vfs_api.h, so at most they are redundant. For the same reason they don't need to be included in the .cpp file (because they are included via vfs_api.h which is included via SPIFFS.h).

Whatever, I have removed them from SPIFFS.h, but I haven't added them to SPIFFS.cpp.

#include "vfs_api.h"
Copy link
Member

@me-no-dev me-no-dev May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FS should be included. vfs_api is a private header.

Copy link
Contributor Author

@judge2005 judge2005 May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18 hours ago, you asked me to remove FS.h and FSImpl.h, now you are asking me to add FS.h back in.

If I remove vfs_api.h I will also have to add FSImpl.h back because SPIFFSImpl is declared in SPIFFS.h.

Copy link
Member

@me-no-dev me-no-dev May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I asked about #include "FSImpl.h" and #include "vfs_api.h".
FS.h is the only one that should be included :) I guess a bit of misunderstanding.

Copy link
Contributor Author

@judge2005 judge2005 May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change requires an additional class, SPIFFFSImpl that inherits VFSImpl, which is declared in vfs_api.h, so something is going to need to include vfs_api.h. Currently this class is declared in SPIFFS.h, which is why SPIFFS.h includes vfs_api.h. How would you have me resolve this?

Copy link
Member

@me-no-dev me-no-dev May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this class need to be public? What happens if the declaration goes into the CPP file?

Copy link
Contributor Author

@judge2005 judge2005 May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine if I move it to the CPP file.


namespace fs
{

class SPIFFSImpl : public VFSImpl
{
public:
SPIFFSImpl();
virtual ~SPIFFSImpl() { }
virtual bool exists(const char* path);
};

class SPIFFSFS : public FS
{
public:
SPIFFSFS(FSImplPtr impl);
SPIFFSFS();
bool begin(bool formatOnFail=false, const char * basePath="/spiffs", uint8_t maxOpenFiles=10);
bool format();
size_t totalBytes();
size_t usedBytes();
void end();
bool exists(const char* path);
bool exists(const String& path);
};

}

extern fs::SPIFFSFS SPIFFS;

#endif /* _SPIFFS_H_ */

#endif