Skip to content
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.

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

Implement object model for files #1532

Merged
merged 3 commits into from
Nov 8, 2016
Merged

Conversation

devsaurus
Copy link
Member

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

Current file access functions operate on a default file and restrict the user to only one opened file at a time. This PR adds an object model to the file module which removes this constraint. The basic model is kept for compatibility.

Compared to the current implementation, this extension comes at costs regarding

  • memory - opening a file now allocates a Lua userdata (containing a single int)
  • run time - functions like file_read() etc. call lua_type() to determine the used model
  • dev complexity - both models have to be maintained in parallel
  • user complexity - users have to understand the differences to select and consistently use one of the models

This topic was briefly mentioned in the course of the FAT concept discussion, but I'd like to re-check whether there's a common agreement that file should evolve in this direction.

@marcelstoer
Copy link
Member

It's probably no surprise that I like this model way better than the existing one. It always felt odd calling functions on "the module" rather than on the "the thing" (i.e. the file) affected by those function calls. Thus the file module was also not consistent with other modules like .e.g. MQTT.

dev complexity - both models have to be maintained in parallel

That should only be temporary. If you clearly mark the old style as being deprecated in the docs and attach a "will be removed in 6 months"-label to it we have a way forward. Which reminds me...we did the same for tmr but without giving a time frame, which was not a good idea.

P.S. "Only one file can be open at any given time." in the intro of the docs needs to be removed or rephrased.

@jmattsson
Copy link
Member

Quick note: SPIFFs working buffer might need to grow, I cut those back to bare minimum (supporting 2 files in parallel) way back when. This comment might already be out of date, however.

@pjsg
Copy link
Member

pjsg commented Oct 11, 2016

I thought that we allowed 64 open spiffs files. I don't think that we need to increase the buffers for multiple files, but I'm not sure.

@devsaurus
Copy link
Member Author

Thanks for highlighting this. I'll set up some stress tests with many open files to probe for the upper limit.

@devsaurus
Copy link
Member Author

I get SPIFFS_ERR_OUT_OF_FILE_DESCS error when opening the 4th file 😞 Wasn't aware that SPIFFS uses a static buffer for the descriptors, but it's pretty obvious from https://github.com/nodemcu/nodemcu-firmware/blob/dev/app/spiffs/spiffs_hydrogen.c#L100:

fs->fd_count = (fd_space_size/sizeof(spiffs_fd));

Our spiffs.c provides 32*4 bytes for descriptor storage which fits 3 instances of spiffs_fd (size 36). Should I turn this into 36 * SPIFFS_MAX_OPEN_FILES and place the define in user_config.h (set to 4)?

@pjsg
Copy link
Member

pjsg commented Oct 12, 2016

I'd use sizeof(spiffs_fd) * SPIFFS_MAX_OPEN_FILES

@TerryE
Copy link
Collaborator

TerryE commented Oct 23, 2016

Thanks Arnim, this sounds like great improvement. I suspect that most Apps won't use it, but when they do it will prove extremely useful. 😄

@devsaurus
Copy link
Member Author

Rebased onto current dev.

@marcelstoer marcelstoer added this to the Post-1.5.4.1-II milestone Nov 8, 2016
@marcelstoer marcelstoer merged commit a0e2e0c into nodemcu:dev Nov 8, 2016
@devsaurus devsaurus deleted the file_object branch November 26, 2016 09:33
marcelstoer added a commit that referenced this pull request Dec 1, 2016
* add u8g.fb_rle display

* move comm drivers to u8g_glue.c

* disable fb_rle per default

* implement file.size for spiffs (#1516)

Another bug squashed!

* Fix start-up race between UART & start_lua. (#1522)

Input during startup (especially while doing initial filesystem format)
ran the risk of filling up the task queue, preventing the start_lua task
from being queued, and hence NodeMCU would not start up that time.

* Reimplemented esp_init_data_default.

To work around the pesky "rf_cal[0] !=0x05" hang when booting on a chip
which doesn't have esp_init_data written to it.

It is no longer possible to do the writing of the esp_init_data_default
from within nodemcu_init(), as the SDK now hangs long before it gets
there.  As such, I've had to reimplement this in our user_start_trampoline
and get it all done before the SDK has a chance to look for the init data.
It's unfortunate that we have to spend IRAM on this, but I see no better
alternative at this point.

* Replace hardcoded init data with generated data from SDK

The esp_init_data_default.bin is now extracted from the SDK (and its
patch file, if present), and the contents are automatically embedded
into user_main.o.

* Rework flashing instructions

Clarifies issues around SDK init data and hopefully clears up some
confusion, when paired with the esp_init_data_default changes in
NodeMCU.

* Fix typo

* Fixes the gpio.serout problem from #1534 (#1535)

* Fix some issues in gpio.serout
* Minor cleanup

* fix dereferencing NULL pointer in vfs_errno() (#1539)

* add map ids for flash sizes 32m-c2, 64m, 128m in user_rf_cal_sector_set() (#1529)

* Somfy/TELIS driver (#1521)

* Reduced LUAL_BUFFERSIZE to 256. Should free up some stack (#1530)

* avoid task queue overrun for serial input (#1540)

Thank you.

* Increase irom0_0_seg size for PR build

* Improve reliability of FS detection. (#1528)

* Version to make filesystem detection more reliable
* Improve bad fs detection

* Version of printf that doesn't suffer from buffer overflows (#1564)

* Small improvement to http client (#1558)

* Remove luaL_buffer from file_g_read() (#1541)

* remove luaL_buffer from file_g_read()
- avoid memory leak when function gets terminated by lua_error
- skip scanning for end_char when reading until EOF
* attempt to free memory in any case

* Change HTTP failures from debug to error messages (#1568)

* Change HTTP failures from debug to error messages

* Add tag to HTTP error messages

* Create macro for error msg and improve dbg msg

* Add ssd1306_128x32 for U8G (#1571)

* Update CONTRIBUTING.md

* Add support to mix ws2812.buffer objects.  (#1575)

* Add load/dump/mix/power operations on the buffer object
* Calculate the pixel value in mix and then clip to the range.
* Fixed the two wrong userdata types
* Added a couple more useful methods
* Add support for shifting a piece of the buffer.
* Fix a minor bug with offset shifts

* Update to the wifi module (#1497)

* Removed inline documentation for several functions and update comments
Since documentation is now part of the repository, the inline
documentation just adds to the already huge wifi.c

* Wifi module: add new functionality, update documentation

Functions Added:
wifi.getdefaultmode(): returns default wifi opmode
wifi.sta.apchange(): select alternate cached AP
wifi.sta.apinfo(): get cached AP list 
wifi.sta.aplimit(): set cached AP limit
wifi.sta.getapindex(): get index of currently configured AP
wifi.sta.getdefaultconfig(): get default station configuration
wifi.ap.getdefaultconfig(): get default AP configuration

functions modified:
wifi.setmode: saving mode to flash is now optional
wifi.sta.config: now accepts table as an argument and save config to
flash is now optional
wifi.sta.getconfig: added option to return table
wifi.ap.config: save config to flash is now optional
wifi.ap.getconfig: added option to return table

Documentation changes:
- Modified documentation to reflect above changes
- Removed unnecessary inline documentation from `wifi.c` 
- Updated documentation for `wifi.sta.disconnect`to address issue #1480 
- Fixed inaccurate documentation for function `wifi.sleeptype`
- Added more details to `wifi.nullmodesleep()`

* Move function `wifi.sleeptype()` to `wifi.sta.sleeptype()`

* Fixed problem where wifi.x.getconfig() returned invalid strings when
ssid or password were set to maximum length.

* fix error in documentation for `wifi.sta.getapindex`

* Renamed some wifi functions
wifi.sta.apinfo -> getapinfo
wifi.sta.aplimit -> setaplimit 
wifi.sta.apchange -> changeap

also organized the wifi_station_map array

* Make the MQTT PING functionality work better. (#1557)

Deal with flow control stopped case

* Implement object model for files (#1532)

* Eus channelfix (#1583)

Squashed commits included:

Bug fixes and final implementation
- Added Content-Length: 0 to all headers
- Endpoint name checks not using trailing space so cache-busting techniques can be used (i.e., append a nonce to the URL)
- Track when connecting so APList scan doesn't take place during (which changes the channel)
- More debugging output added to assist in tracking down some issues

Added /status.json endpoint for phone apps/XHR to get JSON response

Station Status caching for wifi channel workaround + AJAX/CORS
- During checkstation poll, cache the last station status
- Shut down the station if status = 2,3,4 and channel is different than SoftAP
- Add Access-Control-Allow-Origin: * to endpoint responses used by a service
- Add a /setwifi GET endpoint for phone apps/XHR to use (same parameters as /update endpoint). Returns a JSON response containing chip id and status code.
- Add handler for OPTIONS verb (needed for CORS support)

Wi-Fi Channel Issue Workaround
- Do a site survey upon startup, set SoftAP channel to the strongest rssi's channel
- Compare successful station connect channel to SoftAP's. If different, then defer the Lua success callback to the end. Shut down Station and start the SoftAP back up with original channel.
- After the 10 second shutdown timer fires, check to see if success callback was already called. If not, then call it while starting the Station back up.

HTTP Response and DNS enhancements
- If DNS's UDP buffer fills up, keep going as non-fatal. It's UDP and not guaranteed anyways. I've seen this occur when connecting a PC to the SoftAP and every open program tries to phone home at the same time, overwhelming the EUS DNS server.
- Support for detecting/handling pre-gzipped `enduser_setup.html` (and `http_html_backup`) payload. Nice for keeping the size of the `state->http_payload_data` as small as possible (also makes minimization not as critical)
- Corrected misuse of HTTP 401 response status (changed one occurrence to 400/Bad Request, and changed another to 405/Method Not Allowed)

* Normalized formatting (tabs-to-spaces)
* Added documentation
* Corrected misuse of strlen for binary (gzip) data.
* Added NULL check after malloc

* fix vfs_lseek() result checking in enduser_setup and clarify SPIFFS_lseek() return value (#1570)

* Fix link

* Overhaul flashing docs once again (#1587)

* Add chapter about determine flash size plus small fixes
* Rewrite esptool.py chapter, move flash size chapter to end

* i2c - allow slave stretching SCL (just loop and check) (#1589)

* Add note on dev board usage of SPI bus 0 (#1591)

* Turn SPI busses note to admonition note

* support for custom websocket headers (#1573)

Looks good to me. Thank you.

Also:
 - allow for '\0's in received messages

* add client:config for setting websocket headers

Also:
 - headers are case-insensitive now

* fix docs

* fix typo

* remove unnecessary luaL_argcheck calls

* replace os_sprintf with simple string copy

* Handle error condition in file.read() (#1599)

* handle error condition in file.read()

* simplify loop initialization

* Fix macro as suggested in #1548

* Extract and hoist net receive callbacks

This is done to avoid the accidental upval binding

* Fix typo at rtctime.md

rtctime.dsleep -> rtctime.dsleep_aligned
@danjohn
Copy link

danjohn commented Jan 7, 2017

Fine extension to file.* module, appreciate this!

Please add a function to get details out of a file object... maybe I'm wrong, but it seems to be very hidden by your approach, as there are no getters().
If e.g. there is to be making a backup copy of a file, tha filename of the encapsulated object has to be evalueable. Or think of a file object, thats passed between functions.. that like to know, if it's W/R.

I suppose, that can be found somewhere in _G, but a simple file:getStat() (or the like) would be a great help for the file module UI!

@devsaurus
Copy link
Member Author

@danjohn see #1724, adds the file.stat() function to propagate file attributes to Lua level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants