[CDB] Separate CDB handler functionality from CmisAPI - #740
Draft
prgeor wants to merge 3 commits into
Draft
Conversation
Signed-off-by: Prince George <prgeor@microsoft.com>
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors CMIS CDB firmware handling in sonic_platform_base/sonic_xcvr to move CDB firmware handler ownership into the CmisCdbFw helper and reduce coupling/duplicate state in CmisApi, with corresponding updates to API construction call sites and a couple of unit tests.
Changes:
- Refactors
CmisApito use composition (self.cdb_fw) instead of inheritingCmisCdbFw, and adds a wrapper forget_module_fw_info(). - Simplifies/centralizes CDB handler creation inside
CmisCdbFw. - Updates transceiver API factory and a subset of tests to use the new constructor signatures.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sonic_xcvr/test_sfp_optoe_base.py | Updates CMIS/CCMIS API instantiation to match new constructor signature. |
| tests/sonic_xcvr/test_ccmis.py | Updates CCMIS API instantiation to match new constructor signature. |
| sonic_platform_base/sonic_xcvr/xcvr_api_factory.py | Removes init_cdb_fw_handler argument from CMIS-family API creation sites. |
| sonic_platform_base/sonic_xcvr/api/public/cmis.py | Switches from inheritance to composition for CDB firmware support and adds get_module_fw_info() wrapper. |
| sonic_platform_base/sonic_xcvr/api/public/cdb_fw.py | Moves handler/mem-map ownership into CmisCdbFw and changes initialization behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+141
to
+152
| self.cdb_fw = None | ||
| if self.is_cdb_supported(): | ||
| self.cdb_fw = CmisCdbFw(xcvr_eeprom) | ||
|
|
||
| def get_module_fw_info(self): | ||
| """ | ||
| Get firmware information from the CDB helper when supported. | ||
| """ | ||
| if self.cdb_fw is None: | ||
| return {'status': False, 'info': "CDB Not supported", 'result': None} | ||
|
|
||
| return self.cdb_fw.get_module_fw_info() |
Comment on lines
+23
to
27
| def __init__(self, xcvr_eeprom): | ||
| self.xcvr_eeprom = xcvr_eeprom | ||
| self._cdb_mem_map = CdbMemMap(CdbCodes) | ||
| self.cdb_fw_hdlr = self._create_cdb_fw_handler() | ||
|
|
| } | ||
|
|
||
| class CmisApi(CmisCdbFw, XcvrApi): | ||
| class CmisApi(XcvrApi): |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
sonic_platform_base/sonic_xcvr/api/public/cmis.py:143
- CDB helper initialization is decided only once in init based on is_cdb_supported(). If the initial EEPROM read returns None (transient failure), cdb_fw stays None and firmware queries will be permanently treated as unsupported for this instance. Consider lazily instantiating CmisCdbFw on first firmware call, and re-checking capability each time until it is known.
self.cdb_fw = None
if self.is_cdb_supported():
self.cdb_fw = CmisCdbFw(xcvr_eeprom)
sonic_platform_base/sonic_xcvr/api/public/cmis.py:108
- Changing CmisApi to no longer inherit from CmisCdbFw removes CDB firmware helper methods/attributes from the public CmisApi surface (e.g., cdb_fw_hdlr, get_module_fw_mgmt_feature(), get_status_code(), cdb_epl_block_write()). Repository unit tests still call these APIs (see tests/sonic_xcvr/test_cmis.py around lines 1760-1970), so this refactor will break existing callers unless those tests/call sites are updated to use the helper (api.cdb_fw.) or CmisApi provides compatibility delegations.
class CmisApi(XcvrApi):
Comment on lines
+23
to
+26
| def __init__(self, xcvr_eeprom): | ||
| self.xcvr_eeprom = xcvr_eeprom | ||
| self._cdb_mem_map = CdbMemMap(CdbCodes) | ||
| self.cdb_fw_hdlr = self._create_cdb_fw_handler() |
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Prince George <prgeor@microsoft.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change cleans up the CMIS CDB firmware handling by separating capability detection from CDB helper ownership.
Previously, the CDB firmware state was duplicated across the API and the helper object, creating unnecessary coupling:
CmisApi stored a redundant cdb_fw_hdlr alias
the API also exposed a pass-through _create_cdb_fw_handler() method
the actual CDB firmware logic was spread across ownership boundaries instead of staying with the helper object
Motivation and Context
The CDB firmware functionality is a capability-specific helper, not core CMIS API state. Keeping CDB handler creation and memory-map ownership on the API class made the design harder to reason about and duplicated state that could drift out of sync.
This cleanup makes the ownership boundaries explicit and reduces the risk of wrong lifecycle management for CMIS modules that do not support CDB. The refactor keeps the external behavior intact while aligning the implementation with the actual capability model.
This results in a simpler composition model:
How Has This Been Tested?
Additional Information (Optional)