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

Enable / Disable power limiter via MQTT #172

Merged
merged 47 commits into from
Apr 26, 2023

Conversation

MalteSchm
Copy link

This adds basic MQTT control of the power limiter.

The change is larger as I reworked the state machine code and the calc/set functions (there is a separate PR pending in #164)

Rationale for refactoring the existing code was:

  • issues with stopping / start the inverter
  • the code depth and code complexity

Based on my understanding all the old functionality should be enabled, but please let me know if I missed to cover a case.

My original plan was to provide these changes piece wise so that the amount of changes is not overwhelming but I'm currently not clear if an approach with

  • smaller individual changes
  • one large change
  • or no refactoring

is actually preferred.

@madmartin
Copy link

Hello @MalteSchm
please allow me a comment from behind the fence...
If this would be my project, I would ask you to rebase your pull request to a senseful number of commits. The merges with the development branch look ugly in the main project history when this PR gets merged as-is.
Good guide: https://www.atlassian.com/de/git/tutorials/rewriting-history/git-rebase

@MalteSchm
Copy link
Author

MalteSchm commented Apr 13, 2023

Hello @MalteSchm please allow me a comment from behind the fence... If this would be my project, I would ask you to rebase your pull request to a senseful number of commits. The merges with the development branch look ugly in the main project history when this PR gets merged as-is. Good guide: https://www.atlassian.com/de/git/tutorials/rewriting-history/

Thanks this is good input. I'll try to do this.
I'm still learning how to contribute using git/PRs.

EDIT: I tried rebase --interactive but that creates even more commits

@helgeerbe
Copy link
Owner

What is the state of this PR? I see some pending comments.

@MalteSchm
Copy link
Author

What is the state of this PR? I see some pending comments.

Hi @helgeerbe

this code has been working in my setup for some time now. The comment is about the # of commits. I can submit a PR with fewer commits if that is what it takes to integrate this. I would probably close this PR then and create a new PR and start all over once more..
In addition I have some further changes in the pipeline for the Power Limiter. I implemented an option to steer the inverter power based on the MPPT power when the battery is full (to supply excess solar power to the grid). This can be added to this PR if desired, or I create an incremental PR for this change.

@helgeerbe
Copy link
Owner

Hi @MalteSchm there is a pending comment from me, regarding the target power consumption. From my point of view, this shouldn't be uses when solar passthrough is enabled.

@MalteSchm
Copy link
Author

Hi @helgeerbe I feel embarrassed but I have to ask: How do I find this comment? I've kept looking for the last 10 minutes or so using the "files changed" link above but I'm not able to find it.
Can you provide a pointer?

@helgeerbe
Copy link
Owner

image

@helgeerbe
Copy link
Owner

@MalteSchm
Copy link
Author

MalteSchm commented Apr 25, 2023

Thanks @helgeerbe but it is odd. I don't see your review comment. I guess it is because of this feature: https://github.com/orgs/community/discussions/23138

Anyway... Your screenshot was good enough to understand the problem. I addressed this in commit 322f532

I did stumble across another case that was problematic (use of lower inverter limit when on old / outdated data). This would discharge the battery even if if use of solarPowerOnly was requested. This can happen for prolonged periods of time if the power meter input fails for some reason. I fixed this too

I did re-order few calculations to simplify things further and also touched the solar power calculation code. I hope these changes make sense to you

A quick check with my setup was successful but I would wait for tonight to see if this works as expected

@MalteSchm
Copy link
Author

@helgeerbe Status is:
Yesterday I performed some manual tests and discovered and fixed a remaining bug (commit #c7505ea).
Then I had this code running over night with a complete cycle on the battery and observed no problems.

Right now I don't plan further changes to this branch (unless I get feedback / bug reports)

@helgeerbe helgeerbe merged commit c337df6 into helgeerbe:development Apr 26, 2023
@helgeerbe
Copy link
Owner

@MalteSchm I merged your PR but I'm not quite happy with it.
In States you just keep ACTIVE instead of normal and solarpassthrough. In live view I only have the battery icon. In the past a sun indicated that solar passthrough was active.

Copy link

github-actions bot commented Apr 6, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants