-
Notifications
You must be signed in to change notification settings - Fork 19
New State of Health #235
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
base: main
Are you sure you want to change the base?
New State of Health #235
Conversation
… health pre-repo organization to work with new repo structure)
… for boot count and flags
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.
Pull Request Overview
This PR introduces a new State of Health class for aggregating sensor data, including voltage, current, and temperature measurements from various monitoring components. Key changes include the addition of a new StateOfHealth class, comprehensive unit tests covering its behavior, and the removal of legacy state-of-health formatting functions from pysquared/functions.py.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/unit/test_state_of_health.py | New unit tests validating sensor readings and state update functionality. |
pysquared/state_of_health.py | New StateOfHealth class implementing sensor value aggregation and state update. |
pysquared/functions.py | Removal of obsolete state-of-health methods in favor of the new implementation. |
Co-authored-by: Copilot <[email protected]>
|
… into new-state-of-health
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 is a pretty great refactor from the old state of health. Thank you for extracting this functionality from functions.py
. This feels like the start of a new "services" layer in pysquared
.
While you were still building out the ina219 power monitor manager I remember us talking about how useful the protos could be in creating a generic state of health service. While not as customized as this implementation, in #240 I took a swing at spinning up a generic state of health service just to see what it could look like. How do you think of the generic model compares to what you have here?
pysquared/functions.py
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.
I like that you're removing things from functions.py
. Thank you!
pysquared/state_of_health.py
Outdated
""" | ||
Update the state of health | ||
""" | ||
try: |
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.
Looking over the calls in this try/except I think that none of these calls have the possibility of raising an exception. What are you seeing?
pysquared/state_of_health.py
Outdated
self.battery_power_monitor.get_bus_voltage() | ||
+ self.battery_power_monitor.get_shunt_voltage() |
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.
Where does this idea for adding these two voltages together come from?
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 was based on the legacy implementation that we took from Max Holliday's PyCubed!
Good flag on this Nate, because looking at it again I realized that the implementation of system_voltage()
and battery_voltage()
is incorrect. The legacy PyCubed implementation uses an ADM1176
power monitor chip whereas our PySquared boards use the INA219
power monitor chip.
This code should actually be changed so the bus_voltage() + shunt_voltage()
call is in the battery_voltage()
instead. This is explained a bit in Adafruit's guide for the INA219:
# INA219 measure bus voltage on the load side. So PSU voltage = bus_voltage + shunt_voltage
print("Voltage (VIN+) : {:6.3f} V".format(bus_voltage + shunt_voltage))
system_voltage()
should just be based on bus_voltage
, which is the actual amount of voltage available to the system vs what the voltage of the batteries is.
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.
Noticed based on Nate's comments that there should be a slight change to the implementation of the system_voltage()
and battery_voltage()
calls!
pysquared/state_of_health.py
Outdated
self.battery_power_monitor.get_bus_voltage() | ||
+ self.battery_power_monitor.get_shunt_voltage() |
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 was based on the legacy implementation that we took from Max Holliday's PyCubed!
Good flag on this Nate, because looking at it again I realized that the implementation of system_voltage()
and battery_voltage()
is incorrect. The legacy PyCubed implementation uses an ADM1176
power monitor chip whereas our PySquared boards use the INA219
power monitor chip.
This code should actually be changed so the bus_voltage() + shunt_voltage()
call is in the battery_voltage()
instead. This is explained a bit in Adafruit's guide for the INA219:
# INA219 measure bus voltage on the load side. So PSU voltage = bus_voltage + shunt_voltage
print("Voltage (VIN+) : {:6.3f} V".format(bus_voltage + shunt_voltage))
system_voltage()
should just be based on bus_voltage
, which is the actual amount of voltage available to the system vs what the voltage of the batteries is.
Co-authored-by: JamesDCowley <[email protected]>
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 is on a good track. Talking with Michael, since there is nothing we can do on the satellite to fix an overheating condition we should slim this down so that we only care about battery charge.
@nateinaction I think it could go either way with keeping the temperature monitoring in here for flavor or splitting it off into a dedicated thermal manager / interface. It is true that right now there is not actually much we can do in response to our of range temperatures and it is mostly just a nice FYI for the ground operators. If you'd like I can see if someone on my team has some time to address the |
Summary
Closes #159 (finally). Created a new State of Health class to hold information from a bunch of sensors such as temperatures, voltage, current, etc. Will be used for upcoming Mode Manager to manage power states.
How was this tested
(Power monitor stuff is null but does work with a working battery board, but ours blew up)
Works on v4 flight boards with v2 battery boards. Make sure to import and add the
INA219Manager
andStateOfHealth
intomain.py
on whatever version repo.main.py
For v4 flight + v2 battery (will be added in upcoming PR to v4 and v5 boards):