-
Notifications
You must be signed in to change notification settings - Fork 3
Power monitor #76
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
Power monitor #76
Conversation
…power monitor to topology
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.
Hey @asiemsen @nateinaction thanks for the quick work on this! Looks great to me. We might want to revisit the naming of components, but it's semantics and I did just realize naming it this way matches our syntax on the CircuitPython side. So it would make sense to stay consistent so it is easier for people to transition.
I think we should merge it and keep moving forward!
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.
A bit of a big nit pick, but we should probably just name this component INA219 or INA219PowerMonitor or something to maintain the Application / Manager / Driver design pattern separation between things.
A higher level PowerMonitorManager component might group together all of the individual power monitor components into a single interface with a little bit of logic that can be used to manipulate them.
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.
Yeah the lack of consistency around component naming is maddening. This specific plan where we create specific device "managers" in the Drv dir was discussed with @LeStarch but as long as we are consistent it doesn't matter. In this case this device manager is consistent with the Lis2mdl and Lsm6dso managers. I vote to keep the names as-is so we don't spend time futzing with the small potatoes stuff before launch. Some time post launch let's find agreement on a component naming scheme.
Also, as you said the power monitor component will likely change before launch similar to the imu component shifting to a detumble component so there will likely be a chance to change that higher level component this month.
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.
Personal opinion, I kind of like the idea of having a single PowerMonitorManager that can have a configurable number of power monitors assigned to it. Totally understand though that we are in crunch time and this single component single driver pattern is easier to implement fast.
I think we might be able to figure out a bit of a design pattern for these "orchestrator components" in the loadswitch PR. If we do figure something good out over there we can come back to implement it on this side down the line!
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 have refactored so we only have a single PowerMonitor component
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.
Presuming since this is an all new file it was copied in from somewhere? It doesn't seem to cause an issues, but we might want to note in the PR description that this was done in case it does accidentally blow something up down the line.
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.
AC Constants overrides some default F Prime Configurations so we can have more active ports. We also added it in the startup manager component https://github.com/Open-Source-Space-Foundation/proves-core-reference/pull/74/files#diff-5919945c60298d8e5d6218be6e1b60034dea557c091ff4ca0bec93a766a0788c
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.
It is copied from upstream FPrime. I played around with reducing the number of constants pulled over but this seems to be the method to override FPrime defaults. Will note it in desc.
|
Also, the integration tests are failing, I think because we have a bad New integration tests should probably be added for the INA component as well, up to you guys if you want to wait on merging this until those are in or make a follow up PR for them. |
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.
Looks great! I would just really want a sdd updated for clarity!
Dialing things in to match the power monitor performance with our system
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.
LGTM!
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.
WTG!
|
Thanks for all the feedback and help on this! |
Add Power Monitor Component
Description
This PR adds power monitor with telemetry of voltage, current, and power of both the system battery and the solar panels. Configuration for ActiveRateGroupOutputPorts was overridden and raised from 10 to 15.
How Has This Been Tested?
Integration test:
@Mikefly123 Tested telemetry on CubeSat with both battery and solar panels connected.
Checklist