[docs][transceiver]: DOM HLD - operational ranges are per-PN (drop default values, add guidance) - #2
Closed
mihirpat1 wants to merge 1 commit into
Closed
[docs][transceiver]: DOM HLD - operational ranges are per-PN (drop default values, add guidance)#2mihirpat1 wants to merge 1 commit into
mihirpat1 wants to merge 1 commit into
Conversation
Operational ranges depend on each transceiver's normal-operation envelope, so the HLD's concrete default values invite copy-paste that false-fails modules operating outside them. Convert the six _operational_range rows to format placeholders (matching the _threshold_range rows already used) and add guidance notes on per-PN derivation (with the warning-threshold containment rule) and omitting unsupported parameters. Signed-off-by: Mihir Patel <patelmi@microsoft.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the DOM transceiver test plan documentation to avoid implying universal “default” operational ranges for DOM sensors, and adds guidance for configuring operational ranges per transceiver part number (PN), including how to handle unsupported parameters.
Changes:
- Replaces concrete default values for
*_operational_rangeattributes with format placeholders to discourage copy/paste misconfiguration. - Adds guidance to derive operational ranges per PN and keep them within warning thresholds.
- Documents an “unsupported ⇒ omit” rule to avoid running checks against meaningless DOM fields.
Comments suppressed due to low confidence (2)
docs/testplan/transceiver/dom_test_plan.md:60
- Same Markdown rendering issue here:
<float>inside the table cell is interpreted as an HTML tag unless it’s inside inline code or escaped. Wrapping the JSON examples in backticks keeps the placeholders visible.
| laser_temperature_threshold_range | dict | (format) {"lowalarm": <float>, "lowwarning": <float>, "highwarning": <float>, "highalarm": <float>} | O | transceivers | Absolute threshold laser temperature range in Celsius (specify all four; omit to skip laser temperature threshold checks) |
| txLANE_NUMbias_operational_range | dict | (format) {"min": <float>, "max": <float>} | O | transceivers | Realistic operational TX bias current range in mA for lane LANE_NUM during normal operation |
| tx_bias_threshold_range | dict | (format) {"lowalarm": <float>, "lowwarning": <float>, "highwarning": <float>, "highalarm": <float>} | O | transceivers | Absolute threshold TX bias current range in mA (EEPROM limits; skip attribute to disable bias threshold validation) |
| txLANE_NUMpower_operational_range | dict | (format) {"min": <float>, "max": <float>} | O | transceivers | Realistic operational TX power range in dBm for lane LANE_NUM during normal operation |
| tx_power_threshold_range | dict | (format) {"lowalarm": <float>, "lowwarning": <float>, "highwarning": <float>, "highalarm": <float>} | O | transceivers | Absolute threshold TX power range in dBm (define all four for TX power threshold validation) |
docs/testplan/transceiver/dom_test_plan.md:62
- These table placeholders have the same
<float>-as-HTML-tag rendering problem; consider formatting the JSON as inline code so the angle-bracket placeholders remain visible in rendered Markdown.
| rxLANE_NUMpower_operational_range | dict | (format) {"min": <float>, "max": <float>} | O | transceivers | Realistic operational RX power range in dBm for lane LANE_NUM during normal operation |
| rx_power_threshold_range | dict | (format) {"lowalarm": <float>, "lowwarning": <float>, "highwarning": <float>, "highalarm": <float>} | O | transceivers | Absolute threshold RX power range in dBm (omit attribute to skip RX power threshold validation) |
Comment on lines
+51
to
+55
| | temperature_operational_range | dict | (format) {"min": <float>, "max": <float>} | O | transceivers | Realistic operational temperature range in Celsius during normal operation (typical: room temp to moderate heat) | | ||
| | temperature_threshold_range | dict | (format) {"lowalarm": <float>, "lowwarning": <float>, "highwarning": <float>, "highalarm": <float>} | O | transceivers | Absolute threshold temperature range in Celsius (must define all four keys; no implicit defaults) | | ||
| | voltage_operational_range | dict | {"min": 3.20, "max": 3.40} | O | transceivers | Realistic operational voltage range in volts during normal operation (typical: 3.3V ±3%) | | ||
| | voltage_operational_range | dict | (format) {"min": <float>, "max": <float>} | O | transceivers | Realistic operational voltage range in volts during normal operation (typical: 3.3V ±3%) | | ||
| | voltage_threshold_range | dict | (format) {"lowalarm": <float>, "lowwarning": <float>, "highwarning": <float>, "highalarm": <float>} | O | transceivers | Absolute threshold voltage range in volts (provide EEPROM alarm/warn limits; skip to disable voltage threshold validation) | | ||
| | laser_temperature_operational_range | dict | {"min": 20.0, "max": 70.0} | O | transceivers | Realistic operational laser temperature range in Celsius during normal operation | | ||
| | laser_temperature_operational_range | dict | (format) {"min": <float>, "max": <float>} | O | transceivers | Realistic operational laser temperature range in Celsius during normal operation | |
|
/azp run |
Owner
Author
|
Superseded by upstream PR sonic-net#26663 (base should target upstream, not the fork). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
Summary: The DOM test plan's Attributes table lists concrete default values for the six
*_operational_rangeattributes. Operational ranges are inherently per-transceiver — they depend on the module's normal-operation envelope — so a single default invites copy-paste that false-fails modules operating outside it. This change converts the operational-range rows to format placeholders (as the*_threshold_rangerows already are) and documents how to derive operational ranges per part number, plus the "unsupported ⇒ omit" rule.MSFT ADO - 39011423
Type of change
Back port request
Approach
What is the motivation for this PR?
A module whose Tx/Rx power operates at ~+3 dBm would false-fail the current illustrative operational maxes if those were copied verbatim into a module's
dom.json. Because the presence of an attribute drives verification, shipping concrete default values in the HLD is a footgun.How did you do it?
Replaced the default-value cells for the six
*_operational_rangeattributes with(format) {"min": <float>, "max": <float>}(matching the existing*_threshold_rangerows), and added two notes:lowwarning < operational_min,operational_max < highwarning).*_operational_rangeand*_threshold_rangefor a parameter the module does not support (e.g.,laser_temperaturewith all-zero threshold fields), so availability/range checks don't run against meaningless data.How did you verify/test it?
Documentation-only change; no code paths modified. Validated the guidance against a live module whose operational values exceed the previous illustrative maxes.
Any platform specific information?
No.
Supported testbed topology if it's a new test case?
N/A (documentation).
Documentation
HLD updated:
docs/testplan/transceiver/dom_test_plan.md.