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 1 commit
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
54 changes: 17 additions & 37 deletions libraries/SPIFFS/src/SPIFFS.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,26 @@
// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
Copy link
Member

Choose a reason for hiding this comment

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

Do not remove the headers!

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

please use a new line before the curly braces. I see this a few more times below :)

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 @@ -68,8 +62,7 @@ void SPIFFSFS::end()
}
}

bool SPIFFSFS::format()
{
bool SPIFFSFS::format() {
disableCore0WDT();
esp_err_t err = esp_spiffs_format(NULL);
enableCore0WDT();
Expand All @@ -80,34 +73,21 @@ bool SPIFFSFS::format()
return true;
}

size_t SPIFFSFS::totalBytes()
{
size_t SPIFFSFS::totalBytes() {
size_t total,used;
if(esp_spiffs_info(NULL, &total, &used)){
return 0;
}
return total;
}

size_t SPIFFSFS::usedBytes()
{
size_t SPIFFSFS::usedBytes() {
size_t total,used;
if(esp_spiffs_info(NULL, &total, &used)){
return 0;
}
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()));
32 changes: 13 additions & 19 deletions libraries/SPIFFS/src/SPIFFS.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,33 @@
// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
Copy link
Member

Choose a reason for hiding this comment

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

Do not remove the headers!

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#ifndef _SPIFFS_H_
#define _SPIFFS_H_
#pragma once
Copy link
Member

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

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

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

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

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

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

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

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

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();
Copy link
Member

Choose a reason for hiding this comment

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

too much space?

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_ */