-
Notifications
You must be signed in to change notification settings - Fork 163
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
[xcvrd] Modify to support regular expression when parsing the key in media_settings.json #471
Conversation
…media_settings.json
@DennisChiuEC Please add unit test |
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.
OK, I will add the unit test case. |
@tshalvi would you please review? |
if is_si_per_speed_supported(media_dict[key[VENDOR_KEY]]): | ||
if key[LANE_SPEED_KEY] is not None and key[LANE_SPEED_KEY] in media_dict[key[VENDOR_KEY]]: # e.g: 'speed:400GAUI-8' | ||
return media_dict[key[VENDOR_KEY]][key[LANE_SPEED_KEY]] | ||
for dict_key in media_dict.keys(): |
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.
@DennisChiuEC can you write a function get_media_settings
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.
Done
if is_si_per_speed_supported(media_dict[key[MEDIA_KEY]]): | ||
if key[LANE_SPEED_KEY] is not None and key[LANE_SPEED_KEY] in media_dict[key[MEDIA_KEY]]: | ||
return media_dict[key[MEDIA_KEY]][key[LANE_SPEED_KEY]] | ||
for dict_key in media_dict.keys(): |
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.
@DennisChiuEC can you write a function get_media_settings
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.
Done
@@ -168,6 +169,19 @@ def get_media_settings_value(physical_port, key): | |||
media_dict = {} | |||
default_dict = {} | |||
|
|||
def get_media_settings(key, media_dict): | |||
for dict_key in media_dict.keys(): | |||
if ( re.match(dict_key, key[VENDOR_KEY]) or re.match(dict_key, key[VENDOR_KEY].split('-')[0]) # e.g: 'AMPHENOL-1234' |
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.
if ( re.match(dict_key, key[VENDOR_KEY]) or re.match(dict_key, key[VENDOR_KEY].split('-')[0]) # e.g: 'AMPHENOL-1234' | |
if (re.match(dict_key, key[VENDOR_KEY]) or \ | |
re.match(dict_key, key[VENDOR_KEY].split('-')[0]) # e.g: 'AMPHENOL-1234' |
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.
Done
if key[LANE_SPEED_KEY] is not None and key[LANE_SPEED_KEY] in media_dict[dict_key]: # e.g: 'speed:400GAUI-8' | ||
return media_dict[dict_key][key[LANE_SPEED_KEY]] | ||
else: | ||
return {} |
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 are we returning {}
and not 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.
It was added by the previous PR.
I just defined a function to simplify the code.
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.
@tshalvi can you please review?
@prgeor could we add |
@bingwang-ms Can you please help to cherry-pick this to 202405? |
…media_settings.json (sonic-net#471) * [xcvrd] Modify to support regular expression when parsing the key in media_settings.json * fix unit test error * add unit test for getting media settings value with regular expression * define get_media_settings() * apply the suggestion for if condition
Cherry-pick PR to 202405: #537 |
…media_settings.json (#471) * [xcvrd] Modify to support regular expression when parsing the key in media_settings.json * fix unit test error * add unit test for getting media settings value with regular expression * define get_media_settings() * apply the suggestion for if condition
…media_settings.json (sonic-net#471) * [xcvrd] Modify to support regular expression when parsing the key in media_settings.json * fix unit test error * add unit test for getting media settings value with regular expression * define get_media_settings() * apply the suggestion for if condition
Cherry-pick PR to 202311: #544 |
…media_settings.json (#471) * [xcvrd] Modify to support regular expression when parsing the key in media_settings.json * fix unit test error * add unit test for getting media settings value with regular expression * define get_media_settings() * apply the suggestion for if condition
@longhuan-cisco Can you please help to create a separate 202311 PR for PR#533 since the dependent PR#471 is now cherry-picked to 202311. |
raised #545 for it |
Description
Let media_settings.json can use regular expression to match 'vendor key' or 'media key'
when define a set of transceiver to use the same SI value.
Motivation and Context
Make the definition of SI value for a set of vendor or media can be more flexible
How Has This Been Tested?
root@sonic:~# cat /var/log/swss/swss.rec | grep preemphasis
2024-04-17.01:53:10.594465|PORT_TABLE:Ethernet0|SET|preemphasis:0x0c8418,0x0c8418,0x0c8418,0x0c8418
2024-04-17.01:53:11.827324|PORT_TABLE:Ethernet4|SET|preemphasis:0x0c8418,0x0c8418,0x0c8418,0x0c8418
Example of media_settings.json
"GLOBAL_MEDIA_SETTINGS": {
"1-32": {
"VENDOR.*-PN.*": {
"preemphasis": {
"lane0": "0x0c8418",
"lane1": "0x0c8418",
"lane2": "0x0c8418",
"lane3": "0x0c8418",
"lane4": "0x0c8418"
}
}
}
Additional Information (Optional)