stm32duino / STM32SD

Enables reading and writing on SD card using SD card slot of the STM32 Board.
157 stars 23 forks source link

Add RAII support for File class #67

Closed willeccles closed 1 year ago

willeccles commented 1 year ago

When a File object goes out of scope, its resources are not freed. This patch:


Fixes #66.

willeccles commented 1 year ago

CI fails because the example sketches are broken and attempt to copy the file structure. I can either fix the examples or implement copying for this class.

willeccles commented 1 year ago

I suggest fixing the examples. Copying a file handle like this is generally discouraged and can cause some really awful problems. The offending example could easily be fixed by taking File& instead of File (or even File* if you prefer, but I don't see any advantage to using a pointer here).

fpistm commented 1 year ago

Well example is based on the official example. So API changed and it is not desired. Ex: https://github.com/arduino-libraries/SD/blob/master/examples/listfiles/listfiles.ino

willeccles commented 1 year ago

Ah, that is a bit annoying. I could implement copying for files here, but that's definitely not recommended... Perhaps it would be best to close this PR. It's not the first time I've run into Arduino's C++ code being sub-par :/

To implement copying, it's trivial to duplicate the file name, but I don't think there's any reasonable way to manage copies of the file handle itself. Re-opening isn't an option, as this will break if file locking is enabled, and if it's not, could lead to potential races. Copying the file handle directly would work, but then if one copy closes the file, the other is in an unknown state. As this also applies to the existing implementation, it's probably not worth changing.

fpistm commented 1 year ago

Anyway, I understand your point and it is valid.