Feature/877 add battery capacity fault capability #879

Yume27 · 2024-12-19T20:58:20Z

Tickets addressed: bsk-877
Review: By commit
Merge strategy: Squash and Merge

Description

This added the capability to simulate a fault in the simpleBattery module that reduces the actual storage capacity without directly altering the stated capacity.

Verification

Added unit test to verify that

Check if the battery capacity matches its nominal value when a fault occurs.
Verify that the charged capacity does not exceed the faulted capacity or drop below zero.

Documentation

Updated rst file associated with simpleBattery module.

Future work

None

juan-g-bonilla · 2024-12-20T00:39:47Z

src/simulation/power/simpleBattery/simpleBattery.cpp

@@ -59,6 +59,16 @@ void SimpleBattery::evaluateBatteryModel(PowerStorageStatusMsgPayload *msg) {
        this->storedCharge = 0;
    }

+    if(this->fault == true) {   


Is there some value to making these fault and faultCapacityRatio input messages instead of class variables?

Mm, you are thinking like a battery configuration message? I'm not sure I can think of an associated scenario that would need this. I'll think on this.

I was thinking about a separate model that would produce output messages that reduce the effective capacity of the battery. For example, a slow, continuous BatteryDegradationModel or a model that randomly produces an output message that lowers the capacity to simulate discrete random faults.

Mm, interesting. I could see value in that. @Yume27 , what you can do is move the fault state to a message. Then, in simpleBattery you check if this optional message is connected. If yes, you read in the values and use them? In your event handler you would just change the value of this input message.

Also, couldn't this fault feature be added to the powerStorageBase() base class? This way all batteries based on this parent class would get this capability?

juan-g-bonilla · 2024-12-20T00:41:55Z

src/simulation/power/simpleBattery/simpleBattery.cpp

+        if(this->faultCapacityRatio < 0)
+        {
+            this->faultCapacityRatio = 1.0;
+        }
+        if(this->storedCharge > this->storageCapacity * this->faultCapacityRatio) {
+            this->storedCharge = this->storageCapacity * this->faultCapacityRatio;
+        }


Could this be simplified and made more strict regarding acceptable values of faultCapacityRatio?

if (this->faultCapacityRatio < 0 || this->faultCapacityRatio > 1) bskLogger.bskLog(BSK_ERROR, "faultCapacityRatio should be between 0 and 1!"); else this->storedCharge = this->storageCapacity * this->faultCapacityRatio;

I agree. Right now this code silently changes the value if <0 and the user is not notified. I like the more compact version that doesn't silently change values.

@juan-g-bonilla , your code actually always set the stored charge to the max value. I think the code should be:

if (this->faultCapacityRatio < 0 || this->faultCapacityRatio > 1) { bskLogger.bskLog(BSK_ERROR, "faultCapacityRatio should be between 0 and 1!"); } else if(this->storedCharge > this->storageCapacity * this->faultCapacityRatio) { this->storedCharge = this->storageCapacity * this->faultCapacityRatio; } }

Yes, sorry I misread something about that line when writing the comment!

schaubh · 2024-12-20T00:48:05Z

src/simulation/power/simpleBattery/simpleBattery.h

@@ -39,6 +39,8 @@ class SimpleBattery: public PowerStorageBase {

 public:
    double storageCapacity; //!< [W-s] Battery capacity in Watt-seconds (Joules).
+    bool fault = false; //!< Fault flag


These variables should be private variables, and then we need associated setter and getter functions

If you’re using setters, then i would move the check 0<=faultCapacityRatio<=1 to the setter of faultCapacityRatio instead of every time the model is run.

But, if we are reading in the value from a message, then we don't have that control. I like the idea of moving the fault info to a battery fault message. I think it will provide a more elegant solution.

schaubh · 2024-12-20T00:50:14Z

src/simulation/power/simpleBattery/simpleBattery.h

@@ -39,6 +39,8 @@ class SimpleBattery: public PowerStorageBase {

 public:
    double storageCapacity; //!< [W-s] Battery capacity in Watt-seconds (Joules).
+    bool fault = false; //!< Fault flag
+    double faultCapacityRatio = 1.0; //!< Fault capacity ratio


explain what 1.0 default value means here. This means by default we have full capacity, correct?

schaubh · 2024-12-20T00:50:39Z

src/simulation/power/simpleBattery/simpleBattery.cpp

+        if(this->faultCapacityRatio < 0)
+        {
+            this->faultCapacityRatio = 1.0;
+        }
+        if(this->storedCharge > this->storageCapacity * this->faultCapacityRatio) {
+            this->storedCharge = this->storageCapacity * this->faultCapacityRatio;
+        }


I agree. Right now this code silently changes the value if <0 and the user is not notified. I like the more compact version that doesn't silently change values.

schaubh · 2024-12-20T00:50:57Z

src/simulation/power/simpleBattery/_UnitTest/test_simpleBatteryFault.py

+#
+#  ISC License
+#
+#  Copyright (c) 2016, Autonomous Vehicle Systems Lab, University of Colorado at Boulder


update copyright year to 2024

schaubh · 2024-12-20T00:54:17Z

src/simulation/power/simpleBattery/_UnitTest/test_simpleBatteryFault.py

+    test_battery = simpleBattery.SimpleBattery()
+    test_battery.storedCharge_Init = storedChargeInit
+    test_battery.storageCapacity = batteryCapacity  
+    test_battery.fault = True


Right now you are only testing the functionality if there is a fault. You should include a trivial test that checks that the default behavior is no fault.

schaubh · 2024-12-20T00:55:07Z

src/simulation/power/simpleBattery/simpleBattery.rst

@@ -36,3 +36,8 @@ The next step is to attach one or more :ref:`PowerNodeUsageMsgPayload` instances


 For more information on how to set up and use this module, see the simple power system example :ref:`scenarioPowerDemo`.
+
+To simulate a battery capacity fault that reduces the actual storage capacity (without directly altering the stated capacity), users must set up the fault flag::


This is not very clear what we are doing here. Can you please be more explicit on what is done when a fault is modeled?

schaubh · 2024-12-20T00:56:16Z

src/simulation/power/simpleBattery/simpleBattery.rst

+
+To simulate a battery capacity fault that reduces the actual storage capacity (without directly altering the stated capacity), users must set up the fault flag::
+
+   battery.fault = True


This should be updated to use the private class setter function. Check out
https://avslab.github.io/basilisk/Documentation/moduleTemplates/cppModuleTemplate/cppModuleTemplate.html
to see how to name and document the setter and getter functions.

schaubh · 2024-12-20T00:59:39Z

src/simulation/power/simpleBattery/simpleBattery.cpp

+        if(this->faultCapacityRatio < 0)
+        {
+            this->faultCapacityRatio = 1.0;
+        }
+        if(this->storedCharge > this->storageCapacity * this->faultCapacityRatio) {
+            this->storedCharge = this->storageCapacity * this->faultCapacityRatio;
+        }


@juan-g-bonilla , your code actually always set the stored charge to the max value. I think the code should be:

if (this->faultCapacityRatio < 0 || this->faultCapacityRatio > 1) { bskLogger.bskLog(BSK_ERROR, "faultCapacityRatio should be between 0 and 1!"); } else if(this->storedCharge > this->storageCapacity * this->faultCapacityRatio) { this->storedCharge = this->storageCapacity * this->faultCapacityRatio; } }

schaubh · 2024-12-20T01:02:30Z

@Yume27 , your pre-commit test failed. Be sure to install the pre-commit support by reading https://avslab.github.io/basilisk/Support/Developer/CodingGuidlines.html. Look near the bottom for the link to CONTRIBUTING.md. It talks about how to include the checks, and how to many force check specific files if needed.

juan-g-bonilla · 2024-12-20T01:23:10Z

src/simulation/power/simpleBattery/simpleBattery.cpp

@@ -59,6 +59,16 @@ void SimpleBattery::evaluateBatteryModel(PowerStorageStatusMsgPayload *msg) {
        this->storedCharge = 0;
    }

+    if(this->fault == true) {   


Actually, is there a reason to have both fault AND faultCapacityRatio? Doesn’t faultCapacityRatio==1 imply no fault in the capacity and anything below 1 implies a fault?

I would also do if(this->fault) instead of if(this->fault == true).

I agree. If the fault info is coming from an optional input message, we don't need fault at all. Rather, if a fault message is connected, then the fault is read in and applied. If we temporarily don't want a fault, then the message value of faultCapacityRatio would just be set to 1?

Yumeka Nagano added 3 commits December 19, 2024 12:16

Add storage capacity fault capability to simple battery module

33bfcdb

Add test script for simple battery fault

906607d

Add documentation for simple battery fault

8cf2283

Yume27 added the enhancement New feature or request label Dec 19, 2024

Yume27 requested a review from a team as a code owner December 19, 2024 20:58

juan-g-bonilla reviewed Dec 20, 2024

View reviewed changes

schaubh requested changes Dec 20, 2024

View reviewed changes

juan-g-bonilla reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/877 add battery capacity fault capability #879

Feature/877 add battery capacity fault capability #879

Yume27 commented Dec 19, 2024

juan-g-bonilla Dec 20, 2024

schaubh Dec 20, 2024

juan-g-bonilla Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

juan-g-bonilla Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

juan-g-bonilla Dec 20, 2024

schaubh Dec 20, 2024

juan-g-bonilla Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh Dec 20, 2024

schaubh commented Dec 20, 2024

juan-g-bonilla Dec 20, 2024

juan-g-bonilla Dec 20, 2024

schaubh Dec 20, 2024

		@@ -36,3 +36,8 @@ The next step is to attach one or more :ref:`PowerNodeUsageMsgPayload` instances


		For more information on how to set up and use this module, see the simple power system example :ref:`scenarioPowerDemo`.

		To simulate a battery capacity fault that reduces the actual storage capacity (without directly altering the stated capacity), users must set up the fault flag::


		To simulate a battery capacity fault that reduces the actual storage capacity (without directly altering the stated capacity), users must set up the fault flag::

		battery.fault = True

Feature/877 add battery capacity fault capability #879

Are you sure you want to change the base?

Feature/877 add battery capacity fault capability #879

Conversation

Yume27 commented Dec 19, 2024

Description

Verification

Documentation

Future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schaubh commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment