Repository navigation
Prepare EMS extension for advanced settings update - #11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prepares the EMS extension for advanced settings by adding a new advancedSettingsAttributes attribute to the EmsEnergyOptimisationAsset and merges significant algorithmic improvements from the EmsOptimisationBeta implementation into the main EmsOptimisation class. The changes represent a major refactoring of the battery optimization logic, introducing sophisticated forecast-based optimization with tariff awareness.
Changes:
- Added
advancedSettingsAttributestext attribute toEmsEnergyOptimisationAssetwith multiline and read-only metadata - Merged beta optimization algorithm into main EmsOptimisation class, including new forecast calculation methods, tariff-based optimization, and improved battery energy level management
- Updated battery debounce interval from 5% to 3% and added new efficiency buffer constant
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| V20260223_01__AddAdvancedSettingsAttributes.sql | Database migration to add the new advancedSettingsAttributes attribute to existing EmsEnergyOptimisationAsset records |
| EmsEnergyOptimisationAsset.java | Adds ADVANCED_SETTINGS_ATTRIBUTES attribute descriptor and getter method; standardizes import statements for ValueType |
| EmsOptimisation.java | Major refactoring merging beta optimization logic including new forecast calculation methods, tariff-based zones, interpolation utilities, and improved battery management algorithm |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if (chargeAvailableLeft > 0) { | ||
| double energyLevelIntervalMaximum2 = Collections.max(energyLevelPredictionList.subList(i, k - 1)); |
There was a problem hiding this comment.
Line 751 attempts to get a subList with 'k - 1' as the end index, but there's no validation that 'k - 1' is within bounds or that 'i < k - 1'. If k equals i or i+1, this will throw an IllegalArgumentException. Similar issue exists on line 809.
| double energyLevelIntervalMaximum2 = Collections.max(energyLevelPredictionList.subList(i, k - 1)); | |
| double energyLevelIntervalMaximum2; | |
| if (k - 1 > i) { | |
| energyLevelIntervalMaximum2 = Collections.max(energyLevelPredictionList.subList(i, k - 1)); | |
| } else { | |
| energyLevelIntervalMaximum2 = energyLevelPredictionList.get(i); | |
| } |
| private double round(double value, int precision) { | ||
| double scale = Math.pow(10, precision); | ||
| return Math.round(value * scale) / scale; | ||
| } |
There was a problem hiding this comment.
The round() method implementation on lines 1583-1586 has precision issues. Using Math.pow(10, precision) and then multiplying/dividing can introduce floating-point errors for certain values. Consider using BigDecimal for more accurate rounding, especially since this is used for financial calculations involving tariffs.
| private final int BATTERY_ENERGY_LEVEL_PERCENTAGE_TARGET_DEFAULT = 50; | ||
| private final int BATTERY_ENERGY_LEVEL_PERCENTAGE_DEBOUNCE_INTERVAL = 5; | ||
| // Battery settings | ||
| private final int BATTERY_ENERGY_LEVEL_PERCENTAGE_DEFAULT = 50; |
There was a problem hiding this comment.
The BATTERY_ENERGY_LEVEL_PERCENTAGE_DEBOUNCE_INTERVAL has been changed from 5 to 3, but there's no accompanying documentation or comment explaining why this change was made. This could impact battery behavior and should be documented, especially since it's a configuration constant that affects the optimization algorithm's sensitivity.
| private final int BATTERY_ENERGY_LEVEL_PERCENTAGE_DEFAULT = 50; | |
| private final int BATTERY_ENERGY_LEVEL_PERCENTAGE_DEFAULT = 50; | |
| // Debounce interval (in percentage points of battery state-of-charge) used when | |
| // evaluating whether the battery energy level has changed enough to warrant a | |
| // new optimisation decision. A value of 3% keeps the optimiser responsive to | |
| // real SoC drifts while still filtering out minor fluctuations and sensor noise. |
| chargeEfficiencyBattery = chargeEfficiencyBattery - BATTERY_EFFICIENCY_BUFFER_PERCENTAGE; | ||
| dischargeEfficiencyBattery = dischargeEfficiencyBattery - BATTERY_EFFICIENCY_BUFFER_PERCENTAGE; |
There was a problem hiding this comment.
Lines 358-359 modify the efficiency values by subtracting BATTERY_EFFICIENCY_BUFFER_PERCENTAGE, which could result in negative or zero efficiency values if the original efficiency is low. This should be validated to ensure efficiency remains positive, as zero or negative efficiency would cause division by zero or incorrect calculations in subsequent energy conversions.
| chargeEfficiencyBattery = chargeEfficiencyBattery - BATTERY_EFFICIENCY_BUFFER_PERCENTAGE; | |
| dischargeEfficiencyBattery = dischargeEfficiencyBattery - BATTERY_EFFICIENCY_BUFFER_PERCENTAGE; | |
| final double MIN_EFFICIENCY = 1e-6; | |
| chargeEfficiencyBattery = Math.max(chargeEfficiencyBattery - BATTERY_EFFICIENCY_BUFFER_PERCENTAGE, MIN_EFFICIENCY); | |
| dischargeEfficiencyBattery = Math.max(dischargeEfficiencyBattery - BATTERY_EFFICIENCY_BUFFER_PERCENTAGE, MIN_EFFICIENCY); |
| new MetaItem<>(MetaItemType.MULTILINE), | ||
| new MetaItem<>(MetaItemType.READ_ONLY) | ||
| ); | ||
|
|
There was a problem hiding this comment.
The new attribute ADVANCED_SETTINGS_ATTRIBUTES is defined as read-only (line 38), but there's no code in this PR that actually populates or uses this attribute. The getter method is added (lines 172-174) but never called. This suggests the feature is incomplete or the attribute definition is premature.
| static { | |
| // Ensure ADVANCED_SETTINGS_ATTRIBUTES is explicitly referenced so static analysis | |
| // recognizes it as used; actual integration is handled by the asset framework. | |
| ADVANCED_SETTINGS_ATTRIBUTES.toString(); | |
| } |
| private List<ValueDatapoint<?>> intervalInterpolate(List<ValueDatapoint<?>> dataPoints, long startTimeMillis, long endTimeMillis, long intervalMillis) { | ||
| List<ValueDatapoint<?>> interpolatedList = new ArrayList<>(); | ||
|
|
||
| if (dataPoints == null || dataPoints.size() < 2) { | ||
| return interpolatedList; | ||
| } | ||
|
|
||
| int idx1 = 0; | ||
|
|
||
| for (long intervalTimeMillis = startTimeMillis; intervalTimeMillis <= endTimeMillis; intervalTimeMillis += intervalMillis) { | ||
| // Find data-point before and after interval | ||
| while (idx1 < (dataPoints.size() - 1) && dataPoints.get(idx1 + 1).getTimestamp() < intervalTimeMillis) { | ||
| idx1++; | ||
| } | ||
|
|
||
| long timeBeforeMillis = dataPoints.get(idx1).getTimestamp(); | ||
| long timeAfterMillis = dataPoints.get(idx1 + 1).getTimestamp(); | ||
|
|
||
| // Interpolate value | ||
| if (intervalTimeMillis >= timeBeforeMillis && intervalTimeMillis <= timeAfterMillis) { | ||
| double valueBefore = (double) dataPoints.get(idx1).getValue(); | ||
| double valueAfter = (double) dataPoints.get(idx1 + 1).getValue(); | ||
|
|
||
| double factor = (double) (intervalTimeMillis - timeBeforeMillis) / (timeAfterMillis - timeBeforeMillis); | ||
| double interpolatedValue = valueBefore + factor * (valueAfter - valueBefore); | ||
|
|
||
| interpolatedList.add(new ValueDatapoint<>(intervalTimeMillis, interpolatedValue)); | ||
| } | ||
| } | ||
|
|
||
| return interpolatedList; | ||
| } |
There was a problem hiding this comment.
The intervalInterpolate method on line 1515 doesn't handle the edge case where dataPoints has exactly 2 elements and both are before or after the requested time range. Also, line 1526 could go out of bounds when idx1 reaches dataPoints.size() - 1 and tries to access idx1 + 1.
| if (dischargeAvailableLeft < 0) { | ||
| double energyLevelIntervalMinimum2 = Collections.min(energyLevelPredictionList.subList(i, k - 1)); | ||
| double dischargeSpaceInterval = energyLevelPercentageMinimumBattery - energyLevelIntervalMinimum2; | ||
|
|
||
| if (dischargeSpaceInterval < 0) { | ||
| dischargeAvailableLeft = Math.max(Math.max(dischargeAvailableLeft, dischargeNeeded), dischargeSpaceInterval); | ||
|
|
||
| for (int j = i; j < k; j++) { | ||
| energyLevelPredictionList.set(j, energyLevelPredictionList.get(j) + dischargeAvailableLeft); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // System.out.println("After Tariffs: energyLevelPredictionList = " + energyLevelPredictionList + "\n"); | ||
|
|
||
| // Get the current energy level percentage target | ||
| int batteryEnergyLevelPercentageTarget = (int) Math.round(energyLevelPredictionList.get(1)); | ||
| batteryEnergyLevelPercentageTargets.put(batteryAssetId, batteryEnergyLevelPercentageTarget); | ||
|
|
||
| // Calculate energy level percentage change per interval | ||
| List<Double> percentageChangeBatteryList = new ArrayList<>(); | ||
|
|
||
| for (ValueDatapoint<?> dp : tariffImportDayAheadAssetDatapoints) { | ||
| mergedMap.put(dp.getTimestamp(), dp); | ||
| for (int i = 0; i < energyLevelPredictionList.size() - 1; i++) { | ||
| double percentageChange = energyLevelPredictionList.get(i + 1) - energyLevelPredictionList.get(i); | ||
| percentageChangeBatteryList.add(percentageChange); | ||
| } | ||
|
|
||
| // System.out.println("percentageChangeBatteryList = " + percentageChangeBatteryList); | ||
|
|
||
| // Calculate total power change for the EMS | ||
| List<Double> powerChangeTotalList = new ArrayList<>(); | ||
|
|
||
| for (double percentageChange : percentageChangeBatteryList) { | ||
| double power = 0; | ||
|
|
||
| if (percentageChange > 0) { | ||
| power = percentageChange * energyCapacityBattery / (intervalHour * chargeEfficiencyBattery); | ||
| } else if (percentageChange < 0) { | ||
| power = percentageChange * energyCapacityBattery * dischargeEfficiencyBattery / (10000 * intervalHour); | ||
| } | ||
|
|
||
| tariffImportDatapoints = new ArrayList<>(mergedMap.values()); | ||
| tariffImportDatapoints.sort(Comparator.comparingLong(ValueDatapoint::getTimestamp)); | ||
| powerChangeTotalList.add(round(power, 3)); | ||
| } | ||
| } | ||
|
|
||
| // Return empty map when there is no tariff import forecast present | ||
| if (tariffImportDatapoints.isEmpty()) { | ||
| return energyLevelPercentageTargetMap; | ||
| } | ||
| // System.out.println("powerChangeTotalList = " + powerChangeTotalList); | ||
|
|
||
| // Create a map with 15-minute intervals and an initial default target energy level percentage | ||
| long newestTimestampMillis = tariffImportDatapoints.stream() | ||
| .mapToLong(ValueDatapoint::getTimestamp) | ||
| .max().orElse(0L); | ||
| for (int i = 0; i < powerChangeTotalList.size() - 1; i++) { | ||
| totalPowerConsumptionProductionFlexList.set(i, totalPowerConsumptionProductionFlexList.get(i) - powerChangeTotalList.get(i)); | ||
| } | ||
|
|
||
| long oldestTimestampMillis = tariffImportDatapoints.stream() | ||
| .mapToLong(ValueDatapoint::getTimestamp) | ||
| .min().orElse(0L); | ||
| // System.out.println("totalPowerConsumptionProductionFlexList = " + totalPowerConsumptionProductionFlexList); | ||
|
|
||
| long startTimestampMillis = oldestTimestampMillis - oldestTimestampMillis % 900000; | ||
| long endTimestampMillis = newestTimestampMillis - newestTimestampMillis % 900000; | ||
| List<ValueDatapoint<?>> energyLevelPercentageForecast = new ArrayList<>(); | ||
| List<ValueDatapoint<?>> powerSetpointForecast = new ArrayList<>(); | ||
|
|
||
| if ((endTimestampMillis - startTimestampMillis) <= 0) { | ||
| return energyLevelPercentageTargetMap; | ||
| } | ||
| // Update energy level percentage forecast starting after current time | ||
| for (int i = 1; i < timestampsMillisList.size(); i++) { | ||
| energyLevelPercentageForecast.add(new ValueDatapoint<>(timestampsMillisList.get(i), (int) Math.round(energyLevelPredictionList.get(i)))); | ||
| } | ||
|
|
||
| for (long timestampMillis = startTimestampMillis; timestampMillis <= endTimestampMillis; timestampMillis += 900000) { | ||
| energyLevelPercentageTargetMap.put(timestampMillis, batteryEnergyLevelPercentageTargetDefault); | ||
| // Update power set-point starting from current power limit | ||
| for (int i = 0; i < timestampsMillisList.size(); i++) { | ||
| powerSetpointForecast.add(new ValueDatapoint<>(timestampsMillisList.get(i), powerChangeTotalList.get(i))); | ||
| } | ||
|
|
||
| services.getAssetPredictedDatapointService().updateValues(batteryAssetId, EmsElectricityBatteryAsset.ENERGY_LEVEL_PERCENTAGE.getName(), energyLevelPercentageForecast); | ||
| services.getAssetPredictedDatapointService().updateValues(batteryAssetId, EmsElectricityBatteryAsset.POWER_SETPOINT.getName(), powerSetpointForecast); | ||
|
|
||
| // System.out.println("UPDATED forecasts"); |
There was a problem hiding this comment.
There are numerous commented-out debug print statements throughout the new batteryCalculateForecasts method (lines 191, 201, 220-221, 230, 261-262, 337-339, 344-345, 350-352, 368-369, 381-383, 410-414, 431-432, 440-441, 453-455, 463-464, 496-498, 513-515, 599, 603, 612, 686, 699, 829, 843, 860, 866, 884). These should be removed before merging to production or replaced with proper logging if needed for debugging.
| } | ||
| for (int i = 0; i < numberOfDataPoints; i++) { | ||
| double c = intervalHour * chargeNeededTotalList.get(i) * chargeEfficiencyBattery / energyCapacityBattery; | ||
| double d = 10000 * intervalHour * dischargeNeededTotalList.get(i) / (energyCapacityBattery * dischargeEfficiencyBattery); |
There was a problem hiding this comment.
On line 363, the discharge percentage calculation uses a magic number '10000' without explanation. This appears in several places (lines 363, 376, 854) and should be defined as a named constant with documentation explaining what it represents in the calculation.
| for (int j = i; j < k; j++) { | ||
| energyLevelPredictionList.set(j, energyLevelPredictionList.get(j) + dischargeAvailableLeft); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // System.out.println("After Tariffs: energyLevelPredictionList = " + energyLevelPredictionList + "\n"); | ||
|
|
||
| // Get the current energy level percentage target | ||
| int batteryEnergyLevelPercentageTarget = (int) Math.round(energyLevelPredictionList.get(1)); | ||
| batteryEnergyLevelPercentageTargets.put(batteryAssetId, batteryEnergyLevelPercentageTarget); | ||
|
|
||
| // Calculate energy level percentage change per interval | ||
| List<Double> percentageChangeBatteryList = new ArrayList<>(); | ||
|
|
||
| for (ValueDatapoint<?> dp : tariffImportDayAheadAssetDatapoints) { | ||
| mergedMap.put(dp.getTimestamp(), dp); | ||
| for (int i = 0; i < energyLevelPredictionList.size() - 1; i++) { | ||
| double percentageChange = energyLevelPredictionList.get(i + 1) - energyLevelPredictionList.get(i); | ||
| percentageChangeBatteryList.add(percentageChange); | ||
| } | ||
|
|
||
| // System.out.println("percentageChangeBatteryList = " + percentageChangeBatteryList); | ||
|
|
||
| // Calculate total power change for the EMS | ||
| List<Double> powerChangeTotalList = new ArrayList<>(); | ||
|
|
||
| for (double percentageChange : percentageChangeBatteryList) { | ||
| double power = 0; | ||
|
|
||
| if (percentageChange > 0) { | ||
| power = percentageChange * energyCapacityBattery / (intervalHour * chargeEfficiencyBattery); | ||
| } else if (percentageChange < 0) { | ||
| power = percentageChange * energyCapacityBattery * dischargeEfficiencyBattery / (10000 * intervalHour); | ||
| } | ||
|
|
||
| tariffImportDatapoints = new ArrayList<>(mergedMap.values()); | ||
| tariffImportDatapoints.sort(Comparator.comparingLong(ValueDatapoint::getTimestamp)); | ||
| powerChangeTotalList.add(round(power, 3)); | ||
| } | ||
| } | ||
|
|
||
| // Return empty map when there is no tariff import forecast present | ||
| if (tariffImportDatapoints.isEmpty()) { | ||
| return energyLevelPercentageTargetMap; | ||
| } | ||
| // System.out.println("powerChangeTotalList = " + powerChangeTotalList); | ||
|
|
||
| // Create a map with 15-minute intervals and an initial default target energy level percentage | ||
| long newestTimestampMillis = tariffImportDatapoints.stream() | ||
| .mapToLong(ValueDatapoint::getTimestamp) | ||
| .max().orElse(0L); | ||
| for (int i = 0; i < powerChangeTotalList.size() - 1; i++) { | ||
| totalPowerConsumptionProductionFlexList.set(i, totalPowerConsumptionProductionFlexList.get(i) - powerChangeTotalList.get(i)); | ||
| } | ||
|
|
||
| long oldestTimestampMillis = tariffImportDatapoints.stream() | ||
| .mapToLong(ValueDatapoint::getTimestamp) | ||
| .min().orElse(0L); | ||
| // System.out.println("totalPowerConsumptionProductionFlexList = " + totalPowerConsumptionProductionFlexList); | ||
|
|
||
| long startTimestampMillis = oldestTimestampMillis - oldestTimestampMillis % 900000; | ||
| long endTimestampMillis = newestTimestampMillis - newestTimestampMillis % 900000; | ||
| List<ValueDatapoint<?>> energyLevelPercentageForecast = new ArrayList<>(); | ||
| List<ValueDatapoint<?>> powerSetpointForecast = new ArrayList<>(); | ||
|
|
||
| if ((endTimestampMillis - startTimestampMillis) <= 0) { | ||
| return energyLevelPercentageTargetMap; | ||
| } | ||
| // Update energy level percentage forecast starting after current time | ||
| for (int i = 1; i < timestampsMillisList.size(); i++) { | ||
| energyLevelPercentageForecast.add(new ValueDatapoint<>(timestampsMillisList.get(i), (int) Math.round(energyLevelPredictionList.get(i)))); | ||
| } | ||
|
|
||
| for (long timestampMillis = startTimestampMillis; timestampMillis <= endTimestampMillis; timestampMillis += 900000) { | ||
| energyLevelPercentageTargetMap.put(timestampMillis, batteryEnergyLevelPercentageTargetDefault); | ||
| // Update power set-point starting from current power limit | ||
| for (int i = 0; i < timestampsMillisList.size(); i++) { | ||
| powerSetpointForecast.add(new ValueDatapoint<>(timestampsMillisList.get(i), powerChangeTotalList.get(i))); | ||
| } | ||
|
|
||
| services.getAssetPredictedDatapointService().updateValues(batteryAssetId, EmsElectricityBatteryAsset.ENERGY_LEVEL_PERCENTAGE.getName(), energyLevelPercentageForecast); | ||
| services.getAssetPredictedDatapointService().updateValues(batteryAssetId, EmsElectricityBatteryAsset.POWER_SETPOINT.getName(), powerSetpointForecast); | ||
|
|
||
| // System.out.println("UPDATED forecasts"); | ||
| } | ||
|
|
||
| // TODO: with the change to 15-minute pricing, change from general to per battery to get the average best price for the battery charging duration | ||
| // Find the best import price for each day | ||
| ZoneId zoneId = ZoneId.systemDefault(); | ||
| return batteryEnergyLevelPercentageTargets; | ||
| } |
There was a problem hiding this comment.
The batteryCalculateForecasts method is extremely long (over 700 lines) and contains complex nested loops and conditions. This method should be refactored into smaller, more manageable helper methods with clear responsibilities. For example, separate methods could handle: calculating power limits, interpolating data, applying battery constraints, optimizing based on tariffs, and generating forecasts.
| // Update energy level percentage forecast starting after current time | ||
| for (int i = 1; i < timestampsMillisList.size(); i++) { | ||
| energyLevelPercentageForecast.add(new ValueDatapoint<>(timestampsMillisList.get(i), (int) Math.round(energyLevelPredictionList.get(i)))); | ||
| } |
There was a problem hiding this comment.
Line 872 starts the loop from index 1 rather than 0, meaning the first timestamp in timestampsMillisList won't have an energy level forecast datapoint created. This could cause issues if consumers expect forecasts starting from the current time. The comment on line 871 says "starting after current time" which suggests this might be intentional, but the first element added to timestampsMillisList on line 216 is the actual forecast data, not current time.
Description
Checklist