-
Notifications
You must be signed in to change notification settings - Fork 162
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
Apply custom Si settings via CMIS: SONiC xcvrd platform daemon changes #385
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.
Please write the appropriate test code to cover the new APIs/changes
@keboliu @mihirpat1 please review |
@AnoopKamath can you point to the HLD in this PR description? |
@prgeor I have added reference to HLD and CMIS changes also. Thanks |
@prgeor : If there are no comments, please approve. |
|
||
optics_si_settings_file_path = os.path.join(platform_path, "optics_si_settings.json") | ||
if not os.path.isfile(optics_si_settings_file_path): | ||
helper_logger.log_error("No optics SI file exists") |
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.
@AnoopKamath why is this error?
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.
@prgeor : Changed it for debugging. Will revert to info
if vendor_key is None or vendor_name is None: | ||
helper_logger.log_error("Error: No Vendor Key found for port '{}'".format(logical_port_name)) | ||
return optics_si | ||
optics_si = get_optics_si_settings_value(physical_port, lane_speed, vendor_key, vendor_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.
@AnoopKamath what is vendor_key? if PN please rename the variable accordingly.
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.
@prgeor : vendor_key is combination of vendor_pn + vendor_name. We use vendor_name for vendor name search only
sonic-xcvrd/xcvrd/xcvrd.py
Outdated
optics_si_dict = optics_si_parser.fetch_optics_si_setting(pport, lane_speed, sfp) | ||
|
||
if optics_si_dict: | ||
self.log_notice("{}: Optics SI found. Apply".format(lport)) |
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.
@AnoopKamath can we log the vendor_key and the lane speed also in the log?
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.
@prgeor: Sure. Will do it
@StormLiangMS please cherry pick to 202305 |
ADO - 24988570 |
sonic-net#385) * Optics SI settings changes for platform daemon
#385) * Optics SI settings changes for platform daemon
Apply custom Si settings via CMIS: SONiC xcvrd daemon changes
Description
HLD : sonic-net/SONiC#1334
CMIS changes: sonic-net/sonic-platform-common#384
Motivation and Context
Certain high-speed QSFP_DD, OSFP and QSFP modules require Signal Integrity (SI) settings to match platform media settings in order to achieve link stability, right tunning and optimal performance.
How Has This Been Tested?
Added few debugs and made sure right SI settings were picked up and applied successfully.
Validated all flags and eeprom details
Additional Information (Optional)