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

Add TCP source for lightweight, extended data gathering #134

Merged
merged 12 commits into from
May 31, 2022

Conversation

MarijnS95
Copy link
Contributor

Certain Omnik inverters (supposedly with a specific serial number, see sources below) reply with its statistics as binary data when a "magic" packet is sent to them on port 8899 over TCP. This data is more lightweight than an HTML page or JavaScript source with values embedded, and provides more statistics then either similar to what can be found on the LCD display.

The data format and meaning has been extracted from various Python/C sources (see links below), with some of my own "work" to simplify checksum calculation and "the reverse hexadecimal serial number" is nothing more than its little-endian byte representation. Response data is parsed at once with a ctypes.BigEndianStructure instead of unpacking individual ranges of bytes through struct.unpack. Only minimal data massaging is needed to convert the struct to useful information.

It provides the following extra fields:

  • Inverter temperature;
  • Voltage and current for the DC input strings (up to 3);
  • Voltage, current, frequency and power for all AC outputs (also up to 3);
  • Total number of runtime hours (not sure if this is just the Omnik being on, or the time it delivers >0W of power back to the network).

No "device" (network module) data is found in any of the unparsed bits.

I have also not been able to extract solar_rated_power from the data. Comparing to http://www.mb200d.nl/wordpress/2015/11/omniksol-4k-tl-wifi-kit/#more-590 it seems the second byte in the "magic" is 0x81 on a 4k-TL instead of the 0x7d on our 3k-TL. It is just speculation thus far, but this might be an integer identifier of the hardware type? (To be confirmed by the community, I guess). Otherwise padding0 contains a lot of non-zero data too.

Perhaps anyone is interested in dumping the remaining data and seeing if they can uncover their meaning. We could help this by printing the padding fields if they're not what we expect (mostly zero).

Sources:

  1. https://github.com/jbouwh/omnikdatalogger/blob/dev/apps/omnikdatalogger/omnik/InverterMsg.py
  2. http://www.mb200d.nl/wordpress/2015/11/omniksol-4k-tl-wifi-kit/#more-590
  3. https://github.com/Woutrrr/Omnik-Data-Logger
  4. https://github.com/t3kpunk/Omniksol-PV-Logger

@klaasnicolaas klaasnicolaas added the new-feature New features or request. label Feb 20, 2022
@klaasnicolaas
Copy link
Owner

Thank you for your contribution,

It's a pretty big change and is partly beyond my cap. Despite that, I think it would be nice to see what possibilities there are with the various inverters. Could you see if you can pass the linting tests?

@MarijnS95
Copy link
Contributor Author

It's a pretty big change

I can try to split this up (in separate commits and/or PRs) if that helps? Don't immediately see a clear boundary though.

and is partly beyond my cap.

Should go without saying that I'll be utilizing this python package (through https://github.com/robbinjanssen/home-assistant-omnik-inverter once I submit related changes matching this PR there), and I'll be sure to stick around this repository to help out with review and clarification regarding the TCP connection "backend". I'm subscribed to notifications but feel free to @ me if anything shows up!

That's also important regarding the unknown fields and potential changes in the "magic", as detailed in the PR description.

Despite that, I think it would be nice to see what possibilities there are with the various inverters.

Yeah, I hope https://github.com/robbinjanssen/home-assistant-omnik-inverter is open to add logging for all the new fields. Besides temperature most is just a gimmick (though being able to see string balancing could be relevant?), but it'd be interesting to see if one of the unknown fields (or "magic") change across various inverters.

Could you see if you can pass the linting tests?

Thanks, I'll of course make sure this adheres to all the checks! Bit Rusty (:smirk: I write Rust now daily) on the Python side, I hope the code is at least in an acceptable state as it's quite tricky to find the right - concise - ways to write things in Python again. I managed to replicate most failing tests locally, will update this PR soon.

@MarijnS95
Copy link
Contributor Author

Another two things that came up:

  • Perhaps we can support multiple source types;
    • Use auth-less js to extract the serial number automatically, before attempting to use tcp going forward;
    • Send a js request to gather Device information in addition to the tcp request with extended statistics.
  • Maybe more Python Omnik implementations are willing to switch over to this package instead of maintaining a copy of "the TCP code" themselves.

@klaasnicolaas
Copy link
Owner

Could you DM me on one of my socials? I would like to invite you to our slack server, where both me and Robbin are in for the whole omnik development (python package and integration). Also more convenient for the quick communication 😉

@MarijnS95
Copy link
Contributor Author

@klaasnicolaas I think you need just my email for that, which you can pull straight from the commit body :)

Comment on lines 48 to 64
class TcpParser:
"""Functions for interacting with the Omnik over TCP."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since all of these are @classmethod anyway, perhaps these functions should just move into the module directly? Seems like an unnecessary indirection in hindsight.

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #134 (bf54d05) into main (548edbb) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #134    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            4         6     +2     
  Lines          182       325   +143     
  Branches        38        65    +27     
==========================================
+ Hits           182       325   +143     
Impacted Files Coverage Δ
omnikinverter/const.py 100.00% <100.00%> (ø)
omnikinverter/exceptions.py 100.00% <100.00%> (ø)
omnikinverter/models.py 100.00% <100.00%> (ø)
omnikinverter/omnikinverter.py 100.00% <100.00%> (ø)
omnikinverter/tcp.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 548edbb...bf54d05. Read the comment docs.

@MarijnS95 MarijnS95 force-pushed the tcp branch 2 times, most recently from c214621 to eb404e7 Compare February 21, 2022 21:41
@klaasnicolaas klaasnicolaas marked this pull request as ready for review February 21, 2022 21:48
@klaasnicolaas klaasnicolaas changed the title Add TCP source for lightweight, extended data gathering WIP: Add TCP source for lightweight, extended data gathering Feb 21, 2022
@MarijnS95 MarijnS95 force-pushed the tcp branch 3 times, most recently from aee05c3 to 2589402 Compare February 23, 2022 23:18
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale There has not been activity on this issue or PR for quite some time. label Mar 26, 2022
@klaasnicolaas klaasnicolaas added no-stale This issue or PR is exempted from the stable bot. and removed stale There has not been activity on this issue or PR for quite some time. labels Mar 26, 2022
@MarijnS95 MarijnS95 force-pushed the tcp branch 2 times, most recently from bd72be4 to 0118436 Compare March 26, 2022 21:57
.github/workflows/typing.yaml Outdated Show resolved Hide resolved
omnikinverter/omnikinverter.py Show resolved Hide resolved
test_output.py Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the tcp branch 2 times, most recently from 1d1e84e to 935ab48 Compare March 26, 2022 23:00
Comment on lines 427 to 432
socket_mock.recv.side_effect = Exception("Connection broken")

with pytest.raises(Exception) as excinfo:
assert await client.inverter()

assert str(excinfo.value) == "Connection broken"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure what's going on here. pylint wants me to test:

except Exception as exception:
raise OmnikInverterConnectionError(
"Failed to communicate with the Omnik Inverter device over TCP"
) from exception

Yet I can't check for that specific exception here, even though it is clearly visible in the logs. Instead, I have to check for the much less specific Exception("Connection broken") here that is raised by asynctest for us.

Copy link
Contributor Author

@MarijnS95 MarijnS95 May 15, 2022

Choose a reason for hiding this comment

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

I found what is going on, and the details are in comparing raise ... from ... re-wrapping (with The above exception was the direct cause of the following exception:) working in test_connection_failed(), but not in this test_connection_broken(self) which shows During handling of the above exception, another exception occurred:.

Turns out the exception is ALSO raised in the finally: block through the .wait_closed() call, directly while we were handling OSError and calling raise OmnikInverterConnectionError(...) from ..., as per the During handling of the above exception, another exception occurred:.

@MarijnS95 MarijnS95 force-pushed the tcp branch 2 times, most recently from 7e0ebc6 to 02e42e5 Compare March 30, 2022 07:47
MarijnS95 added a commit to MarijnS95/home-assistant-omnik-inverter that referenced this pull request Apr 27, 2022
[robbinjanssen#134] introduces a TCP connection backend to the `python-omnikinverter`
package, whch is "lower" in overhead when gathering data from the
inverter while at the same time providing more statistics.  Entities for
these additional statistics - as well as some that are currently missing
- will be added in a separate PR.  Note that device information
(firmware, IP address and WiFi signal strength) is unavailable through
this API.

[robbinjanssen#134]: klaasnicolaas/python-omnikinverter#134
MarijnS95 added a commit to MarijnS95/home-assistant-omnik-inverter that referenced this pull request Apr 27, 2022
[robbinjanssen#134] introduces a TCP connection backend to the `python-omnikinverter`
package, whch is "lower" in overhead when gathering data from the
inverter while at the same time providing more statistics.  Entities for
these additional statistics - as well as some that are currently missing
- will be added in a separate PR.  Note that device information
(firmware, IP address and WiFi signal strength) is unavailable through
this API.

[robbinjanssen#134]: klaasnicolaas/python-omnikinverter#134
Certain Omnik inverters (supposedly with a specific serial number, see
sources below) reply with its statistics as binary data when a "magic"
packet is sent to them on port 8899 over TCP.  This data is more
lightweight than an HTML page or JavaScript source with values embedded,
and provides more statistics then either similar to what can be found on
the LCD display.

The data format and meaning has been extracted from various Python/C
sources (see links below), with some of my own "work" to simplify
checksum calculation and "the reverse hexadecimal serial number" is
nothing more than its little-endian byte representation.  Response data
is parsed at once with a `ctypes.BigEndianStructure` instead of
unpacking individual ranges of bytes through `struct.unpack`.  Only
minimal data massaging is needed to convert the struct to useful
information.

It provides the following extra fields:

- Inverter temperature;
- Voltage and current for the DC input strings (up to 3);
- Voltage, current, frequency and power for all AC outputs (also up to
  3);
- Total number of runtime hours (not sure if this is just the Omnik
  being on, or the time it delivers >0W of power back to the network).

No "device" (network module) data is found in any of the unparsed bits,
nor is `solar_rater_power` available.

Sources:

1. https://github.com/jbouwh/omnikdatalogger/blob/dev/apps/omnikdatalogger/omnik/InverterMsg.py
2. http://www.mb200d.nl/wordpress/2015/11/omniksol-4k-tl-wifi-kit/#more-590
3. https://github.com/Woutrrr/Omnik-Data-Logger
4. https://github.com/t3kpunk/Omniksol-PV-Logger
tests/test_models.py Outdated Show resolved Hide resolved
MarijnS95 added a commit to MarijnS95/home-assistant-omnik-inverter that referenced this pull request May 15, 2022
The [TCP backend in python-omnikinverter] emits, in addition to existing
fields (but without any info on the WiFi "Device"):
- Inverter temparature;
- Total hours of inverter uptime;
- Voltage and current of a total of 3 DC input strings;
- Voltage, current, frequency and power for AC output (also 3...).

[TCP backend in python-omnikinverter]: klaasnicolaas/python-omnikinverter#134

This requires new types here as the DC input and AC output values are
provided in arrays.
@MarijnS95 MarijnS95 requested a review from klaasnicolaas May 16, 2022 17:41
@MarijnS95 MarijnS95 changed the title WIP: Add TCP source for lightweight, extended data gathering Add TCP source for lightweight, extended data gathering May 16, 2022
@klaasnicolaas klaasnicolaas merged commit 37e71ca into klaasnicolaas:main May 31, 2022
@MarijnS95 MarijnS95 deleted the tcp branch May 31, 2022 12:43
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-feature New features or request. no-stale This issue or PR is exempted from the stable bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants