-
Notifications
You must be signed in to change notification settings - Fork 387
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
No description provided.