-
Notifications
You must be signed in to change notification settings - Fork 438
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
DRAFT - Implementation DEYE SUN-12K-SG04LP3-EU #2614
base: develop
Are you sure you want to change the base?
Conversation
@Rayleigh3105 this is 100% not finished, maybe consider marking this PR as draft as I and the others know based on the "Copy-Paste" it is not finished yet - will add some comments beforehand :) See for e.g.: https://openems.github.io/openems.io/openems/latest/contribute/coding-guidelines.html Your complete Checkstyle is missing, there are no tests, and so on and so forth |
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.
To maintain transparency, I believe it would be more efficient for @sfeilmeier if you could address the issues in this pull request before resubmitting it. Like you, I am diligently working on my pull requests and acknowledge that I am not without fault. Based on my observations, @sfeilmeier is undoubtedly busy and appreciates contributions. However, he also faces the challenge of reviewing a significant amount of code that may be redundant, unnecessary, or incorrect in some pull requests, including some of my own.
@AttributeDefinition(name = "Modbus-Unit-ID", description = "Unit ID of Modbus bridge.") | ||
int unit_id() default 0; | ||
|
||
@AttributeDefinition(name = "Power limit on PowerDecreaseCausedByOvertemperature error; '0' to disable power limit logic", description = "") |
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.
What does this Config Point do?
Maybe consider changing it to something like:
@AttributeDefinition(name = "ThermalPowerLimitControl", description = "Defines the power limit adjustment in response to overtemperature events. Set to '0' to disable.")
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.
Done - Removed.
@AttributeDefinition(name = "Surplus Feed-In: PV-Limit on PowerDecreaseCausedByOvertemperature", description = "") | ||
int surplusFeedInPvLimitOnPowerDecreaseCausedByOvertemperature() default 5_000; | ||
|
||
String webconsole_configurationFactory_nameHint() default "Deye [{id}]"; |
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.
Deye what? Maybe also consider to refactor the Config if all of the Settings are needed for this "quite Simple" ESS ? As this all is copied from Fenecon Commercial 40 personally I do not think it is needed to set all these things up here
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.
Done.
private static final int MIN_REACTIVE_POWER = -10000; | ||
|
||
private static final int MAX_REACTIVE_POWER = 10000; | ||
|
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.
why are these for e.g. Hard Coded ? Aren't there more then one Combinations Possible ?
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.
This is just copied from commerical 40. Dont know if I need these.
ModbusComponent.ChannelId.values(), // | ||
SymmetricEss.ChannelId.values(), // | ||
ManagedSymmetricEss.ChannelId.values(), // | ||
HybridEss.ChannelId.values(), // |
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.
Are all of these really needed ? Seems like most of them are Commercial specific...
|
||
private LocalDateTime lastDefineWorkState = null; | ||
|
||
private void defineWorkState() { |
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.
Is there a "Work State" on Deye ?
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.
return new Constraint[] { // | ||
this.createPowerConstraint("Commercial40 Min Reactive Power", Phase.ALL, Pwr.REACTIVE, | ||
Relationship.GREATER_OR_EQUALS, MIN_REACTIVE_POWER), // | ||
this.createPowerConstraint("Commercial40 Max Reactive Power", Phase.ALL, Pwr.REACTIVE, |
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.
Commercial 40 ?
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.
Done.
public class SurplusFeedInHandler { | ||
|
||
private static final int GOING_DEACTIVATED_MINUTES = 15; | ||
private static final float PV_LIMIT_FACTOR = 0.9f; |
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.
All of these values are Commercial 40 related i guess..
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.
Removed.
var areSurplusConditionsMet = this.areSurplusConditionsMet(this.parent, chargers, config); | ||
|
||
if (chargers.isEmpty()) { | ||
// Is no Charger set (i.e. is this not a Commercial 40-40 "DC") |
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.
Again commercial 40 related ?
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.
Removed.
25220d3
to
a308f1b
Compare
@Rayleigh3105 you completely reverted my changes again.. I am sorry but as @sfeilmeier also told me please consider looking into the getting started and also learn some things about github before creating a PR please.. also use the getting started please ! |
@Rayleigh3105 reformatted it again, removed unused Classes again, please do not revert as it is not needed :) |
@Sn0w3y Ahh sorry never wanted to create a PR this early. I just played around in gitHub. I´m trying to sort out the code in the next days 😆 |
final Consumer<Value<Integer>> calculate = ignore -> { | ||
final Integer generatorPower = meter.getActivePowerGen().getNextValue().get(); | ||
Integer generatorPowerToSet = 0; | ||
|
||
if (generatorPower > 32768) { | ||
generatorPowerToSet = generatorPower - 65536; | ||
} else { | ||
generatorPowerToSet = generatorPower; | ||
} | ||
|
||
meter._setActivePower(TypeUtils.sum(// | ||
meter.getActivePowerS1Channel().getNextValue().get(), // | ||
meter.getActivePowerS2Channel().getNextValue().get(), // | ||
meter.getActivePowerS3Channel().getNextValue().get(), // | ||
meter.getActivePowerS4Channel().getNextValue().get(), generatorPowerToSet)); // | ||
}; | ||
meter.getActivePowerS1Channel().onSetNextValue(calculate); | ||
meter.getActivePowerS2Channel().onSetNextValue(calculate); | ||
meter.getActivePowerS3Channel().onSetNextValue(calculate); | ||
meter.getActivePowerS4Channel().onSetNextValue(calculate); | ||
meter.getActivePowerGen().onSetNextValue(calculate); |
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.
@Sn0w3y Is this the right way to implement the Gen Port on a inverter?
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.
What do you mean with "Gen Port" ?
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.
Deye does have a port to plug in a Generator source. Like a Diesel generator. So the values what it generates are now written to active power.
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.
Yeah never meant to open the PR in this stage, did forgot to make it a draft. Thanks for your participation. |
@Rayleigh3105 could you please check if the Code works for your Deye ? |
da67180
to
e05dded
Compare
edc660d
to
240240f
Compare
I took some time to review as the Deye inverter is popular and it would be nice to have it in standard. https://community.openems.io/t/implementation-deye-inverter-approach/2541/16. But honestly at the moment the code is not very good. Most part is just copied from FENECON Commercial 40, with lots of unused channels etc.. Also most JUnit tests are missing at the moment. Please somebody else take over from here... :-) |
Codecov ReportAttention: Patch coverage is ❌ Your patch check has failed because the patch coverage (3.48%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #2614 +/- ##
=============================================
+ Coverage 58.42% 60.46% +2.04%
Complexity 139 139
=============================================
Files 2477 2213 -264
Lines 105240 98557 -6683
Branches 7770 6678 -1092
=============================================
- Hits 61480 59580 -1900
+ Misses 41518 36823 -4695
+ Partials 2242 2154 -88 |
No description provided.