Skip to content
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

[platform api] Fix sfp index #2937

Closed
wants to merge 1 commit into from

Conversation

antoninamelnyk
Copy link
Contributor

Signed-off-by: Antonina Melnyk [email protected]

Description of PR

Summary:
Fixes # (issue)
Change sfp index from 0-based to 1-based

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Approach

What is the motivation for this PR?

On practice chassis.get_sfp function accept a 1-based index as its argument. Before the fix, returned values were shifted by one.

How did you do it?

Change sfp index from 0-based to 1-based

How did you verify/test it?

Test is locally

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Signed-off-by: Antonina Melnyk <[email protected]>
@jleveque
Copy link
Contributor

jleveque commented Feb 5, 2021

On practice chassis.get_sfp function accept a 1-based index as its argument. Before the fix, returned values were shifted by one.

This is not true. chassis.get_sfp() accepts a 0-based index. Please see here

    def get_sfp(self, index):
        """
        Retrieves sfp represented by (0-based) index <index>
        Args:
            index: An integer, the index (0-based) of the sfp to retrieve.
                   The index should be the sequence of a physical port in a chassis,
                   starting from 0.
                   For example, 0 for Ethernet0, 1 for Ethernet4 and so on.
        Returns:
            An object dervied from SfpBase representing the specified sfp
        """

@antoninamelnyk
Copy link
Contributor Author

chassis.get_sfp() accepts a 0-based index.

Ok, I see. But most implementations have this method overridden to accept 1-based index. So returned values in tests are shifted by one. Do we need platform-specific exceptions for this?

@jleveque
Copy link
Contributor

jleveque commented Feb 9, 2021

chassis.get_sfp() accepts a 0-based index.

Ok, I see. But most implementations have this method overridden to accept 1-based index. So returned values in tests are shifted by one. Do we need platform-specific exceptions for this?

Can you please link to an example of an implementation where the method is overridden to accept 1-based index?

@antoninamelnyk
Copy link
Contributor Author

Can you please link to an example of an implementation where the method is overridden to accept 1-based index?

For example, Arista implementation or Mellanox implementation

jleveque added a commit to sonic-net/sonic-platform-common that referenced this pull request Feb 9, 2021
…h other drivers (#163)

Update method name to align with other drivers. Change from `parse_qsfp_dom_capability` to `parse_dom_capability`. Other drivers were aligned via sonic-net/sonic-mgmt#2937.

When this submodule is updated in sonic-buildimage, all callers will need to be updated to reflect the new nomenclature.
jleveque added a commit to sonic-net/sonic-platform-common that referenced this pull request Feb 9, 2021
…h other drivers (#163)

Update method name to align with other drivers. Change from `parse_qsfp_dom_capability` to `parse_dom_capability`. Other drivers were aligned via sonic-net/sonic-mgmt#2937.

When this submodule is updated in sonic-buildimage, all callers will need to be updated to reflect the new nomenclature.
@jleveque
Copy link
Contributor

jleveque commented Feb 10, 2021

Can you please link to an example of an implementation where the method is overridden to accept 1-based index?

For example, Arista implementation or Mellanox implementation

This is due to the fact that these vendors have defined their starting index to be 1 rather than 0 (example). If you look through all the platforms, you will see many which index their ports starting from 0. We suggest vendors index their ports starting from 0 (example). If you read the SONiC Porting Guide, "if the front panel ports are starting from 1, the index should start from 1. If the front panel ports start from 0, the index should start form 0. The break out ports from the same physical port should have same index.". Therefore, the base of this index is platform-specific.

Copy link
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not merge this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants