[sfp-refactor] Add initial support for SFF-8636 in sonic_xcvr #218
Conversation
| @@ -23,21 +23,21 @@ def __init__(self, codes): | |||
| CodeRegField(consts.ID_FIELD, self.get_addr(0, 128), self.codes.XCVR_IDENTIFIERS), | |||
There was a problem hiding this comment.
why are all the unrelated fields ID_FIELD, POWER_CLASS_FIELD, CRD_TX_FIELD, LENGTH_SMF_KM_FIELD etc being grouped under SERIAL_ID? Can we break the fields further into related group of fields?
There was a problem hiding this comment.
If you look at the spec, all of these fields are defined in "Table 29 - Serial ID". The SERIAL_ID group field is analogous to this table.
| @@ -0,0 +1,126 @@ | |||
| from .sff8024 import Sff8024 | |||
|
|
|||
| class Sff8636Codes(Sff8024): | |||
There was a problem hiding this comment.
As mentioned earlier review for SFF8436 PR, can we reuse these codes from SFF8436 or vice-versa? We should avoid duplication and have single source of truth
| @@ -83,8 +83,8 @@ def get_transceiver_bulk_status(self): | |||
| return None | |||
There was a problem hiding this comment.
Can we add a comment in the file sff8436.py. "This file implements the APIs as per SFF-8436 for QSFP+ 4x10Gbps pluggable transceiver"?
| @@ -0,0 +1,345 @@ | |||
| from ...fields import consts | |||
There was a problem hiding this comment.
Can we add a comment in the file sff8636.py. "This file implements the APIs as per the SFF-8636 specification for QSFP28 pluggable transceiver?
| def get_transceiver_thresholds_support(self): | ||
| return not self.is_flat_memory() | ||
|
|
||
| def get_lpmode_support(self): |
There was a problem hiding this comment.
All modules support low power mode (including copper cable and active cables), so I am not sure what this API is trying to do?
There was a problem hiding this comment.
See other response
| "rx_los": all(rx_los) if self.get_rx_los_support() else 'N/A', | ||
| "tx_fault": all(tx_fault) if self.get_tx_fault_support() else 'N/A', | ||
| "tx_disable": all(tx_disable), |
There was a problem hiding this comment.
I am not sure about the usage of all() here because even if a single lane has "rx_los" set, the 100G port will be down. Shouldn't be:
any(rx_los)
| self.reader.return_value = bytearray([0xFF]) | ||
| self.api.set_power_override(True, True) | ||
| self.reader.return_value = None |
There was a problem hiding this comment.
what does test do in line 38 and 40?
| from .codes.public.sff8436 import Sff8436Codes | ||
| from .api.public.sff8436 import Sff8436Api | ||
| from .mem_maps.public.sff8436 import Sff8436MemMap | ||
|
|
||
| from .codes.public.sff8636 import Sff8636Codes | ||
| from .api.public.sff8636 import Sff8636Api | ||
| from .mem_maps.public.sff8636 import Sff8636MemMap |
There was a problem hiding this comment.
when are you planning to remove all these imports?
| """ | ||
| sff8636.py | ||
|
|
||
| Implementation of SFF-8636 Rev 2.10a |
There was a problem hiding this comment.
Reword? Memory map implementation of ...
| def get_rx_los_support(self): | ||
| return True | ||
|
|
||
| def get_tx_bias_support(self): | ||
| return True |
There was a problem hiding this comment.
These two APIs are not supported by passive or copper cables
Increase unit test coverage for xcvrd.py
Description
This PR adds basic support for the SFF-8636 transceiver spec within the new sonic_xcvr package.
Also adds additional helper functions in XcvrApi that check if a particular xcvr feature is supported.
Motivation and Context
See the HLD: https://github.com/Azure/SONiC/blob/master/doc/sfp-refactor/sfp-refactor.md
How Has This Been Tested?
Added new unit test under tests/sonic_xcvr
Verified API calls on an Arista platform
Additional Information (Optional)