Skip to content

Conversation

@qlambert-pro
Copy link
Contributor

The magic number 1200, should be carying a unit in order for unit checking to pass. I wasn't inspired, when it came to naming the constant, so feel free to suggest a better name.

@HansOlsson
Copy link
Contributor

Looking at the code it's as if it is the short-circuit current of the battery, which explains the dangerously high number.

@beutlich beutlich added example Issue only addresses example(s) L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries labels Apr 17, 2023
@AHaumer AHaumer marked this pull request as draft April 27, 2023 16:56
@AHaumer AHaumer requested review from AHaumer and removed request for AHaumer April 27, 2023 16:57
@AHaumer
Copy link
Contributor

AHaumer commented Apr 27, 2023

See #4412 #4112:
parameter Modelica.Units.SI.Current Isc = 1200 "Short-circuit current of cell at OCVmax";
Instead of having a lot of single PR's, why don't look through these examples systematically and provide the same for CCCV_Cell, CCCV_CellRC, CCCV_Stack, CCCV_StackRC.

@HansOlsson
Copy link
Contributor

See #4412: parameter Modelica.Units.SI.Current Isc = 1200 "Short-circuit current of cell at OCVmax"; Instead of having a lot of single PR's, why don't look through these examples systematically and provide the same for CCCV_Cell, CCCV_CellRC, CCCV_Stack, CCCV_StackRC.

You mean #4112 - and I agree that having one common PR for them makes more sense.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Coordinate/merge with #4112

@HansOlsson
Copy link
Contributor

Coordinate/merge with #4112

Specifically:

  • All of the similar classes in one PR
  • Same name (Isc) and description "Short circuit current"
  • Same variability (parameter)

christiankral added a commit to henrikt-ma/ModelicaStandardLibrary that referenced this pull request Oct 18, 2023
@christiankral
Copy link
Contributor

This issue is fixed in 8450a8e by #4112

@christiankral
Copy link
Contributor

This issue is fixed in 8450a8e by #4112

Therefore, this ticket can be closed.

@beutlich beutlich removed the request for review from casella November 14, 2023 17:28
@beutlich beutlich added this to the never milestone Nov 14, 2023
@beutlich beutlich added the duplicate Duplicate issue label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate Duplicate issue example Issue only addresses example(s) L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unit error in Modelica.Electrical.Batteries.Examples.BatteryDischargeCharge

6 participants