jneilliii / OctoPrint-Tasmota

62 stars 16 forks source link

Power measurement data gets saved to settings, causes expensive data processing during prints #103

Closed foosel closed 3 years ago

foosel commented 3 years ago

As discussed on Discord, if the monitored Tasmota device supports power measurement, on every polling interval (at least I think that it's related to that) the plugin will save all its settings, including label information (with the constantly changing power measurement data), thus prompting a bunch of somewhat processing intense tasks that can cause resource drain for the print job and thus stuttering.

I can currently NOT reproduce it after upgrading to 0.8.16rc2, as for some reason that version no longer appears to recognize the power measuring capabilities of my tasmota device (tooltip is pretty empty too) and no saving is happening. It might still get triggered on the frontend, at least I see some debug logging happening that might originate from the same routine (https://github.com/jneilliii/OctoPrint-Tasmota/blob/rc/octoprint_tasmota/static/js/tasmota.js#L301 - should prolly be removed).

This was the location where the save was triggered: https://github.com/jneilliii/OctoPrint-Tasmota/blob/7e8045d0573350f03585d860c3a0ad8e5994df5b/octoprint_tasmota/static/js/tasmota.js#L340

My suggestion would be to not save this kinda stuff to the settings in the first place. It's not settings, it's data, so it should go into the data folder. If it even needs saving in the first place, not really sure why you'd even want to persist that data considering that the Tasmota device has it. Frontend specific things like a label/tooltip text certainly don't belong in the settings or anywhere into the backend, unnecessary wear on what's usually SDs holding the file system.

OctoPrint version 1.5.0rc3 OctoPrint-Tasmota version 0.8.15 (also tested 0.8.16rc2 but can't repro due to no power measurement getting detected)

Somewhat related to #60.

jneilliii commented 3 years ago

If it even needs saving in the first place, not really sure why you'd even want to persist that data considering that the Tasmota device has it.

I'm actually storing the sensor/power data into the data folder already and it's solely for graphing on the tab.

I assume this kind of data would be better suited as a localized observable to the viewmodel that only gets updated via send_plugin_message payload or something similar. Same for current state (you may remember this conversation from before with tplink). I had used settings because it seemed to be the most consistent way to get my observables updated at the time of writing the plugin. My knowledge has gotten a little better since then, so I don't see why this can't be adjusted outside of time...

I am interested in why the rc version doesn't still have the issue though, because no processing logic was changed, just the knockout sortable library I was using to be compatible with the latest knockout in 1.5.0.

jneilliii commented 3 years ago

AKA...where's the logs!!!!! lol.

foosel commented 3 years ago

Where's the contribution guidelines telling me which ones you need :P Here's octoprint.log and a tasmota debug log I found. Device is a Gosund SP111

https://drive.google.com/folderview?id=1vihmSUFOkTSKiHhoixCOyYv_kgg63tZW

(Sorry for gdrive, currently on mobile)

jneilliii commented 3 years ago

no worries, I was just giving you a hard time anyway...thanks for the sponsorship!

jneilliii commented 3 years ago

Interesting looking at your logs, it appears at first you were missing your username/password combination, but then may have figured that out and it did start to get power data later in the log.

jneilliii commented 3 years ago

BTW your password is in those logs too...

foosel commented 3 years ago

You should probably change that being in those logs then ;) the good thing is, it's easy to just generate a new one.

Also yes, I changed it before today, and for some reason couldn't get the settings to save the new one. In the end I changed it directly in config.yaml and restarted, but even after that it didn't give me power readings, when I created this ticket.

foosel commented 3 years ago

Quick update though, can confirm that it still happens on the RC now.

jneilliii commented 3 years ago

Thanks. I'll look into both encrypting the passwords using the built in salt and getting around these constant saves. For TPLink I ended up making the polling during print progress optional, which resolved the print quality issues there. I'm on vacation this upcoming week so will have some time to work on my plugins. If you have any pointers/helper functions on the encryption front that you can point me to that would be awesome, but I will be checking into the docs. I guess the hard thing is dealing with saves and re-encrypting the already encrypted password.

foosel commented 3 years ago

Maybe just sanitising the log output would already be an option?

jneilliii commented 3 years ago

That would be an option, but the data is still stored in plain text in config.yaml. I guess that could be reversed engineered if you had access to the salt anyway.

jneilliii commented 3 years ago

Seems easy enough to re-use this maybe and then just handle the re-encryption part by doing a check_password on save.

https://github.com/OctoPrint/OctoPrint/blob/077d4cd0d4b57af1309860863fecc2d2ff8df6eb/src/octoprint/access/users.py#L147-L185

jneilliii commented 3 years ago

I think I've fixed this in the above commit. If you don't mind testing out the newest 0.8.16rc3 version linked below that would be great (or if you have 1.5.0 you can switch to the Release Candidate channel πŸ˜‰).

https://github.com/jneilliii/OctoPrint-Tasmota/archive/0.8.16rc3.zip

foosel commented 3 years ago

Looking good! πŸ‘ So far have not been able to reproduce the constant settings refresh. Have not yet had a chance to test a print, but before this RC I could trigger it somewhat easily just by powering up & connecting (thus using power), waiting a bit, then downloading a file or something, thus triggering a websocket reconnect and detecting a settings change. Currently that's no longer possible.

jneilliii commented 3 years ago

Cool, I'm interested in if the print quality issue could still be there, because there is still data being saved to the database on every status check of power/sensor connected devices.

foosel commented 3 years ago

Printing a benchy right now and so far no slowdowns observed. Also no more network request storms getting triggered every n minutes due to a SettingsUpdated event, so that looks good indeed! Do you want me to close this or do you keep issues open until a stable release, like I do on OctoPrint?

jneilliii commented 3 years ago

Thanks, I keep them open until stable release.