pimoroni / enviro

MIT License
101 stars 79 forks source link

Local readings generate a new file for each reading #193

Closed nils-s closed 2 months ago

nils-s commented 10 months ago

Observed behaviour

On an Enviro Grow, configured to not upload data, each new reading generates a new file, leading to lots of files in the readings-folder on the device, each with only a single entry.

Expected behaviour

Multiple readings are batched into one file, e.g. one file per day containing all readings for that day. This is also the behaviour suggested by the comments in the save_reading function (in enviro/__init__.py).

Suggested fix

Currently, the save_reading function generates the name of the file in which to store the reading using the helpers.datetime_file_string function. Instead, the helpers.date_string function should be used.

This would be the minimal fix, bringing the actual behaviour in line with what the comments in save_reading currently suggest. The change has been tested locally and confirmed to work as intended.

Other options

There is already PR #135, which suggests a configurable number of readings per file. Alternatively, all readings could be written to one file, or the roll-over interval could be made configurable (e.g. one file per week).

As a sidenote, the files are currently named .txt, which (given that the file format is CSV) could be renamed as well (if desired).

guruthree commented 10 months ago

Apologies for double posting, I'm putting this here also to keep discussion in one location.

I designed #135 to work in a similar fashion to uploads, sort of treating the stored file as if it were upload target by dumping all the stored readings into that file analogous to the way that readings are uploaded all at the same time. It's probably not the most straightforward approach, but hopefully provides some consistency between the upload and local logging options for users.

nils-s commented 10 months ago

Thanks for explaining the rationale of your PR :)

I only noticed lots of reading files after getting my Enviro and playing around with it, and since your PR has been open for quite some time now I thought I'd give it another try to get the problem fixed. Since I had the fix for my local version anyway, I thought I might as well push it.

I'm mainly interested in any sort of fix that prevents individual files per reading, so as long as a fix gets merged I'm happy :)

nils-s commented 2 months ago

Thanks to @Gadgetoid #194 got merged, so the issue should be fixed. File extension for local readings was changed to .csv in #220, so everything mentioned in this issue (with the exception of #135) is now implemented. I assume that #135 will be tracked separately, so I'll close this ticket.

Gadgetoid commented 2 months ago

so the issue should be fixed.

Pending a release, yes, entertained issuing a patch for these but there might be some other stuff I can sweep up in short order so we'll see.