Implemented temperature and humidity display in graph #392

Merged
Dak0r merged 4 commits from graph-pr-branch into master 2021-10-13 18:44:14 +01:00
Dak0r commented 2021-02-10 20:44:35 +00:00 (Migrated from github.com)

Hi, first of all great plugin!

With my changes every temperature sensor now has a "Show temperature in graph" and a "Show humidity in graph" option which allows to show these values in the temperature graph.

I added this by implementing the octoprint.comm.protocol.temperatures.received-hook and automatically add the temperature and humidity values to the data-set for all sensors that have these options checked.

Remarks:
The default graph currently does not display these custom values, yet, but it works perfectly with @jneilliii s OctoPrint-PlotlyTempGraph Plugin: https://github.com/jneilliii/OctoPrint-PlotlyTempGraph

If you enable one of the new "show in graph"-checkboxes, it also automatically displays a message, which informs the user about this (See screenshots for on/off examples).

Testing:
I did a fresh branch for the PR, but I have my implementation running since November, without any issues 😄
(See my master branch https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/commits/master )

Related issues: https://github.com/vitormhenrique/OctoPrint-Enclosure/issues/34 https://github.com/vitormhenrique/OctoPrint-Enclosure/issues/303

Again thanks to @jneilliii for his Graph plugin and helping me with getting the settings patcher going 👍


Graph with "Enclosure" named Temperature Sensor:

graph

Settings for the shown Temperature Sensor:

new setting on off

Hi, first of all great plugin! With my changes every temperature sensor now has a "_Show temperature in graph_" and a "_Show humidity in graph_" option which allows to show these values in the temperature graph. I added this by implementing the ```octoprint.comm.protocol.temperatures.received```-hook and automatically add the temperature and humidity values to the data-set for all sensors that have these options checked. **Remarks:** The default graph currently does not display these custom values, yet, but it works perfectly with @jneilliii s OctoPrint-PlotlyTempGraph Plugin: https://github.com/jneilliii/OctoPrint-PlotlyTempGraph If you enable one of the new "show in graph"-checkboxes, it also automatically displays a message, which informs the user about this (See screenshots for on/off examples). **Testing:** I did a fresh branch for the PR, but I have my implementation running since November, without any issues 😄 (See my master branch https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/commits/master ) **Related issues:** https://github.com/vitormhenrique/OctoPrint-Enclosure/issues/34 https://github.com/vitormhenrique/OctoPrint-Enclosure/issues/303 Again thanks to @jneilliii for his Graph plugin and helping me with getting the settings patcher going 👍 ----------------------- **Graph with "Enclosure" named Temperature Sensor:** ![graph](https://user-images.githubusercontent.com/4193623/107569174-4b02ee00-6be8-11eb-9679-382ebb99a17e.png) **Settings for the shown Temperature Sensor:** ![new setting on off](https://user-images.githubusercontent.com/4193623/107569219-59510a00-6be8-11eb-9da0-b9396e9e1ed9.png)
raoulh (Migrated from github.com) reviewed 2021-03-07 10:11:56 +00:00
@@ -154,3 +153,4 @@
return 9
def on_settings_migrate(self, target, current=None):
self._logger.warn("######### current settings version %s target settings version %s #########", current, target)
raoulh (Migrated from github.com) commented 2021-03-07 10:11:56 +00:00

This if target >= 7 block will never execute, because the previous block is if current >= 4 and target == 6:

target can't be equal to 6 and >= 7 at the same time...

This `if target >= 7` block will never execute, because the previous block is `if current >= 4 and target == 6:` `target` can't be equal to 6 and >= 7 at the same time...
Dak0r (Migrated from github.com) reviewed 2021-03-07 10:52:01 +00:00
@@ -154,3 +153,4 @@
return 9
def on_settings_migrate(self, target, current=None):
self._logger.warn("######### current settings version %s target settings version %s #########", current, target)
Dak0r (Migrated from github.com) commented 2021-03-07 10:52:01 +00:00

Thanks for looking at this! :)
I just pushed a fix for this, it should of course be "target >= 6": 77ac49ecf9

That was an oversight when preparing the branch for the PR, I had the upgrade tested and working in my "main" branch:
https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/blob/master/octoprint_enclosure/init.py#L132

Sorry for that

Thanks for looking at this! :) I just pushed a fix for this, it should of course be "target >= 6": https://github.com/vitormhenrique/OctoPrint-Enclosure/pull/392/commits/77ac49ecf98bfef2b3fac9ac2f7a51edf2f5276f That was an oversight when preparing the branch for the PR, I had the upgrade tested and working in my "main" branch: https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/blob/master/octoprint_enclosure/__init__.py#L132 Sorry for that
wickedbeernut commented 2021-03-21 18:42:50 +00:00 (Migrated from github.com)

@Dak0r,

I'd like to see support for the target temperature and humidity (in addition to the actual temperature and humidity) in the case of rpi_outputs with output_type = 'temp_hum_control' and control_type = 'heater' / 'cooler' and 'dehumidifier', respectively. Rather than just returning the actual temperature and humidity corresponding to rpi_inputs with input_type = 'temperature_sensor', you'd start with the rpi_outputs. Each of the beforementioned rpi_outputs is linked to a 'temperature_sensor'. You'd return the actual and target temperature and humidity corresponding to the rpi_outputs, then you'd return the actual temperature and humidity corresponding to the rpi_inputs not associated with 'temp_hum_control' rpi_outputs. Of course, if the user doesn't have any 'temp_hum_control' rpi_outputs, the end result would be exactly as with your current implementation. Hope this makes sense.

How should I go about making this feature request? Is this an appropriate place for it or should I add it to the master branch? Obviously, it builds on the excellent work you've done.

image

@Dak0r, I'd like to see support for the target temperature and humidity (in addition to the actual temperature and humidity) in the case of rpi_outputs with output_type = 'temp_hum_control' and control_type = 'heater' / 'cooler' and 'dehumidifier', respectively. Rather than just returning the actual temperature and humidity corresponding to rpi_inputs with input_type = 'temperature_sensor', you'd start with the rpi_outputs. Each of the beforementioned rpi_outputs is linked to a 'temperature_sensor'. You'd return the actual and target temperature and humidity corresponding to the rpi_outputs, **then** you'd return the actual temperature and humidity corresponding to the rpi_inputs not associated with 'temp_hum_control' rpi_outputs. Of course, if the user doesn't have any 'temp_hum_control' rpi_outputs, the end result would be exactly as with your current implementation. Hope this makes sense. How should I go about making this feature request? Is this an appropriate place for it or should I add it to the master branch? Obviously, it builds on the excellent work you've done. ![image](https://user-images.githubusercontent.com/47407361/111916692-ca82a900-8a41-11eb-97d1-1c7128c0c989.png)
Dak0r commented 2021-03-24 16:58:26 +00:00 (Migrated from github.com)

@wickedbeernut thanks for your idea!
As you already spent quite some thoughts on it, feel free to fork my repo, implement it, and make a PR there:
https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/

I can then merge your changes into the branch I used for this PR (graph-pr-branch) so it will be included, if this PR will be merged eventually 😄

Otherwise, I can look into it once I have some time available to work on it 👍

@wickedbeernut thanks for your idea! As you already spent quite some thoughts on it, feel free to fork my repo, implement it, and make a PR there: https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/ I can then merge your changes into the branch I used for this PR ([graph-pr-branch](https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/tree/graph-pr-branch)) so it will be included, if this PR will be merged eventually 😄 Otherwise, I can look into it once I have some time available to work on it 👍
wickedbeernut commented 2021-03-26 15:38:40 +00:00 (Migrated from github.com)

Thank you, @Dak0r. I'd first like to better understand the process / timeline for merging your PR (without any additional changes) into the master branch. I don't see where master branch issues (bugs and feature requests) have been acknowledged in almost three months. I understand @vitormhenrique has been recovering from an injury. I'm maintaining several bug fixes and features privately. I rather not invest the time if the PR isn't going anywhere any time soon.

Thank you, @Dak0r. I'd first like to better understand the process / timeline for merging your PR (without any additional changes) into the master branch. I don't see where master branch issues (bugs and feature requests) have been acknowledged in almost three months. I understand @vitormhenrique has been recovering from an injury. I'm maintaining several bug fixes and features privately. I rather not invest the time if the PR isn't going anywhere any time soon.
vitormhenrique commented 2021-03-26 18:13:37 +00:00 (Migrated from github.com)

Hello Guys,

First, thanks for contributing to this plugin! I love to see it getting used and people helping out...

I started completely rewriting this to properly support python 3 few months ago, I hate the way that i wrote this on the first place, it's way to hacky and it was my first project on python.

I finished the initial part that I really dislike that it the setting, the long UI causes a LOT of issues with people that are not tech savvy, and now I'm gonna focus on proper class based IO's.

I also wanna move away for the manual installed libraries and including circuit python as standard on the plugin.

I can definitely merge all pull request in the meanwhile, but I'm not gonna be able to test them, so basically if you say that is OK for merging, I'll trust you...

@Dak0r, @wickedbeernut , and @raoulh let me know what you guys need from me and I'll do it to get this on the master branch.
but if you guys really wanna help, we can schedule a quick meeting, get you guys as contributors for this plugin (access to this branch), and divide the work that I'm doing possibly making prod ready faster....

I'm now mostly busy with my masters, and thankfully fully recovered from my injury, so I wanna get back to this project as soon as I can....

Hello Guys, First, thanks for contributing to this plugin! I love to see it getting used and people helping out... I started completely rewriting this to properly support python 3 few months ago, I hate the way that i wrote this on the first place, it's way to hacky and it was my first project on python. I finished the initial part that I really dislike that it the setting, the long UI causes a LOT of issues with people that are not tech savvy, and now I'm gonna focus on proper class based IO's. I also wanna move away for the manual installed libraries and including circuit python as standard on the plugin. I can definitely merge all pull request in the meanwhile, but I'm not gonna be able to test them, so basically if you say that is OK for merging, I'll trust you... @Dak0r, @wickedbeernut , and @raoulh let me know what you guys need from me and I'll do it to get this on the master branch. but if you guys really wanna help, we can schedule a quick meeting, get you guys as contributors for this plugin (access to this branch), and divide the work that I'm doing possibly making prod ready faster.... I'm now mostly busy with my masters, and thankfully fully recovered from my injury, so I wanna get back to this project as soon as I can....
wickedbeernut commented 2021-03-26 22:23:57 +00:00 (Migrated from github.com)

I don't think you want me on your team. I'm a firmware guy. I worked for IBM for seven years and retired from Cisco Systems after 20 years (age 50), both in the Silicon Valley. I have zero formal Python training.

I'll leave the Enclosure plugin "core" to you guys.

I'd be happy to help Dak0r extend his PR from temperature and humidity sensors to include temperature and humidity controllers. I discussed this with @jneilliii (PlotlyTempGraph) a little over a month ago.

My primary interest is in I2C GPIO inputs and outputs. It needs to be "I2C for Dummies" (like the existing BME280 and TMP102 support).

I don't think you want me on your team. I'm a firmware guy. I worked for IBM for seven years and retired from Cisco Systems after 20 years (age 50), both in the Silicon Valley. I have zero formal Python training. I'll leave the Enclosure plugin "core" to you guys. I'd be happy to help Dak0r extend his PR from temperature and humidity sensors to include temperature and humidity controllers. I discussed this with @jneilliii (PlotlyTempGraph) a little over a month ago. My primary interest is in I2C GPIO inputs and outputs. It needs to be "I2C for Dummies" (like the existing BME280 and TMP102 support).
Dak0r commented 2021-04-29 17:45:20 +01:00 (Migrated from github.com)

Hey, yeah as you can see from my late response, I'm all in for implementing a feature now and then, but I already have too many side projects to join a long term commitment... 😓
I'll update this PR to solve the conflicts as soon as I can, I'm still using my branch without any issues, so from my side there's nothing that would prevent a merge.
I'd see all discussed changes in a separate branch just to prevent this PR from blowing up. 👍

Hey, yeah as you can see from my late response, I'm all in for implementing a feature now and then, but I already have too many side projects to join a long term commitment... 😓 I'll update this PR to solve the conflicts as soon as I can, I'm still using my branch without any issues, so from my side there's nothing that would prevent a merge. I'd see all discussed changes in a separate branch just to prevent this PR from blowing up. 👍
Dak0r commented 2021-10-13 14:18:03 +01:00 (Migrated from github.com)

Hi everyone. I finally took the time to fix the merge conflicts for this PR.
I'm still using it on a regular basis, without any issues.

I have not tested it on Octoprint 1.7, yet.

@vitormhenrique Some month ago, I developed one small additional feature: An Auto Shutdown if an error was detected. Would you be ok if I include it in this PR? Like the graphs I have it running in my environment for months, working as expected: Output Feature: Shutdown on Serial Connection Error
I wanted to make a separate PR, but I fear that this one might be the last one that goes into the "old" Enclosure version 😅

Hi everyone. I finally took the time to fix the merge conflicts for this PR. I'm still using it on a regular basis, without any issues. I have not tested it on Octoprint 1.7, yet. @vitormhenrique Some month ago, I developed one small additional feature: An Auto Shutdown if an error was detected. Would you be ok if I include it in this PR? Like the graphs I have it running in my environment for months, working as expected: [Output Feature: Shutdown on Serial Connection Error](https://github.com/Dak0r/OctoPrint-Enclosure-with-Custom-Graphs/pull/3) I wanted to make a separate PR, but I fear that this one might be the last one that goes into the "old" Enclosure version 😅
vitormhenrique commented 2021-10-13 18:43:09 +01:00 (Migrated from github.com)

@Dak0r You can create another PR with the extra feature, I'll merge both.

I'll test on 1.7 this weekend and if anyone else can also test let me know.

@Dak0r You can create another PR with the extra feature, I'll merge both. I'll test on 1.7 this weekend and if anyone else can also test let me know.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Gandalf/OctoPrint-Enclosure#392