Add support for custom serdes attributes#643
Add support for custom serdes attributes#643longhuan-cisco wants to merge 9 commits intosonic-net:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prgeor @mihirpat1 could you please review, thanks |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for custom SerDes attributes in the media settings parser, allowing custom attributes to be defined in media_settings.json with a CUSTOM: prefix and converted to JSON format for consumption by the SAI layer.
- Introduces parsing logic for custom SerDes attributes prefixed with
CUSTOM: - Adds conversion functions to handle hex/int string values and convert them to signed integers
- Implements JSON serialization of custom attributes for database storage
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sonic-xcvrd/xcvrd/xcvrd_utilities/media_settings_parser.py | Adds custom SerDes attribute parsing logic, helper functions for string-to-int conversion, and integration into the media setting notification flow |
| sonic-xcvrd/tests/test_xcvrd.py | Adds unit tests for the custom SerDes attribute handling functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sonic-platform-daemon side change: sonic-net/sonic-platform-daemons#643 What I did Support SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION (i.e. custom serdes attributes represented in JSON based string format) Use boost::variant for the serdes_attr map value to support both std::vector<uint32_t> and std::string in a type-safe way, with easy extensibility to add more types later.
|
@prgeor kindly reminder, thanks |
| return port_speed, lane_count, subport_num | ||
|
|
||
|
|
||
| def convert_to_int32(value): |
There was a problem hiding this comment.
@longhuan-cisco I am not sure why custom values need this conversion compared to existing SI values
There was a problem hiding this comment.
@prgeor Agree, changed it to take custom attr values as-is from media_settings.json
It's up to the vendor/platform to use whatever types of values.
|
@longhuan-cisco can we have an HLD describing this ? We have similar for We can have |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@prgeor
Were you suggesting to put custom attributes in a new |
Signed-off-by: Longyin Huang <longhuan@cisco.com>
Signed-off-by: Longyin Huang <longhuan@cisco.com>
Signed-off-by: Longyin Huang <longhuan@cisco.com>
Signed-off-by: Longyin Huang <longhuan@cisco.com>
…des attrs Signed-off-by: Longyin Huang <longhuan@cisco.com>
Signed-off-by: Longyin Huang <longhuan@cisco.com>
8aa21fb to
e7c8400
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sonic-platform-daemon side change: sonic-net/sonic-platform-daemons#643 What I did Support SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION (i.e. custom serdes attributes represented in JSON based string format) Use boost::variant for the serdes_attr map value to support both std::vector<uint32_t> and std::string in a type-safe way, with easy extensibility to add more types later. Signed-off-by: Kalash Nainwal <kalash@nexthop.ai>
sonic-platform-daemon side change: sonic-net/sonic-platform-daemons#643 What I did Support SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION (i.e. custom serdes attributes represented in JSON based string format) Use boost::variant for the serdes_attr map value to support both std::vector<uint32_t> and std::string in a type-safe way, with easy extensibility to add more types later.
@longhuan-cisco do you have the PR for HLD? |
|
Signed-off-by: Longyin Huang <longhuan@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sonic-platform-daemon side change: sonic-net/sonic-platform-daemons#643 What I did Support SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION (i.e. custom serdes attributes represented in JSON based string format) Use boost::variant for the serdes_attr map value to support both std::vector<uint32_t> and std::string in a type-safe way, with easy extensibility to add more types later.
- add CUSTOM_MEDIA_SETTINGS lookup with shared media key matching - convert custom SerDes attrs into custom_serdes_attrs JSON payload - add selector parsing + tests for custom media settings and notify path Signed-off-by: Longyin Huang <longhuan@cisco.com>
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
711fb17 to
3858b95
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sonic-platform-daemon side change: sonic-net/sonic-platform-daemons#643 What I did Support SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION (i.e. custom serdes attributes represented in JSON based string format) Use boost::variant for the serdes_attr map value to support both std::vector<uint32_t> and std::string in a type-safe way, with easy extensibility to add more types later. Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>
|
|
||
| return None | ||
|
|
||
| def is_port_selected(port_selector, physical_port): |
There was a problem hiding this comment.
@longhuan-cisco can we keep the parsing for port ',' and '-' parsing similar to done in GLOBAL_MEDIA_SETTINGS_KEY
There was a problem hiding this comment.
@prgeor
The code here is achieving the same functionality as GLOBAL_MEDIA_SETTINGS_KEY in terms of parsing ',' and '-', just written in a cleaner way.
The only additional thing here is: print notice syslog messages to tell syntax error of ranged-ports key instead of silently ignoring it.
The code here is only consumed by CUSTOM_MEDIA_SETTINGS. The GLOBAL_MEDIA_SETTINGS still relies on the existing ranged-ports parsing code <-- I didn't touch that part.
Please let me know if you really prefer a copy-paste duplication of the original ranged-port parsing code of GLOBAL_MEDIA_SETTINGS_KEY.
|
@longhuan-cisco I have opened this refactor PR and should make things easier for you. can you please review? |
@prgeor Thanks for the refactor — this is heading in a nice direction. |
HLD: sonic-net/SONiC#2173
sonic-swss side change: sonic-net/sonic-swss#3764
Description
Add support for custom serdes attrs SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION
In media_settings.json, custom attributes can be defined by using
CUSTOM:as prefix, and existing parsing logic/format will be reused.Example media_settings.json snippet
Motivation and Context
How Has This Been Tested?
Tested end-to-end xcvrd->OA->SAI/SDK on 8-hostlanes optics with both non-breakout and breakout modes.
Additional Information (Optional)