pimoroni / enviro

MIT License
101 stars 79 forks source link

Store multiple readings in each log file #135

Open guruthree opened 1 year ago

guruthree commented 1 year ago

If no upload destination/frequency is specified, logged readings are instead stored locally. Currently the Enviro firmware will create one file for each logged reading. This is unwieldy both for downloading and then later processing the data.

The existing save_reading function will currently append to an existing log file, but on each call used a different readings_filename, resulting in one file per reading. I propose the introduction of a readings_per_file configuration item that is used in creating a multi-use filename so that multiple readings can be properly appended into files, keeping both file sizes and file numbers manageable.

The new filename retains the same format, but rather than using the current time, a filename time is calculated using floor() on the Unix timestamp divided by (reading frequency × readings per file × 60), and then multiplied back up by the latter, similar to how the numbers of minutes into the day is calculated. For example, if the reading interval is 15 minutes and the number of readings per file is 96, then the filename for today would be readings/2022-12-30T00_00_04Z.txt.

The new configuration item is exposed during provisioning when the 'Never' upload option is selected.

ZodiusInfuser commented 1 year ago

Thanks for raising this pull request! We haven't had that many users make use of the local logging feature, so the issue of file count wasn't really considered by us. I appreciate you including the provisioning options for it too!

I will test this code out and let you know if there's any tweaks that need to be made. At first glance some of the numbers in the comments seem to have the wrong units. 15 per minute for instance.

nils-s commented 11 months ago

Ran into the same problem and found this PR, which seems to have gone stale.

Judging by the wording of the comments around the save_reading function in enviro/__init__.py (specifically the comment on line 366), I'd say a fix could actually be a lot simpler than what is provided by this PR: on line 370, instead of using helpers.datetime_file_string() to generate the filename, simply use helpers.date_string(), which would create one file per day, with all the logs for that day (consistent with the comments on lines 366 and 368).

@ZodiusInfuser: this would fix (or at least alleviate) the issue of one file per reading, albeit without the configurable number of readings per file (as suggested and provided by @guruthree). Should I create a PR for that, or is this PR still on track?

Edit: instead of continuing to necro-post here, I've created issue #193 to discuss possible solutions, and PR #194 containing my suggested minimal fix.

guruthree commented 10 months ago

Yes your approach is a lot simpler. I designed this PR specifically to be similar to the way uploads work, sort of treating the stored file as if it were upload target, dumping all the stored readings into that file the same way they're uploaded in a group. Possibly not the most straightforward of approach, but it hopefully provides a consistent experience between choosing upload and store to file for users.

Edit: apologies for further necromancy, I missed your edit about creating an issue.