-
Notifications
You must be signed in to change notification settings - Fork 177
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
Added Ext Media Library #129
base: master
Are you sure you want to change the base?
Conversation
@@ -25,6 +25,8 @@ | |||
from .sff8436 import sff8436InterfaceId # Dot module supports both Python 2 and Python 3 using explicit relative import methods | |||
from .sff8436 import sff8436Dom # Dot module supports both Python 2 and Python 3 using explicit relative import methods | |||
from .inf8628 import inf8628InterfaceId # Dot module supports both Python 2 and Python 3 using explicit relative import methods | |||
import sys |
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.
sys is imported above
@@ -25,6 +25,8 @@ | |||
from .sff8436 import sff8436InterfaceId # Dot module supports both Python 2 and Python 3 using explicit relative import methods | |||
from .sff8436 import sff8436Dom # Dot module supports both Python 2 and Python 3 using explicit relative import methods | |||
from .inf8628 import inf8628InterfaceId # Dot module supports both Python 2 and Python 3 using explicit relative import methods | |||
import sys | |||
import sonic_platform_base.sonic_sfp.ext_media_api as ext_media_module |
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.
Move this import to the third-party import group (the second group), and place it in alphabetical order within that group.
@@ -0,0 +1,200 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,158 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,147 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,162 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,144 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,113 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,30 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,125 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
@@ -0,0 +1,165 @@ | |||
######################################################################## | |||
# DellEMC |
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 file is not Dell-specific. Please remove this line.
It would be helpful to add UT here with mocked-up EEPROM data. However, no UT infra exists for this repo. Open question for community: should we gate this PR based on also providing the entire UT infra for this repo? Or can we address the UT infra in a separate PR? UT infra and comprehensive test cases would be beneficial for sure, but will take significant time to develop. IMHO it might even be helpful to track significant efforts like these as deliverable features on their own. |
# Get the corresponding form-factor | ||
form_factor_name, form_factor_module = get_form_factor_info(eeprom_bytes) | ||
if None is form_factor_name: | ||
return None |
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.
We should log an error message that a ext_media_handler is needed for this xcvr type.
if None is form_factor_name: | ||
return None | ||
if None is form_factor_module: | ||
raise NotImplementedError("Unable to find implementation for form-factor: " + str(form_factor_name)) |
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.
We probably need a different message here to indicate form_factor_module mapping not found for str(form_factor_name) in ext_media_common.py::form_factor_handler_to_ff_info.
# | ||
######################################################################## | ||
|
||
import importlib |
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.
unused
if form_factor_module_name.split('ext_media_handler_')[1] == cl_name: | ||
handler_class = cl | ||
break | ||
if handler_class is None: |
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.
pls log an error message to indicate that the class with required name is not defined in the media handler file for this xcvr type.
Unit test infrastructure has been added to the repo via #139. You should now be able to add unit tests. |
pass | ||
|
||
@abstractmethod | ||
def get_vendor_name(self, eeprom): |
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.
what's the intention to add this API here? Isn't it already available in API get_transceiver_bulk_status() which is defined in sfp_base.py? same comments goes to vendor_oui, vendor_serial_number, etc.
def get_transceiver_info(self):
"""
Retrieves transceiver info of this SFP
Returns:
A dict which contains following keys/values :
================================================================================
keys |Value Format |Information
---------------------------|---------------|----------------------------
type |1*255VCHAR |type of SFP
hardware_rev |1*255VCHAR |hardware version of SFP
serial |1*255VCHAR |serial number of the SFP
manufacturer |1*255VCHAR |SFP vendor name
model |1*255VCHAR |SFP model name
connector |1*255VCHAR |connector information
encoding |1*255VCHAR |encoding information
ext_identifier |1*255VCHAR |extend identifier
ext_rateselect_compliance |1*255VCHAR |extended rateSelect compliance
cable_length |INT |cable length in m
nominal_bit_rate |INT |nominal bit rate by 100Mbs
specification_compliance |1*255VCHAR |specification compliance
vendor_date |1*255VCHAR |vendor date
vendor_oui |1*255VCHAR |vendor OUI
application_advertisement |1*255VCHAR |supported applications advertisement
================================================================================
from ext_media_utils import * | ||
from ext_media_handler_base import media_static_info | ||
|
||
QSFP28_LENGTH_ADDR = media_eeprom_address(offset=146) |
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.
why QSFP28 and QSFP+ need to be handled in different files? Shouldn't they all follow SFF8436/SFF8636? Btw, we already have an sff8436 parser, are all these new added here not available in the current SFF8436 parser? If yes could I suggest that to extend the current parser instead of adding a new separate parser?
from ext_media_utils import * | ||
from ext_media_handler_base import media_static_info | ||
|
||
QSFP56_DD_CMIS_1_LENGTH_ADDR = media_eeprom_address(offset=146) |
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.
We have a QSFP-DD parser which support CMIS modules, is it possible to reuse it here?
@@ -995,7 +997,27 @@ def get_transceiver_info_dict(self, port_num): | |||
transceiver_info_dict['specification_compliance'] = str(compliance_code_dict) | |||
|
|||
transceiver_info_dict['nominal_bit_rate'] = str(sfp_interface_bulk_data['data']['NominalSignallingRate(UnitsOf100Mbd)']['value']) | |||
|
|||
sys.path.append('/usr/share/sonic/platform') |
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 path only work inside PMON, is there some case that this file could be called from the host side?
DEFAULT_NO_DATA_VALUE = 'N/A' | ||
|
||
# Connector codes are common to all form-factors | ||
connector_code_to_name_map = { |
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.
we already have this defined in both parser SFF8472 and SFF8436, can I suggest to extend and reuse?
SFF8436
connector = {
'00': 'Unknown or unspecified',
'01': 'SC',
'02': 'FC Style 1 copper connector',
'03': 'FC Style 2 copper connector',
'04': 'BNC/TNC',
'05': 'FC coax headers',
'06': 'Fiberjack',
'07': 'LC',
'08': 'MT-RJ',
'09': 'MU',
'0a': 'SG',
'0b': 'Optical Pigtail',
'0c': 'MPOx12',
'0d': 'MPOx16',
'20': 'HSSDC II',
'21': 'Copper pigtail',
'22': 'RJ45',
'23': 'No separable connector'
}
SFF8472
connector = {'00': 'Unknown',
'01': 'SC',
'02': 'Fibre Channel Style 1 copper connector',
'03': 'Fibre Channel Style 2 copper connector',
'04': 'BNC/TNC',
'05': 'Fibre Channel coaxial headers',
'06': 'FibreJack',
'07': 'LC',
'08': 'MT-RJ',
'09': 'MU',
'0a': 'SG',
'0b': 'Optical pigtail',
'0c': 'MPO Parallel Optic',
'20': 'HSSDCII',
'21': 'CopperPigtail',
'22': 'RJ45'}
affe6be
to
c31636e
Compare
…r conditions/events (sonic-net#129) * [xcvrd] Fix y_cable state update to unknown on erroraneous events This PR provides the support for replacing the state DB updates from 'failure' to 'unknown' in case there is an error event in the functioning of Y cable What is the motivation for this PR? the schema agreed upon with linkmgr and orchagent interaction with xcvrd, is that if there is an error event xcvrd need to fill the state DB with 'unknown' as the state value rather than 'failure', this PR handles that How did you do it? identified error scenario's in the code and made the changes Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
- What I did
Added media parser library
CLI PR is sonic-net/sonic-utilities#1156
- How I did it
Added new common lib
- How to verify it
Visually verified it against expected values
- Previous command output (if the output of a command-line utility has changed)
Nil
- New command output (if the output of a command-line utility has changed)