Repository navigation
Conversation
|
is this ready for review? If not, mark as draft? |
StarrryNight
left a comment
StarrryNight
left a comment
There was a problem hiding this comment.
Left some comments. LGTM but can you make this a part of a stacked PR so we can get coherent code merges? There will probably be some changes to the abstract interface once we start implementing the concrete classes.
Once we resolve the comments we can start implementing the concrete classes.
Just to clarify. The plan rn for autokick-chip is to deprecate the breakbeam condition in chicker and make it a state machine trigger event?
| // Chicker break beam | ||
| /** | ||
| * @return true if the break beam is tripped, false otherwise | ||
| */ | ||
| virtual bool getBreakBeamTripped() = 0; |
There was a problem hiding this comment.
Do we still need this if we are making breakbeam tripped an update event.
| virtual float getBatteryVoltage() = 0; | ||
| /** | ||
| * @return the current draw in amps | ||
| */ |
There was a problem hiding this comment.
There should probably be a micros() function to get time, implemented differently in EspHal and SimHal
| * @param pulse_width kicker pulse width in microseconds | ||
| */ | ||
| virtual void kick(uint32_t pulse_width) = 0; | ||
| /** | ||
| * Fires the chipper. | ||
| * | ||
| * @param pulse_width chipper pulse width in microseconds | ||
| */ |
There was a problem hiding this comment.
We will probably clamp the upper limit of pulse_width somewhere, or is it clamped upstream? If we want to clamp it in the firmware add docs.
| virtual void setDribblerSpeed(uint32_t speed_rpm) = 0; | ||
| /** | ||
| * Advances the dribbler ramp and applies the resulting PWM output. | ||
| */ |
There was a problem hiding this comment.
Could this be combined such that update_dribbler passes in a speed value and also run the ramp towards it?
| * @return the battery voltage in volts | ||
| */ | ||
| virtual float getBatteryVoltage() = 0; | ||
| /** |
There was a problem hiding this comment.
Missing unit since the getCurrentDrawAmp() below comes with units
| virtual void setCapacitorPin(bool pin_state) = 0; | ||
| /** | ||
| * Updates the capacitor charger and recharges if necessary. | ||
| */ |
There was a problem hiding this comment.
Whats the difference? Could we just keep one of them?
Description
Created abstract class PowerHAL. Serves as an interface for tests and the ESP to talk through. See issue 3981 for full architecture and scope of future changes.
Testing Done
N/A
Resolved Issues
Resolves: #3980
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue