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

Disable PL debug mode, increasing power threshold for active channel #301

Merged

Conversation

MalteSchm
Copy link

@MalteSchm MalteSchm commented Jul 9, 2023

Smaller PR that:

  1. Disables debug printouts
  2. Increases the threshold for producing channel detection.
  3. Fixes an issue with maximum power limiting

Rationale:

  1. The PL was recently changed to no longer run in a 10s interval. This causes the debug printouts to be printed way more often. I believe this has lead to stability issues highlighted here Stability issues with development branch #298
    This PR disables the printouts for now which is kind of a hotfix

  2. My Inverter reports a power of 0.9W on a single channel when idle. The other channels indicate about 1.5W. This triggers the producing channels detection code and will scale a requested limit by a factor of 1.3 when starting up. This PR increases the detection threshold to 2W.

  3. When no active channels are detected the limit is not checked against the maximum inverter power. This PR makes this a mandatory check

Issues / Questions
Right now setNewPowerLimit enforces the maximum configured limit before the producing channel detection / scaling. This seems like a bug to me as the user would expect a UI configured value to be used.
Am I right here or is this intensional?

    // enforce configured upper power limit
    int32_t effPowerLimit = std::min(newPowerLimit, config.PowerLimiter_UpperPowerLimit);

    // scale the power limit by the amount of all inverter channels devided by
    // the amount of producing inverter channels. the inverters limit each of
    // the n channels to 1/n of the total power limit. scaling the power limit
    // ensures the total inverter output is what we are asking for.
    std::list<ChannelNum_t> dcChnls = inverter->Statistics()->getChannelsByType(TYPE_DC);
    int dcProdChnls = 0, dcTotalChnls = dcChnls.size();
    for (auto& c : dcChnls) {
        if (inverter->Statistics()->getChannelFieldValue(TYPE_DC, c, FLD_PDC) > 2.0) {
            dcProdChnls++;
        }
    }
    if (dcProdChnls > 0) {
        if (dcProdChnls != dcTotalChnls) {
          MessageOutput.printf("[PowerLimiterClass::setNewPowerLimit] %d channels total, %d producing channels, scaling power limit\r\n",
                  dcTotalChnls, dcProdChnls);
        }
        effPowerLimit = round(effPowerLimit * static_cast<float>(dcTotalChnls) / dcProdChnls);
    }

@schlimmchen
Copy link
Collaborator

  1. Thanks to the backtrace provided in Stability issues with development branch #298, the root cause for the instabilities you observed should be eliminated with DPL: do not use nullptr when printing debug messages #303. I understand that the DPL causes a lot of noise on the serial line and web UI console, but until the live view is equipped with (much) more DPL information, the debug messages are important to diagnose issues. Users that do not build their own firmware will not be able to provide additional information or properly understand what's going on when they have issues with the DPL. Would you agree that a setting would be helpful here? "Enable DPL debug messages", default "off"?
  2. I support this change. The problem is the exact same for me. I anticipate another long-term solution: Enabling this whole scaling-logic using a (per-inverter) setting.
  3. Where is this implemented? This changeset seems not to include this? I thought a while about this and I think I understand what you mean now. It confuses me that I overlooked this. Nice catch! I suggest effPowerLimit = std::min<int32_t>(effPowerLimit, inverter->DevInfo()->getMaxPower()); right after if (dcProdChnls > 0) { ... }.

Regarding you question: I read the setting "Upper power limit:" in the DPL settings like this: "Never make the inverter output more than this amount of power." Then it makes sense to scale the power limit beyond this setting, as we expect to scale the power limit such that the actual AC output power matches the "Upper power limit:" value. However, see my comment on enabling the whole scaling logic in point 2 above.

@MalteSchm
Copy link
Author

MalteSchm commented Jul 9, 2023

Thanks for spotting the issue on 3. that was what I meant. I did fix this in the code but I probably overwrote this change.
Updated the PR with your suggestion

I agree that the debug messages are very helpful. I also liked the status printouts that you implemented... Kudos!
The issue I see right now is that they are overwritten by the Hoymiles output which limits its use.

@schlimmchen
Copy link
Collaborator

Kudos!

Thanks 😊

I was looking at this again (coming from #300) and noticed that you only guard the message with if (dcProdChnls != dcTotalChnls), but the scaling is performed if (dcProdChnls > 0). I guess your new condition if (dcProdChnls != dcTotalChnls) is better and should guard both the console message and the actual scaling calculation.

@MalteSchm
Copy link
Author

Yeah I agree. I'll update that this evening
I'll also re-enable the logs for now.

@helgeerbe helgeerbe marked this pull request as draft July 11, 2023 07:43
@helgeerbe
Copy link
Owner

@MalteSchm let me know when this PR is ready to merge

@MalteSchm
Copy link
Author

MalteSchm commented Jul 11, 2023

@helgeerbe I'm done & ready to merge

@helgeerbe helgeerbe marked this pull request as ready for review July 12, 2023 11:19
@helgeerbe helgeerbe merged commit 95d7ac7 into helgeerbe:development Jul 12, 2023
8 checks passed
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