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

Local readings generate a new file for each reading #193

Closed
nils-s opened this issue Aug 9, 2023 · 4 comments
Closed

Local readings generate a new file for each reading #193

nils-s opened this issue Aug 9, 2023 · 4 comments

Comments

@nils-s
Copy link
Contributor

nils-s commented Aug 9, 2023

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
Copy link

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
Copy link
Contributor Author

nils-s commented Aug 19, 2023

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
Copy link
Contributor Author

nils-s commented Apr 24, 2024

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.

@nils-s nils-s closed this as completed Apr 24, 2024
@Gadgetoid
Copy link
Member

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.

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

No branches or pull requests

3 participants