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

Issue 14 Fix & Add Timestamps #15

Closed
wants to merge 14 commits into from

Conversation

sirkingchase
Copy link
Contributor

This pull request is to fix the issue that is encountered when installing the package as described here: #14 (comment)

This PR also includes the addition of Timestamps to the emeter json value and it adds a new json topic to the switches that contain the status value & timestamp. The timestamps are needed for MQTT v3 persistence to work with QOS > 0

Copy link
Owner

@flavio-fernandes flavio-fernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for doing this! Please take a pass on the changes I asked and see if they make sense.

@@ -39,6 +39,16 @@ locations:
# example where we indicate a specific poll interval.
# Also, adding a task to publish emeter info at provided interval


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... these changes are a bit too specific to you. How about creating a data/config.yaml.sirkingchase and leaving the file as it was originally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is my config, I meant to exclude it. I couldnt figure out how to edit my PR after it was submitted.

@@ -114,7 +114,7 @@ locations:
host: 192.168.66.144
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep these on a separate file.

@@ -21,7 +21,7 @@ globals:
topic_format: /{}/switch
# kasa will monitor the current state of the device every
# poll interval, in seconds. You can override on a per device
poll_interval: 60
poll_interval: 55
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious... why 55?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is my consumers poll at 60 second intervals so I wanted their to be an overlap between the time kasa pushes the metric and it is consumed. However, I have MQTT queuing working not so it doesnt matter.

WORKDIR /src
RUN python3 setup.py install

ENTRYPOINT ["python3", "/src/mqtt2kasa/main.py", "/src/data/config.yaml"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, please add end of line

@@ -1,4 +1,4 @@
aiomqtt < 2.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... is this PR also adressing #13 ? I suspect not, which makes me think we should not change this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I had some difficulty getting everything to install from scratch but I dont remember the exact error or reason I had to do this, I think it could be because of the version thats in pip

@@ -3,25 +3,25 @@
knobs:
# Note: normally you don't set these... here just to show how to
# devel and debug
# log_to_console: false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create data/config.yaml.sirkingchase and leave this one as is. It is here mostly as an example and should be complete but simple.

@@ -0,0 +1,21 @@
#!/bin/bash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I am hoping we could move the files here, instead of duplicating them. If that is intentional, then maybe we could consider using symbolic links. Let me thinks a bit more about this.

@@ -1 +1,4 @@
Flavio Fernandes <flavio-fernandes@users.noreply.github.com>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove lines 1 and 4

@@ -0,0 +1,174 @@
#!/usr/bin/env python
Copy link
Owner

@flavio-fernandes flavio-fernandes Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much duplication... is that intentional?

@flavio-fernandes
Copy link
Owner

Hi @sirkingchase! Sorry for the delay in addressing this PR. Do you have any updates regarding the changes I suggested? If not, no worries. I welcome your contributions and will try to review them soon, including adding the timestamp you need. However, I'm not sure exactly when I will be able to do so.

Regarding the Dockerfile, the changes seem a bit disorganized. Could you please take another look and possibly clean up the modifications? This would greatly enhance the readability and maintainability of the Docker configuration. Thanks for your efforts!

@sirkingchase
Copy link
Contributor Author

Hi @sirkingchase! Sorry for the delay in addressing this PR. Do you have any updates regarding the changes I suggested? If not, no worries. I welcome your contributions and will try to review them soon, including adding the timestamp you need. However, I'm not sure exactly when I will be able to do so.

Regarding the Dockerfile, the changes seem a bit disorganized. Could you please take another look and possibly clean up the modifications? This would greatly enhance the readability and maintainability of the Docker configuration. Thanks for your efforts!

Hey the timestamp enhancement was included in the PR, I am not sure why its showing that code as a duplicate but there should be the timestamp code in there. FYI I have been using it for the last 3 months and it does exactly what I want - it queues the messages in Mosquitto MQTT and when by consumer comes online it processes the stored backlog.

@flavio-fernandes
Copy link
Owner

Nice. I'm happy to hear it is working for you! What I will do then is to squash all your changes and give it a polish.
Then I will ping you to have a look and confirm it looks good to you. My only ask is for a ton of patience from you,
as I still am not sure when I will get to play with this. #etoomanyballsintheair :)

@sirkingchase
Copy link
Contributor Author

Thanks, I'd like to help with getting the individual emeter values from kasa strips as well (currently this project only has total for entire strip & not individual plugs)

I found this project that has code that does it. I'll work on replicating it here eventually so you may see another pull request from me (I'll try to make it better next time)

The function that loops through the strip is one 162 here: https://github.com/mjonuschat/kasa-plug-exporter/blob/main/kasa_plug_exporter/app.py

flavio-fernandes added a commit that referenced this pull request May 27, 2024
Author: Chase <sirkingchase@gmail.com>
Date:   Fri Mar 29 14:32:25 2024 -0500

added json status to switches with timestamp for MQTT persistantance

Fixes: #15

Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
Co-authored-by: Chase <sirkingchase@gmail.com>
flavio-fernandes added a commit that referenced this pull request May 27, 2024
Author: Chase <sirkingchase@gmail.com>
Date:   Fri Mar 29 14:32:25 2024 -0500

Added JSON status to switches with timestamp for MQTT persistence.

This is a squash of #15
with a few tweaks. Hopefully, it will provide an equivalent solution
to what is addressed in that pull request.

Fixes: #14

Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
Co-authored-by: Chase <sirkingchase@gmail.com>
@flavio-fernandes
Copy link
Owner

Thanks, I'd like to help with getting the individual emeter values from kasa strips as well (currently this project only has total for entire strip & not individual plugs)

I found this project that has code that does it. I'll work on replicating it here eventually so you may see another pull request from me (I'll try to make it better next time)

The function that loops through the strip is one 162 here: https://github.com/mjonuschat/kasa-plug-exporter/blob/main/kasa_plug_exporter/app.py

Let's address that as part of #17

@flavio-fernandes
Copy link
Owner

Closing this PR: it is being folded into #22

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

Successfully merging this pull request may close these issues.

2 participants