Introducing a Burnwire Manager #258
Conversation
…r BurnwireManager
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a burnwire management module including a protocol, a manager implementation, and comprehensive unit tests to validate various activation scenarios and error handling.
- Added a new BurnwireProto interface in pysquared/protos/burnwire.py
- Implemented BurnwireManager with configurable enable logic and error handling in pysquared/hardware/burnwire/manager/burnwire.py
- Provided unit tests covering normal operation, retries, exception handling, and cleanup in tests/unit/hardware/test_burnwire.py
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tests/unit/hardware/test_burnwire.py | New unit tests covering burn activation logic, retry behavior, and cleanup after errors |
| pysquared/protos/burnwire.py | New protocol defining the burnwire interface with docstring specifications |
| pysquared/hardware/burnwire/manager/burnwire.py | Implementation of BurnwireManager with burn logic, error handling, and usage documentation |
Comments suppressed due to low confidence (2)
pysquared/hardware/burnwire/manager/burnwire.py:14
- Typo in usage example: 'BurnwireManger' should be corrected to 'BurnwireManager'.
from lib.pysquared.hardware.burnwire.manager.burnwire import BurnwireManger
pysquared/hardware/burnwire/manager/burnwire.py:140
- [nitpick] Consider updating the log message 'Burnwire Safed' to a clearer phrase such as 'Burnwire set to safe state' for improved clarity.
self._log.info("Burnwire Safed")
You can tell I didn't use AI for this because of all the spelling mistakes haha! Co-authored-by: Copilot <[email protected]>
Signed-off-by: Michael Pham <[email protected]>
There was a problem hiding this comment.
Hey Michael,
Looking over this now that I have some more time on my hands.
A nitpick here, but not sure if you meant to cut out a parameter (looks like to me it is more for smart_burn rather than dumb_burn) or if the documentation is just incorrect, but on burnwire/manager/burnwire.py Line 37 you do include the following:
:param deployment_sensor: Generic input for a sensor object that will act as the deployment sensor for smart_burn(). Defaults to 'False'.
Otherwise, looks good to me! Excited to give the unit tests a spin.
|
Good catch on that unneeded line in the docstring @wrong-formatt! I went ahead and removed it. Let's try and test this code out on the new Flight Controller boards tomorrow and we'll go ahead and merge it in if all looks good! |
|
Summary
Hey team! While I was on the plane to my vacation this week I took some time to scaffold a burn wire manger to address #249!
I didn't bring a board with me, so I haven't been able to test it on hardware yet. I did have the AI overlords code up some unit tests though, which actually did catch some potential logical issues with the code before making this PR.
Intended Usage
I think the docstrings should explain all, but generally what you do with this is for every burn wire port you might want to use you would first initialize the pins you want to use separately as digitalio (catch here is you have to absolutely make sure to default initialize them to the
offstate haha) then pass those two pins in when you initialize the individual manager. You also pass in the logger and an optional param for setting whether the enable logic isTrueorFalse.There is also a burn wire proto introduced with this PR. I was originally going to include the
smart_burn(brunwire with a sensor for feedback on when to stop!) but that was going to convolute the proto because sometimes you won't have a sensor to pass into the manager. So this PR will be for thedumb_burnwhich just takes amax_timeoutas a parameter and a future PR will introduce thesmart_burnas its own manager class.How was this tested