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

Resolves #2957 - Batteries #3054

Merged
merged 12 commits into from
Oct 22, 2019
Merged

Resolves #2957 - Batteries #3054

merged 12 commits into from
Oct 22, 2019

Conversation

AHaumer
Copy link
Contributor

@AHaumer AHaumer commented Jul 17, 2019

This resolves #2957 - Batteries.
As I said before, I got some results from master thesis, ending up in improved cell and stack models.
Since the whole thing has grown, I vote for moving Batteries out from Analog to an own sublibrary Electrical.Batteries.
This answers questions from users regarding Modelica_EnergyStorages, which shall be replaced by this library in the long run - simpler and easier to use. We hope to get suggestions for improvements from users.

@AHaumer AHaumer added enhancement New feature or enhancement L: Electrical.Analog Issue addresses Modelica.Electrical.Analog P: high High priority issue labels Jul 17, 2019
@AHaumer AHaumer added this to the MSL4.0.0 milestone Jul 17, 2019
@beutlich
Copy link
Member

beutlich commented Jul 17, 2019

So we need library officers for Electrical.Batteries. According to our project rules we have:

When a new library or sublibrary is provided, a copyright transfer contract has to be signed between the contributor(s) and the MA or the contributor(s) assigns an appropriate license to the MA. In this contract the initial Library Officers might be defined. Once available, the MA Contributor License Agreement (CLA) has to be signed by all contributors.

Since you have signed the MA CLA I believe it is enough if you just mention the two or more new library officers.

I also will create a new label for Electrical.Batteries.

@beutlich beutlich added L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries and removed L: Electrical.Analog Issue addresses Modelica.Electrical.Analog labels Jul 17, 2019
@AHaumer
Copy link
Contributor Author

AHaumer commented Jul 17, 2019

Library Officers: @AHaumer and @christiankral , both have alreay signed the CLA. Thanks!

beutlich added a commit to AHaumer/ModelicaStandardLibrary that referenced this pull request Jul 17, 2019
beutlich added a commit to AHaumer/ModelicaStandardLibrary that referenced this pull request Jul 17, 2019
beutlich added a commit to AHaumer/ModelicaStandardLibrary that referenced this pull request Jul 17, 2019
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, both Modelica.Electrical.Batteries.Examples.BatteryDischargeCharge and Modelica.Electrical.Batteries.Examples.CCCVcharging abort simulation in SimulationX 4.0.2 by

cellData.rcData[1].C=Modelica.Electrical.Batteries.ParameterRecords.RCData.Tau*Modelica.Electrical.Batteries.ParameterRecords.RCData.R^-1;
Invalid call of operator "/" (0/0).
Help: Eval.InvalidBinOp

In more detail:

(cellData.Ri|0.0035000000000000001):=(cellData.OCVmax|4.2000000000000002)/(cellData.Isc|1200)
(cellData.rcData[1].R|0.0007000000000000001):=0.20000000000000001*(cellData.Ri|0.0035000000000000001)
(cellData.rcData[2].R|0.00035000000000000005):=0.10000000000000001*(cellData.Ri|0.0035000000000000001)
(cellData.rcData[1].C|0)=(Modelica.Electrical.Batteries.ParameterRecords.RCData.Tau|0)/(Modelica.Electrical.Batteries.ParameterRecords.RCData.R|0)
EXCEPTION:
Invalid call of operator "/" (0/0).
ID: Eval.InvalidBinOp

In the initial system, it calculates the correct value cellData.rcData[1].R, but does not use this value when calculating cellData.rcData[1].C. Thus there seems to be some lookup issue. @gkurzbach Can you elaborate please.

By the way, referencing the component name in the modification is ugly and error-prone

      parameter ParameterRecords.ExampleData cellData2(
        Qnom=18000,
        useLinearSOCDependency=false,
        Idis=0.1,
        Isc=1200,
        rcData={Batteries.ParameterRecords.RCData(R=0.2*cellData2.Ri, Tau=60),
            Batteries.ParameterRecords.RCData(R=0.1*cellData2.Ri, Tau=10)})
        annotation (Placement(transformation(extent={{60,-80},{80,-60}})));

and should not be promoted. Unfortunately the modelica/ModelicaSpecification#213 proposal using the self-keyword (which resolved the issue) never made it in the Modelica specification.

@AHaumer
Copy link
Contributor Author

AHaumer commented Jul 17, 2019

@beutlich regarding 0/0:
To avoid 0/0, I set explicitely R=0 Tau=0 C=0 in the CellStack which doesn't use the RC-elements.
I could think of a solution of setting R to 1 since this value isn't used in CellStack.

Regarding rcData={Batteries.ParameterRecords.RCData(R=0.2cellData2.Ri, Tau=60),
Batteries.ParameterRecords.RCData(R=0.1
cellData2.Ri, Tau=10)}) -
This is a record constructor (like a function call) - looks clumsy but the only solution I know of.
I'm willing to use any nicer construct as soon as available.
Any hints, maybe @HansOlsson ?

@AHaumer
Copy link
Contributor Author

AHaumer commented Jul 17, 2019

@beutlich would you please try again, I hope the problem 0/0 is cured.
For the RecordConstructor, I don't have an idea (since it's an array of records where the length is defined by the user).

@beutlich
Copy link
Member

beutlich commented Jul 17, 2019

@beutlich would you please try again, I hope the problem 0/0 is cured.

Yes, it is resolved, but disguising the actual tool issue, since cellData.rcData[1].C should not be zero in the initial step, right?

(cellData.Ri|0.0035000000000000001):=(cellData.OCVmax|4.2000000000000002)/(cellData.Isc|1200)
(cellData.rcData[1].R|0.0007000000000000001):=0.20000000000000001*(cellData.Ri|0.0035000000000000001)
(cellData.rcData[2].R|0.00035000000000000005):=0.10000000000000001*(cellData.Ri|0.0035000000000000001)
(cellData.rcData[1].C|0):=(if ((Modelica.Electrical.Batteries.ParameterRecords.RCData.R|0)<=0)|true then 0 else(Modelica.Electrical.Batteries.ParameterRecords.RCData.T|0)/(Modelica.Electrical.Batteries.ParameterRecords.RCData.R|0))

For the RecordConstructor, I don't have an idea (since it's an array of records where the length is defined by the user).

Not the array of records is the issue, but using the component name in the modifier again. See the bold names in the lines above.

@christiankral
Copy link
Contributor

I tested Modelica.Electrical.Batteries.Examples.BatteryDischargeCharge with the actual Nightly Build version of OpenModelica and ran into a problem with the new frontend (the old frontend translates and simulates fine). The following error message is issued in case of the new frontend:

Constant battery2.cellData.useLinearSOCDependency is used without having been given a value.

I guess this is a tool issue of the new frontend, as the parameter cellData2 is instantiated as class ParameterRecords.ExampleData which clearly defines the parameter useLinearSOCDependency as:

  parameter ParameterRecords.ExampleData cellData2(
    Qnom=18000,
    useLinearSOCDependency=false,
    Idis=0.1,
    Isc=1200,
    rcData={Modelica.Electrical.Batteries.ParameterRecords.RCData(R=0.2*
        cellData2.Ri, T=60),
        Modelica.Electrical.Batteries.ParameterRecords.RCData(R=0.1*
        cellData2.Ri, T=10)})

@christiankral
Copy link
Contributor

As this a is a new package you could actually introduce the Batteries package as single files as discussed in #2975.

@christiankral
Copy link
Contributor

I created ticket OM#5597 on the OpenModelica issue tracker regarding the two examples which do not simulate.

@AHaumer
Copy link
Contributor Author

AHaumer commented Jul 18, 2019

In Dymola and OM all three examples simulate without any complaint.
Except that OM doesn't recognize "Ah" - IMHO this is a tool issue.

@AHaumer
Copy link
Contributor Author

AHaumer commented Jul 18, 2019

@beutlich I'm afraid I don't understand the issue with C=0:
In CellStacks the all parameters regarding RC are ignored.
In CellRCStacks, the RC parameters have to be non-zero (defined by the user).

@beutlich
Copy link
Member

@beutlich I'm afraid I don't understand the issue with C=0:
In CellStacks the all parameters regarding RC are ignored.
In CellRCStacks, the RC parameters have to be non-zero (defined by the user).

Don't worry. I need some feedback from the SimulationX frontend dev @gkurzbach here if the tool issue can be confirmed.

@beutlich beutlich changed the title resolves #2957 - Batteries Resolves #2957 - Batteries Jul 23, 2019
@beutlich
Copy link
Member

@henrikt-ma The three Batteries examples have various validaton/simulation issues in Wolfram SystemModeler. Can you have a look at it?

@gkurzbach
Copy link

@beutlich I'm afraid I don't understand the issue with C=0:
In CellStacks the all parameters regarding RC are ignored.
In CellRCStacks, the RC parameters have to be non-zero (defined by the user).

Don't worry. I need some feedback from the SimulationX frontend dev @gkurzbach here if the tool issue can be confirmed.

I confirm that the "Invalid call of operator "/" (0/0)" is a tool issue. The original code is fine and the additional modifications are not necessary. This will be fixed in next release.

@beutlich
Copy link
Member

@beutlich I'm afraid I don't understand the issue with C=0:
In CellStacks the all parameters regarding RC are ignored.
In CellRCStacks, the RC parameters have to be non-zero (defined by the user).

Don't worry. I need some feedback from the SimulationX frontend dev @gkurzbach here if the tool issue can be confirmed.

I confirm that the "Invalid call of operator "/" (0/0)" is a tool issue. The original code is fine and the additional modifications are not necessary. This will be fixed in next release.

Thanks for confirmation, @gkurzbach .

@AHaumer Your commit 21d2b55 can be reverted as it only disguises the tool issue (0/0) but still yields wrong simulation results.

Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

21d2b55 should be reverted.

@beutlich
Copy link
Member

@christiankral @christophclauss Never mind my previous comment. Can you please review the proposed changes!

Copy link
Contributor

@christiankral christiankral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK now.

@beutlich
Copy link
Member

beutlich commented Oct 9, 2019

Resolved the merge commit via rebasing and force-pushing.

@beutlich beutlich removed the request for review from christophclauss October 22, 2019 14:56
@beutlich beutlich merged commit c21d073 into modelica:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries P: high High priority issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement idealized battery
5 participants