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

Bring back the sun indicator #195

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

MalteSchm
Copy link

@helgeerbe mentioned here: #172 (comment) that I broke the sun indicator for Power Limiter solar pass-through. I investigated and believe the code in this PR fixes this. It works for me but I hardly ever get direct SolarPassthrough in my setup.
Can you kindly check?

Note: I did not compare the MPPT power and the inverter power to indicate if the battery is actually being discharged or charged. Is that the desired behavior or shall this be done?
Note 2: The UI shows the last requested inverter power currently. This is correct as per methods used currently but also means that there is a power value shown case if the inverter is off. Should this indicate 0 in this case?

@helgeerbe
Copy link
Owner

helgeerbe commented Apr 27, 2023

It was much simpler

uint8_t PowerLimiterClass::getPowerLimiterState() {
     CONFIG_T& config = Configuration.get();

     std::shared_ptr<InverterAbstract> inverter = Hoymiles.getInverterByPos(config.PowerLimiter_InverterId);
     if (inverter == nullptr || !inverter->isReachable()) {
         // inactive icon
         return PL_UI_STATE_INACTIVE;
     }

     if (inverter->isProducing() && _batteryDischargeEnabled) {
      // battery icon
       return PL_UI_STATE_USE_SOLAR_AND_BATTERY;
     }

     if (inverter->isProducing() && !_batteryDischargeEnabled) {
       // sun icon
       return PL_UI_STATE_USE_SOLAR_ONLY;
     }

     if(!inverter->isProducing()) {
       // battery charging icon
       return PL_UI_STATE_CHARGING;
     }

     return PL_UI_STATE_INACTIVE;
 }

I know that the power number is not correct, then inverter is switched off. It was more the last set power.

@madmartin
Copy link

@MalteSchm can you rebase your three commits into one and do a "git push -f" ?
In all your PRs there are unnecessary many commits that make the main history look messy.
If you need assistance let me know.

@MalteSchm
Copy link
Author

Hi @madmartin

two questions:

  • The reason why there is a merge in this branch is that I made a commit followed by a push. Then I tried to amend that commit (to not create additional commits). This triggers this merge after which I need to commit again. So instead of doing one commit I end up doing three. Is there a suggested way to amend a commit that I've pushed to the origin without running into this issue?
  • When doing git rebase: What should be the new base? Is that "git rebase upstream/development" or "git rebase -i". The last time I tried this I ended up with multiple commits and it made things worse.

Sorry for the hassle. I have not really become accustomed to git yet.

@helgeerbe helgeerbe merged commit 7006055 into helgeerbe:development Apr 27, 2023
@madmartin
Copy link

@MalteSchm No problem, I try to guide you.

Whatever the reason is that you have 3 commits (or more on other pull requests=PR's) in your log on top of the original branch, in a PR all unnecessary stuff should not be there.
So in this PR, IMHO there should be only one commit. You can do an interactive rebase - I explain:

First look into your git log:

commit d4a054803fddde5ce08590c811cada0fee1d5505 
Merge: c054dca 29ca38b
Author: MalteSchm <malte.schmidt@gmx.de>
Date:   Thu Apr 27 11:46:18 2023 +0200

    Merge branch 'the_sunshine_branch' of https://github.com/MalteSchm/OpenDTU-OnBattery into the_sunshine_branch

commit c054dcab1e7e655f16127190390f81ef5cd63653
Author: MalteSchm <malte.schmidt@gmx.de>
Date:   Wed Apr 26 21:04:48 2023 +0200

    Adding states to display in UI

commit 29ca38b69a5024b4ce158b4aa1342e1e2491951f
Author: MalteSchm <malte.schmidt@gmx.de>
Date:   Wed Apr 26 21:04:48 2023 +0200

    Adding states to display in UI

commit 89209b6bf79cb467cb38ccb079f21aa0a5d56406 (origin/development)
Author: helgeerbe <helge@erbehome.de>
Date:   Wed Apr 26 15:23:17 2023 +0200

    Actions and badges reflects openDTU-onBattery now

commit e65b2196bfa5d91add0041cde5fd80b2d03de72d
Author: helgeerbe <helge@erbehome.de>
Date:   Wed Apr 26 12:53:50 2023 +0200

    add webapp

The newest commit is at the top. Search from top down to the first commit which is not from you, copy the commit hash (the first 8 digits are sufficient). In this example, it is

commit 89209b6bf79cb467cb38ccb079f21aa0a5d56406 (origin/development)

The execute git rebase -i 89209b6bf79cb467cb38ccb079f21aa0a5d56406 - then an editor is opened with a list of your commits, this time the OLDEST is at the top and NEWEST at the end of the list:

pick c054dca Adding states to display in UI
pick 29ca38b Adding states to display in UI

# Rebase 89209b6..d4a0548 onto 89209b6 (2 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
#                    commit's log message, unless -C is used, in which case
#                    keep only this commit's message; -c is same as -C but
#                    opens the editor
# x, exec <command> = run command (the rest of the line) using shell
# b, break = stop here (continue rebase later with 'git rebase --continue')
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
#         create a merge commit using the original merge commit's
#         message (or the oneline, if no original merge commit was
#         specified); use -c <commit> to reword the commit message
# u, update-ref <ref> = track a placeholder for the <ref> to be updated
#                       to this position in the new commits. The <ref> is
#                       updated at the end of the rebase
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#

Read the instructions in the comment section. Take note that the merge commit is not shown, because it does not contain any changed file content.
I recommend you to "sqash" the two commits into one.
To do this, go to the second line and change the word "pick" to "s" or "squash".

pick c054dca Adding states to display in UI
s 29ca38b Adding states to display in UI

# Rebase 89209b6..d4a0548 onto 89209b6 (2 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
...

Then leave the editor, writing the document.

When git is able to put these commits together, the rebase is successful. Here, you have changed some lines two times, which gives a "merge conflict".
Git aborts the rebase at this point and writes detailed instructions what you should do.

Auto-merging include/PowerLimiter.h
CONFLICT (content): Merge conflict in include/PowerLimiter.h
Auto-merging src/PowerLimiter.cpp
CONFLICT (content): Merge conflict in src/PowerLimiter.cpp
error: could not apply 29ca38b... Adding states to display in UI
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 29ca38b... Adding states to display in UI

Now all conflicting files must be edited (what have you done here???). Every section looking like this

<<<<<<< HEAD
lalalala
=======
huhuhuhu
>>>>>>> 29ca38b (Adding states to display in UI)

must be cleaned up - delete the unwanted content together with the <<< and === and >>> lines.

Example in "include/PowerLimiter.h":

before:

<<<<<<< HEAD
#define PL_UI_STATE_INACTIVE 0
#define PL_UI_STATE_CHARGING 1
#define PL_UI_STATE_USE_SOLAR_ONLY 2
#define PL_UI_STATE_USE_SOLAR_AND_BATTERY 3
=======
#define PL_UI_STATE_DISABLED 0
#define PL_UI_STATE_OFF 1
#define PL_UI_STATE_SOLAR_ONLY 2
#define PL_UI_STATE_SOLAR_AND_BATTERY 3
>>>>>>> 29ca38b (Adding states to display in UI)

after:

#define PL_UI_STATE_INACTIVE 0
#define PL_UI_STATE_CHARGING 1
#define PL_UI_STATE_USE_SOLAR_ONLY 2
#define PL_UI_STATE_USE_SOLAR_AND_BATTERY 3

Then - like printed out in the instructions from the rebase abort:

git add include/PowerLimiter.h src/PowerLimiter.cpp

git rebase --continue

Next, git presents you the usual commit editor for the one squashed commit, which you should edit (because it contains the summary of all commit messages).

When finished, your git log will look like this:

commit 79dbf17fbb9cd36cb44d49dc4078e4b21e380fd8
Author: MalteSchm <malte.schmidt@gmx.de>
Date:   Wed Apr 26 21:04:48 2023 +0200

    Adding states to display in UI

commit 89209b6bf79cb467cb38ccb079f21aa0a5d56406 (origin/development)
Author: helgeerbe <helge@erbehome.de>
Date:   Wed Apr 26 15:23:17 2023 +0200

    Actions and badges reflects openDTU-onBattery now

commit e65b2196bfa5d91add0041cde5fd80b2d03de72d
Author: helgeerbe <helge@erbehome.de>
Date:   Wed Apr 26 12:53:50 2023 +0200

    add webapp

When you are happy with the outcome, you must execute git push -f to force-push the modified branch to github. Your pullrequest is updated instantly, and has only one commit then.

While writing this instructions, I see that @helgeerbe has merged your PR, so you this is a little obsolete, but please keep this on your records for your next pull request.

@MalteSchm
Copy link
Author

MalteSchm commented Apr 28, 2023

thanks @madmartin
I also saw that this has been merged but I walked through the steps and ended up with a single commit in my branch
(thanks a lot ! for the detailed explanation)

... not sure where you got the lalala lines from but I'd be very surprised if I wrote them.

@helgeerbe
The code currently checked in development still checks for MPPT producing to detect a battery charging

    if(VeDirect.veFrame.PPV > 0) {
      return PL_UI_STATE_CHARGING;
    }

whereas you suggested above to check if the inverter is not producing:

     if(!inverter->isProducing()) {
       // battery charging icon
       return PL_UI_STATE_CHARGING;
     }

I changed this yesterday but could not commit as we had a network outage. I updated the branch just now. Let me know if this should be changed.

@MalteSchm
Copy link
Author

@madmartin
I tried this on another branch https://github.com/MalteSchm/OpenDTU-OnBattery/tree/toggle_inverter_fix but this approach leads me into an infinite loop.
upon completion of git rebase (after selecting the commits, merging and doing the git rebase --continue(s)) I cannot commit the result

$ git rebase --continue
Successfully rebased and updated refs/heads/toggle_inverter_fix.
$ git push
To https://github.com/MalteSchm/OpenDTU-OnBattery.git
 ! [rejected]        toggle_inverter_fix -> toggle_inverter_fix (non-fast-forward)
error: failed to push some refs to 'https://github.com/MalteSchm/OpenDTU-OnBattery.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

From there if I do a git pull I need to merge once more. This needs to be followed by rebase again to get rid of the additional commits. I keep arriving at this point with more and more commits in my branch.

Git and I are no real friends yet

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.

3 participants