Add support for get_power_override_support in sfp API#641
Add support for get_power_override_support in sfp API#641arista-hpandya wants to merge 1 commit into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
317650c to
0bd88f3
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds a new SFP API method to query whether a module supports power override, to let callers check capability before invoking get_power_override() / set_power_override().
Changes:
- Introduces
get_power_override_support()onSfpBaseas part of the common SFP interface. - Implements
get_power_override_support()inSfpOptoeBaseby delegating to the underlyingxcvr_api.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sonic_platform_base/sfp_base.py |
Adds the new SFP interface method for power-override support querying. |
sonic_platform_base/sonic_xcvr/sfp_optoe_base.py |
Implements the new method for optoe-based SFPs via get_xcvr_api() delegation. |
| Retrieves whether power override is supported by this SFP | ||
|
|
||
| Returns: | ||
| A Boolean, True if power-override is supported, False if not supported | ||
| """ | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Adding a new method to SfpBase that raises NotImplementedError introduces a new required override for every downstream platform SFP implementation. This is a backwards-incompatible API change because callers can now hit a runtime exception on platforms that haven’t implemented the new method. Provide a safe default implementation (e.g., return False when unsupported) and document the default/unsupported behavior in the docstring so existing platforms don’t break.
| Retrieves whether power override is supported by this SFP | |
| Returns: | |
| A Boolean, True if power-override is supported, False if not supported | |
| """ | |
| raise NotImplementedError | |
| Retrieves whether power override is supported by this SFP. | |
| Returns: | |
| bool: True if power-override is supported, False if not supported. | |
| The default implementation returns False to indicate that | |
| power override is not supported unless overridden by a | |
| platform-specific implementation. | |
| """ | |
| return False |
There was a problem hiding this comment.
It raises a NotImplementedError to align with the existing methods and prompt vendors to implement this. Returning False as a default wll be misleading and has a potential to cause silent failures here since the xcvr might support the power override. sonic-mgmt tests will handle NotImplementedError on the caller side.
There was a problem hiding this comment.
@arista-hpandya which vendor? this comment is valid. we cannot break or introduce this failure . can you check all the implementation APIs, SFF, CMIS etc
There was a problem hiding this comment.
@prgeor per your suggestion, I relaxed the return value to None from NotImplementedError but I am not sure I understand the concern that NotImplementedError could lead to errors. SfpBase is only inherited by SfpOptoeBase, which calls the API specific get_power_override_support method. Currently all APIs have implemented this method:
cmis.py: def get_power_override_support(self):
cmis.py- return False
--
sff8436.py: def get_power_override_support(self):
sff8436.py- return not self.is_copper()
--
sff8636.py: def get_power_override_support(self):
sff8636.py- return not self.is_copper()
--
sff8472.py: def get_power_override_support(self):
sff8472.py- return False
CCmisApi, CmisAec800gApi, and CmisFr800gApi all inherit from Cmis so they too cover this method.
So the goal is to just expose it via the sfp platform api layer. The APIs returned by get_xcvr_api include:
id_mapping = {
0x03: (self._create_api, (Sff8472Codes, Sff8472MemMap, Sff8472Api)),
0x0D: (self._create_qsfp_api, ()),
0x11: (self._create_api, (Sff8636Codes, Sff8636MemMap, Sff8636Api)),
0x18: (self._create_cmis_api, ()),
0x19: (self._create_cmis_api, ()),
0x1b: (self._create_cmis_api, ()),
0x1e: (self._create_cmis_api, ()),
0x7e: (self._create_api, (AmphBackplaneCodes,
AmphBackplaneMemMap, AmphBackplaneImpl)),
}
Each of the valid return values currently map to one of the api source files mentioned above. Do let me know if I am missing something.
0bd88f3 to
701dcc8
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: arista-hpandya <hpandya@arista.com>
701dcc8 to
ddbb0d7
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@arista-hpandya code coverage missing in the pr checked |
Description
This PR introduces the get_power_override_support method to the SFP API within sonic-platform-common. Needed to address: sonic-net/sonic-buildimage#26372
sfp_base.py: Added the abstract method definition to the SfpBase class to define the interface.
sfp_optoe_base.py: Provided an implementation in SfpOptoeBase that delegates the call to the underlying transceiver API (xcvr_api).
This allows platform-specific implementations and upper-layer applications to query whether a specific SFP module supports power override capabilities before attempting to get or set override values.
Motivation and Context
Currently, while get_power_override and set_power_override exist, there isn't a standardized way to check for feature support across different hardware modules. This change improves the robustness of the SFP interface by allowing callers to verify support programmatically, preventing unnecessary errors or invalid calls on modules that do not support this feature.
This will help us move away from hardcoded functions like these that need constant maintenance: https://github.com/sonic-net/sonic-mgmt/blob/a1d1fd8717c4525055a3a4a9e3250232ee3b0d86/tests/platform_tests/api/test_sfp.py#L374-L378
How Has This Been Tested?
Verified by running the test_sfp sonic-mgmt tests to use this API
Additional Information (Optional)
NA