Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a PMOS-based ramp limiter generator with better characteristics than NTC-based solutions, including faster reset time and lower power dissipation. The ramp limiter provides inrush current limiting by controlling voltage rise rate.
Key changes:
- Implements RampLimiter circuit with PMOS FET, capacitive divider, and control logic
- Moves generic components (CustomDiode, CustomFet, GenericCapacitor, GenericResistor) to abstract_parts for broader access
- Enhances FET models with gate drive voltage specifications and threshold voltage parameters
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| edg/parts/init.py | Removes imports for components moved to abstract_parts |
| edg/abstract_parts/init.py | Adds imports for moved components and new DummyDigitalSource |
| edg/abstract_parts/PowerCircuits.py | Implements new RampLimiter class with KiCad schematic import |
| edg/abstract_parts/AbstractFets.py | Adds gate threshold voltage parameter and updates FET selection logic |
| edg/abstract_parts/ResistiveDivider.py | Adds KiCad symbol pinning support for schematic import |
| edg/abstract_parts/DummyDevices.py | Adds DummyDigitalSource for testing |
| edg/abstract_parts/CustomFet.py | Updates inheritance and imports for abstract_parts move |
| edg/abstract_parts/CustomDiode.py | Updates imports for abstract_parts move |
| edg/abstract_parts/GenericResistor.py | Updates imports for abstract_parts move |
| edg/abstract_parts/GenericCapacitor.py | Updates imports for abstract_parts move |
| edg/abstract_parts/test_power_circuits.py | Adds comprehensive test for RampLimiter functionality |
| edg/abstract_parts/test_resistor_generic.py | Updates import statement for moved components |
| edg/abstract_parts/test_capacitor_generic.py | Updates import statement for moved components |
| edg/abstract_parts/resources/RampLimiter.kicad_sch | KiCad schematic file for RampLimiter circuit |
| edg/abstract_parts/resources/RampLimiter.asc | LTSpice simulation file for circuit analysis |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| @init_in_parent | ||
| def __init__(self, frequency: RangeLike, drive_current: RangeLike, **kwargs) -> None: | ||
| def __init__(self, *, frequency: RangeLike = 0*Hertz(tol=0), drive_current: RangeLike = Range.all(), **kwargs) -> None: |
There was a problem hiding this comment.
The change from positional to keyword-only arguments for frequency and drive_current may break existing code that calls this constructor with positional arguments. This is a breaking API change that should be documented or handled with backward compatibility.
| def __init__(self, *, frequency: RangeLike = 0*Hertz(tol=0), drive_current: RangeLike = Range.all(), **kwargs) -> None: | |
| def __init__(self, frequency: RangeLike = 0*Hertz(tol=0), drive_current: RangeLike = Range.all(), **kwargs) -> None: |
| capacitance=( | ||
| (1/(self.drv.actual_gate_drive.lower()*self._cdiv_vgs_factor)).shrink_multiply(self.pwr_in.link().voltage) - 1 | ||
| ).shrink_multiply( | ||
| self.cap_gd.actual_capacitance | ||
| ), | ||
| voltage=(0 * Volt(tol=0)).hull(self.pwr_in.link().voltage) | ||
| )) |
There was a problem hiding this comment.
This complex calculation for capacitance should be extracted into a helper method with clear documentation explaining the formula derivation. The current inline calculation is difficult to understand and maintain.
| capacitance=( | |
| (1/(self.drv.actual_gate_drive.lower()*self._cdiv_vgs_factor)).shrink_multiply(self.pwr_in.link().voltage) - 1 | |
| ).shrink_multiply( | |
| self.cap_gd.actual_capacitance | |
| ), | |
| voltage=(0 * Volt(tol=0)).hull(self.pwr_in.link().voltage) | |
| )) | |
| capacitance=self._calculate_gs_capacitance( | |
| self.drv.actual_gate_drive.lower(), | |
| self._cdiv_vgs_factor, | |
| self.pwr_in.link().voltage, | |
| self.cap_gd.actual_capacitance | |
| ), | |
| voltage=(0 * Volt(tol=0)).hull(self.pwr_in.link().voltage) | |
| )) | |
| def _calculate_gs_capacitance(self, actual_gate_drive_lower, cdiv_vgs_factor, pwr_in_voltage, cap_gd_actual_capacitance): | |
| """ | |
| Calculates the gate-source capacitance (Cgs) for the capacitive divider. | |
| Formula derivation: | |
| Treats Cgs and Cgd as a capacitive divider with Cgs on the bottom. | |
| The calculation is: | |
| Cgs = [ (1 / (actual_gate_drive_lower * cdiv_vgs_factor)) * pwr_in_voltage - 1 ] * cap_gd_actual_capacitance | |
| where: | |
| - actual_gate_drive_lower: The lower bound of the gate drive voltage. | |
| - cdiv_vgs_factor: A scaling factor for the divider. | |
| - pwr_in_voltage: The input power rail voltage. | |
| - cap_gd_actual_capacitance: The actual gate-drain capacitance. | |
| Returns the calculated Cgs value. | |
| """ | |
| divider = (1 / (actual_gate_drive_lower * cdiv_vgs_factor)) | |
| result = (divider.shrink_multiply(pwr_in_voltage) - 1).shrink_multiply(cap_gd_actual_capacitance) | |
| return result |
| self.div = self.Block(ResistiveDivider(ratio=self.target_vgs.shrink_multiply(1/self.pwr_in.link().voltage), | ||
| impedance=(1 / self.target_ramp).shrink_multiply(self.drv.actual_gate_drive.lower() / (self.cap_gd.actual_capacitance)) | ||
| )) |
There was a problem hiding this comment.
The impedance calculation is complex and spans multiple lines. Consider extracting this calculation into a helper method with documentation explaining the relationship between target_ramp, gate_drive, and capacitance.
This adds a PMOS-based ramp limiter generator with a enable control input. This can be used for inrush current limiting and has better characteristics (faster reset time, lower power dissipation) compared to a NTC.
This circuit is not robust to steps in input voltage post-turn-on, but may be externally commanded off by a digital signal prior to a step.
A future change may make the control line optional, this needs schematic import to be able to skip parts.
Adds an analysis file for RampLimiter.
Other changes: