-
Notifications
You must be signed in to change notification settings - Fork 413
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
Multi use addition to symmetric peak shaving - Q3/24 Hackathon #2794
base: develop
Are you sure you want to change the base?
Conversation
@sfeilmeier review |
@@ -28,6 +28,16 @@ | |||
|
|||
@AttributeDefinition(name = "Recharge power", description = "If grid purchase power is below this value battery is recharged.") | |||
int rechargePower(); | |||
|
|||
@AttributeDefinition(name = "Enable multiple ess constraints", description = "A fixed capacity configured by the minSocLimit is used for peakshaving. Additional capacity could be used by other controllers.") | |||
boolean enableMultipleEssConstraints() default false; |
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.
Do we really need this? Is there any scenario where we need this, if we set minSocLimit to 100?
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.
Hi @parapluplu,
in a few controllers and components we used this approach to have a defined behaviour that could be enabled or disabled. (Good example: GoodWeBatteryInverter, Bad example: Evcs Controller using power 0 for Mode OFF)
If you use this configuration property in an APP, it is also easier to maintain/explain.
ess.setActivePowerGreaterOrEquals(calculatedPower); | ||
} | ||
} | ||
|
||
ess.setReactivePowerEquals(0); |
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.
Do we always have to set reactive power? Or leave it over to other controllers in case we're not in a peak shaving or force recharging for peak shaving scenario?
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.
Could be a better approach, yes - Reactive Power Applications could run when it is not forced to do peakshaving. Do you think you could change it inside? Or @Laksh26 ?
.input(ESS_ACTIVE_POWER, 0) // | ||
.input(METER_ACTIVE_POWER, 60_000) // | ||
.input(ESS_SOC, 49)// | ||
.output(ESS_SET_ACTIVE_POWER_EQUALS, 10_000)) |
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 any good and easy way to test this controller together with another controller?
Our groups best guess was the snippet below, which had one flaw where we did not find an easy fix. The flaw was that the DummyEss battery does not apply any constraints. So we can test that the controller outputs the right values to the channels, but not that follow up controllers actually perform multi use. We've tested it manually by setting up a simulation environment instead.
var ess = new DummyManagedSymmetricEss(ESS_ID).setPower(new DummyPower(1.0, 0.0, 0.0));
int rechargePower = 50_000;
new ControllerTest(new ControllerEssPeakShavingImpl()) //
.addReference("componentManager", new DummyComponentManager()) //
.addComponent(ess) //
.addComponent(new DummyElectricityMeter(METER_ID)) //
.activate(MyConfig.create() //
.setId(CTRL_ID) //
.setEssId(ESS_ID) //
.setMeterId(METER_ID) //
.setPeakShavingPower(100_000) //
.setRechargePower(rechargePower) //
.setMinimumSoc(50) //
.build())
.next(new TestCase("Ess in multi use mode") //
.input(ESS_ACTIVE_POWER, 0) //
.input(ESS_SOC, 52)
.input(METER_ACTIVE_POWER, 1_000) //
.onAfterControllersCallbacks(() -> {
// simulate follow up controller call
ess.setActivePower(5);
assertEquals(0, (int) ess.getActivePower().get());
})) //
...aving/src/io/openems/edge/controller/symmetric/peakshaving/ControllerEssPeakShavingImpl.java
Outdated
Show resolved
Hide resolved
@parapluplu Thank you for a first review 💪 |
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.
In my opinion, it is already a good improvement.
After fixing/adding the small changes according to the comments, we could merge it and additional improvements could be also added as separate feature requests.
Possible next steps:
- Add config properties in Peakshaving-App
- Add separate Status Channel to show the current behaviour
-- Default states or information that could be relevant: Somewhere "IN_PEAKSHAVING", "ABOVE_PEAKSHAVING_RESERVE" for dynamic/flexible use - An initial implementation of the required methods for the energy schedule (Fahrplanmanagement) -> Add fix Constraints that has to be considered
ui/package-lock.json
Outdated
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 should be not part of the PR
@Laksh26 Could you please provide me access to the branch? Then I would continue to add the desired changes to this pull request |
@parapluplu i am not much familiar with GitHub, but i think i was able to add you as a collaborator to the fork. Does that work? |
|
@Laksh26 nice, I have push access, that should be sufficient. Thank you! Next week I'll likely find some time to work on this. @sebastianasen That's only helpful to checkout this project locally. It's not enough to contribute to this pull request, as I need push access to the branch of the fork project where the pull request comes from :) |
I've reworked the controller a bit so it states more explicitly to support a parallel multi use scenario. I'm still not happy with the configuration (minSocLimit and socHysteresis are always present, even allowParallelMultiuse flag is disabled. This is a bit confusing - but right now I don't have a better idea either, except removing the allowParallelMultiuse flag and just control everything via the minSocLimit flag - which we disregarded already in the discussion, so I'll keep the config as is for now :) ) I'm currently also working on writing an automated integration test for this change to test it with a running system (currently with simulated grid, production and consumption meter and simulated battery - it should also support real hardware setups, which is a bit more complicated). I'm planning to publish this integration test environment soon once it's a bit mature and covers some (really basic) tests - but that's another chapter and I'll open a post in the community forum about that. But before merging this change, I would also like todo an integration test to ensure it's quite stable. Unit tests are running into limits here for me for this use case. Since this is an explicit multi use controller, it means it must allow other follow up controllers to run. Unit tests do not fully emulate channel overwrite behavior, power constraint setting behavior etc. The latter one is quite easily solvable with a |
refactor to explicitly state the controller supports parallel multi use extend test documentation publish state channel to publish the current multi use state
8c21693
to
35a730e
Compare
Group: Laksh + Sebastian
The concept of fixed and soft limitations were created in this controller