-
-
Notifications
You must be signed in to change notification settings - Fork 1
Generalise port implementation for IP comms in addition to serial #60
base: main
Are you sure you want to change the base?
Conversation
…and update testing
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
===========================================
- Coverage 79.26% 66.49% -12.77%
===========================================
Files 10 12 +2
Lines 786 982 +196
===========================================
+ Hits 623 653 +30
- Misses 152 310 +158
- Partials 11 19 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* chore(deps): bump golang.org/x/net from 0.33.0 to 0.38.0 Bumps [golang.org/x/net](https://github.com/golang/net) from 0.33.0 to 0.38.0. - [Commits](golang/net@v0.33.0...v0.38.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-version: 0.38.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * chore: bump versions --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: jadon <16850875+wolffshots@users.noreply.github.com>
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.4.14 to 5.4.18. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v5.4.18/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v5.4.18/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 5.4.18 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…est in /web (#72) * chore(deps): bump esbuild, @sveltejs/vite-plugin-svelte, vite and vitest Bumps [esbuild](https://github.com/evanw/esbuild) to 0.25.2 and updates ancestor dependencies [esbuild](https://github.com/evanw/esbuild), [@sveltejs/vite-plugin-svelte](https://github.com/sveltejs/vite-plugin-svelte/tree/HEAD/packages/vite-plugin-svelte), [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) and [vitest](https://github.com/vitest-dev/vitest/tree/HEAD/packages/vitest). These dependencies need to be updated together. Updates `esbuild` from 0.21.5 to 0.25.2 - [Release notes](https://github.com/evanw/esbuild/releases) - [Changelog](https://github.com/evanw/esbuild/blob/main/CHANGELOG-2024.md) - [Commits](evanw/esbuild@v0.21.5...v0.25.2) Updates `@sveltejs/vite-plugin-svelte` from 3.0.2 to 5.0.3 - [Release notes](https://github.com/sveltejs/vite-plugin-svelte/releases) - [Changelog](https://github.com/sveltejs/vite-plugin-svelte/blob/main/packages/vite-plugin-svelte/CHANGELOG.md) - [Commits](https://github.com/sveltejs/vite-plugin-svelte/commits/@sveltejs/vite-plugin-svelte@5.0.3/packages/vite-plugin-svelte) Updates `vite` from 5.4.18 to 6.3.1 - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/create-vite@6.3.1/packages/vite) Updates `vitest` from 1.6.1 to 3.1.1 - [Release notes](https://github.com/vitest-dev/vitest/releases) - [Commits](https://github.com/vitest-dev/vitest/commits/v3.1.1/packages/vitest) --- updated-dependencies: - dependency-name: esbuild dependency-version: 0.25.2 dependency-type: indirect - dependency-name: "@sveltejs/vite-plugin-svelte" dependency-version: 5.0.3 dependency-type: direct:development - dependency-name: vite dependency-version: 6.3.1 dependency-type: direct:development - dependency-name: vitest dependency-version: 3.1.1 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> * chore: commit tool versions and bump nodejs * chore: bump nodejs --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: jadon <16850875+wolffshots@users.noreply.github.com>
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 6.3.1 to 6.3.4. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v6.3.4/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 6.3.4 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [devalue](https://github.com/sveltejs/devalue) from 5.1.1 to 5.3.2. - [Release notes](https://github.com/sveltejs/devalue/releases) - [Changelog](https://github.com/sveltejs/devalue/blob/main/CHANGELOG.md) - [Commits](sveltejs/devalue@v5.1.1...v5.3.2) --- updated-dependencies: - dependency-name: devalue dependency-version: 5.3.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR generalizes the port implementation to support both serial and IP communications in preparation for running the app on a separate device from the one communicating with the inverter. The changes introduce a common interface for communication ports and add support for IP-based connections while maintaining backward compatibility with serial communication.
- Introduced a common
comms.Portinterface to abstract communication methods - Added IP communication support alongside existing serial communication
- Refactored code to use the new abstracted port interface throughout the application
Reviewed Changes
Copilot reviewed 33 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| comms/comms.go | Defines the common Port interface and connection types for both serial and IP communications |
| ip/ip.go | Implements IP-based communication port with TCP socket connection |
| serial/serial.go | Refactored to implement the common Port interface while maintaining serial-specific functionality |
| main.go | Updated configuration parsing and connection setup to support both serial and IP connection types |
| messages/*.go | Updated to use the common Port interface instead of serial-specific types |
| config.json.example | Updated configuration structure to support both connection types |
| diagnostics/diagnostics.go | Added new diagnostics system for error tracking and health monitoring |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if i := bytes.IndexByte(data, '\r'); i != -1 { | ||
| return string(data[:i+1]), nil | ||
| } |
Copilot
AI
Sep 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Read function has an infinite loop with no timeout handling. If no carriage return is received, the function will loop forever despite setting a read deadline. Consider adding a check for timeout errors or implementing proper timeout handling.
| if ip.Conn == nil { | ||
| return "", errors.New("ip port is not open") |
Copilot
AI
Sep 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message inconsistency: This returns 'ip port is not open' while the Write method returns 'ip port is nil on write'. Consider using consistent error messages like 'ip port is nil on read' to match the pattern used in serial implementation.
| if ip.Conn == nil { | |
| return "", errors.New("ip port is not open") | |
| return "", errors.New("ip port is nil on read") |
| mqtt.CRITICAL = log.New(os.Stdout, "[CRIT] ", 0) | ||
| mqtt.WARN = log.New(os.Stdout, "[WARN] ", 0) | ||
| mqtt.DEBUG = log.New(os.Stdout, "[DEBUG] ", 0) | ||
| // mqtt.DEBUG = log.New(os.Stdout, "[DEBUG] ", 0) |
Copilot
AI
Sep 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Commented out debug logging should be removed or made configurable rather than left as commented code. Consider using a configuration flag to enable/disable debug logging.
| Baud: configuration.Connection.Serial.Baud, | ||
| Retries: configuration.Connection.Serial.Retries, | ||
| } | ||
| log.Printf("Opening Serial port: %v\n", serialPort) |
Copilot
AI
Sep 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent capitalization in log messages. 'Serial port' should be 'serial port' to match the style of 'IP port' or vice versa for consistency.
| log.Printf("Opening Serial port: %v\n", serialPort) | |
| log.Printf("Opening serial port: %v\n", serialPort) |
| Port: configuration.Connection.IP.Port, | ||
| Retries: configuration.Connection.IP.Retries, | ||
| } | ||
| log.Printf("Opening IP port: %v\n", ipPort) |
Copilot
AI
Sep 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent capitalization in log messages. 'Serial port' should be 'serial port' to match the style of 'IP port' or vice versa for consistency.
This is in preparation for being able to run the app on a separate device to the one that actually communicates with the inverter.
The goal is to be able to run this project on my main server with a more beefy system and higher reliability than a Raspberry Pi (which is how it is currently deployed) and replace the Pi with an ESP board running esphome-stream-server powered by the serial port directly rather than having to externally power the Pi.
Long term goal is to Dockerise the project (#59) and then I can work on improving efficiency further and implementing more robust healthchecks which can restart the project properly. Currently it just dispatches a command to restart the systemd service when certain issues are encountered.