-
Notifications
You must be signed in to change notification settings - Fork 12
Ramp limiter generator #420
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
Conversation
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 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: |
Copilot
AI
Sep 3, 2025
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.
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) | ||
| )) |
Copilot
AI
Sep 3, 2025
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 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)) | ||
| )) |
Copilot
AI
Sep 3, 2025
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.
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: