Prefer lower e-series values for resistors and capacitors #430
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the E-series component selection logic to prioritize lower E-series values and adds functionality to determine which E-series a given value belongs to. The changes also update component selections across multiple example projects as a result of the new prioritization.
- Removed E1 series and merged it into E3, eliminating redundant combined series definitions (E1+E192 variants)
- Added
series_of()method to identify which E-series contains a given component value - Updated component selection logic in
AbstractResistorandAbstractCapacitorto prefer lower E-series values - Adjusted capacitance specifications in OLED display drivers to use more standard values
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| edg/abstract_parts/ESeriesUtil.py | Refactored E-series definitions, added series_of() method, removed E1 and combined series |
| edg/abstract_parts/AbstractResistor.py | Added E-series prioritization to resistor part selection |
| edg/abstract_parts/AbstractCapacitor.py | Added E-series prioritization to capacitor part selection |
| edg/abstract_parts/test_e_series.py | Added tests for series_of(), removed tests for obsolete combined E-series |
| edg/abstract_parts/test_resistive_divider.py | Updated test to use explicit E1 series list instead of SERIES[1] |
| edg/parts/Oled_Er_Oled_096_1_1.py | Changed VCC capacitor upper bound from 20µF to 10µF |
| edg/parts/Oled_Er_Oled_091_3.py | Changed capacitance specs from ranges to tolerance-based values |
| examples/test_usb_source_measure.py | Increased clamp current lower bound from 2.5mA to 5mA |
| examples/*/*.net | Auto-generated component value updates reflecting new E-series prioritization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| # reverse mapping of value to series, reverse SERIES so lower series preferred | ||
| VALUE_SERIES = {v: k for k, series in reversed(SERIES.items()) for v in series} |
There was a problem hiding this comment.
The reversed() function returns an iterator, not a reversed dictionary view. When used with dict.items(), this will fail at runtime with a TypeError because iterators don't have an items() method. You need to use reversed(list(SERIES.items())) instead.
Example fix:
VALUE_SERIES = {v: k for k, series in reversed(list(SERIES.items())) for v in series}| VALUE_SERIES = {v: k for k, series in reversed(SERIES.items()) for v in series} | |
| VALUE_SERIES = {v: k for k, series in reversed(list(SERIES.items())) for v in series} |
| Performs limited rounding to account for floating point issues.""" | ||
| if value == 0: | ||
| return default | ||
| normalized_value = value * math.pow(10, -math.floor(math.log10(value))) |
There was a problem hiding this comment.
The series_of method doesn't handle negative values correctly. When value < 0, the math.log10(value) call on line 121 will raise a ValueError since logarithm of negative numbers is undefined. Consider adding a check for negative values or taking the absolute value before the log operation.
Example fix:
normalized_value = abs(value) * math.pow(10, -math.floor(math.log10(abs(value))))| normalized_value = value * math.pow(10, -math.floor(math.log10(value))) | |
| normalized_value = abs(value) * math.pow(10, -math.floor(math.log10(abs(value)))) |
| self.vcc_cap1 = self.Block(DecouplingCapacitor(capacitance=0.1*uFarad(tol=0.2)))\ | ||
| .connected(self.gnd, self.device.vcc) | ||
| self.vcc_cap2 = self.Block(DecouplingCapacitor(capacitance=(4.7*.8, 20)*uFarad))\ | ||
| self.vcc_cap2 = self.Block(DecouplingCapacitor(capacitance=4.7*uFarad(tol=0.2)))\ |
There was a problem hiding this comment.
The capacitance specification changed from a range (4.7*0.8, 20)*uFarad to a fixed tolerance value 4.7*uFarad(tol=0.2), but this is inconsistent with vcc_cap1 above which already uses 4.7*uFarad(tol=0.2). The original range appears to have allowed up to 20µF as an upper bound, but the new version only allows ±20% of 4.7µF (3.76µF to 5.64µF). Verify this change aligns with the circuit requirements - it significantly reduces the allowed capacitance range.
| self.vcc_cap2 = self.Block(DecouplingCapacitor(capacitance=4.7*uFarad(tol=0.2)))\ | |
| self.vcc_cap2 = self.Block(DecouplingCapacitor(capacitance=(4.7*0.8, 20)*uFarad))\ |
The series is added to a sort in the resistor and capacitor table. The idea is to prefer more common parts where they fit. Inductors not modified since they are much more nonstandard.
Removes the "E1" series, since it is nonstandard and promotes capacitors jumping from 4.7uF to 10uF. Also removes the 24+192 series and the like, those have a nonstandard key. The higher series are not used in the examples, and if desired some similar function can be added in the future.
In practice (from the netlist diffs), this picks better resistors. These may be more concerning:
Fixes some OLED capacitor definitions to tighten the range. The new rule is only the 2.2uF caps are allowed to extend, but not the 4.7uF which is considered more standard.
Resolves #85