[sonic_y_cable] add abstract class YCableBase required for Y-cable API support for multiple vendors #186

vdahiya12 · 2021-05-05T05:49:41Z

Description

For multiple Y-Cable vendor support once we do have a mapping from vendor/part number to the appropriate Y-Cable module to load, we need to map appropriate port to a module as well. This PR adds definition for base abstract class.

Motivation and Context

Basically, the key idea is once we have a port identified as being of a certain vendor and it has been identified to load a certain module, it is then needed to call the correct module/API on each port each time we call the API on the port
it is required to maintain this mapping in memory since xcvrd does not want to read/parse this y_cable_vendor_mapping.py file again and again each time we call the Y-Cable API
Also note that the module loaded might change during xcvrd running lifetime since cable/transceiver can be changed from one vendor to another. So we need to take this into consideration as well

Proposed Solution for this
Each module of the Y-Cable vendor can be a class (of each transceiver type) and all we need to do is instantiate the objects of these classes as class instances and these objects will provide the interface of calling the API's for the appropriate vendor Y-Cable.

This instantiation will be done inside xcvrd, when xcvrd starts

These objects basically emulate Y-Cable instances and whatever action/interaction needs to be done with the Y-Cable the methods of these objects would provide that
each vendor in their implementation can inherit from a base class where there will be definitions for all the supported capabilities of the Y-Cable.
for vendors the recommended approach in case their subclass implementation does not support a method, is to set the method equal to None. This differentiates it from a method they forgot to implement. Then, the calling code should first check if the method is None before attempting to call it.

Design document for the support is here

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com

How Has This Been Tested?

Not tested, since this PR just defines the abstract base class implementation.

Additional Information (Optional)

…I support for multiple vendors Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

theasianpianist · 2021-05-05T21:31:26Z

sonic_y_cable/y_cable_base.py

+        TOR A. This means if the Y cable is actively routing, the "check_active_linked_tor_side"
+        API will now return Tor A. It also implies that if the link is actively routing on this port, Y cable


nit: maybe change 'routing' to 'sending', since 'routing' tends to imply some decision-making capability which is not the case for the cable.

Looks like it still says 'routing'

theasianpianist · 2021-05-05T21:31:53Z

sonic_y_cable/y_cable_base.py

+        TOR B. This means if the Y cable is actively routing, the "check_active_linked_tor_side"
+        API will now return Tor B. It also implies that if the link is actively routing on this port, Y cable


Same as above

same as above

sonic_y_cable/y_cable_base.py

theasianpianist · 2021-05-05T21:34:18Z

sonic_y_cable/y_cable_base.py

+
+    def check_if_link_is_active_for_NIC(self, port):
+        """
+        This API specifically checks if NIC side of the Y cable's link is active


What is the definition we are using for 'active'? Does that mean the cable is plugged in, link is set to admin up, traffic is flowing through this link, etc.

yes basically link is up means cable is up and configured correctly, show int status should show "oper" up

Is this definition commonly known? It might be worth including this info in the docstring.

Hmm maybe I can elaborate a little. I think "oper" up on Cli show interfaces status might confuse the vendors, but objectively this API it's just their capability to check if all the parameters for transceiver initialization happened correctly, and the link is active. Actually when the lanes come up they send some packets internally on Serdes to check if lanes are functioning properly and a bunch of other things to check if link is healthy. While this generally translates to link is up, they cant tell if the link will be up on SONiC side. So I think we can make this definition more concrete but maybe we should not put show int status output a cross check for them. Thoughts ?

I think as long as the vendors know what this means, it's fine as is. I'll defer to your judgement here.

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

theasianpianist · 2021-05-10T18:32:12Z

sonic_y_cable/y_cable_base.py

+        """
+        self.port = port
+
+    def toggle_mux_to_torA(self):


Suggest renaming to toggle_mux_to_tor_a to make snake case consistent

xinyulin · 2021-06-18T10:29:18Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def enable_loopback_mode(self, target, mode=NEAR_END_LOOPBACK, lane_mask):


Suggested change

def enable_loopback_mode(self, target, mode=NEAR_END_LOOPBACK, lane_mask):

def enable_loopback_mode(self, target, lane_mask, mode=LOOPBACK_MODE_NEAR_END):

Suggest to swap the order of lane_mask and mode to fix 'non-default argument follows default argument' syntax error and correct the variable name with 'LOOPBACK_MODE_NEAR_END'

xinyulin · 2021-06-18T12:52:26Z

sonic_y_cable/y_cable_base.py

+        raise NotImplementedError
+
+    def get_switch_count_tor_a(self, clear_on_read=False):
+        """


Both get_switch_count_tor_a() and get_switch_count_tor_b() are not supported in current MCU firmware. The cable can only return the total auto/manual switch count by reading the upper page 0x5, byte 141 to 148. If this information is useful for diagnostics, I will update the API document and MCU FW to support these two APIs from next firmware release.

sounds good

Originally, we defined a total manual switching count in upper page 0x4, byte 141-144. In order to support these two APIs

get_switch_count_tor_a()

get_switch_count_tor_b()

I'd like to change the definition as below

upper page 0x4, byte 141-144: number of manual switching count initiated from TOR A

upper page 0x4, byte 225-229: number of manual switching count initiated from TOR B

and the get_switch_count_total() will return the sum of TOR A (byte 141-144) and TOR B (byte 225 - 228).

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

xinyulin · 2021-06-22T06:56:54Z

sonic_y_cable/y_cable_base.py

+
+#############################################################################################
+###                                  Debug Functionality                                  ###
+#############################################################################################


There is a general question of the debug functionality. We observed that those diagnostic functions (get_ber_inf(), enable_loopback_mode()...etc.) can not work well when AN/LT enabled. Both sides receiver will jump to wait for AN (auto-negotiation) state after we enabled the debug mode (ex. set prbs generation, set loopback mode...). The AN/LT can no longer be finished because the TX were constantly transmit PRBS data instead of responding the AN signal. User cannot get correct BER info due to the receiver is not ready.

To solve the deadlock between AN/LT and debug function, the alternative solution is to re-configure both sides (SONiC and y cable) to disable AN/LT first, then run the debugging functions (PRBS, BER, Loopback)
For the next new firmware, the y_cable will support a mode configuration register in upper page 0x4, byte 209. User can configure the speed (50G, 100G) and ANLT on/off via this register.
For SONiC OS, is there any simple way to enable/disable the AN/LT without reloading the Config_db?

I dont think there is any other way for now, other than to reload the confg_db.json

xinyulin · 2021-06-22T07:24:26Z

sonic_y_cable/y_cable_base.py

+        raise NotImplementedError
+
+    def create_port(self, speed, fec_mode_tor_a=FEC_MODE_NONE, fec_mode_tor_b=FEC_MODE_NONE,
+                    fec_mode_nic=FEC_MODE_NONE, anlt_tor_a=False, anlt_tor_b= False, anlt_nic=False):


Is it make sense to change the input parameter as suggestion?
For current y_cable firmware implementation, both TOR A and TOR B must be operated in the same mode. it is not allowed to configure AN/LT, FEC on TOR A and B individually.

Suggested change

fec_mode_nic=FEC_MODE_NONE, anlt_tor_a=False, anlt_tor_b= False, anlt_nic=False):

fec_mode_nic=FEC_MODE_NONE, anlt_tor=False, anlt_nic=False):

fixed, can you please check

xinyulin · 2021-06-22T07:24:45Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def create_port(self, speed, fec_mode_tor_a=FEC_MODE_NONE, fec_mode_tor_b=FEC_MODE_NONE,


Suggested change

def create_port(self, speed, fec_mode_tor_a=FEC_MODE_NONE, fec_mode_tor_b=FEC_MODE_NONE,

def create_port(self, speed, fec_mode_tor=FEC_MODE_NONE,

fixed, can you please check

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

sonic_y_cable/y_cable_base.py

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

jleveque · 2021-06-30T19:52:41Z

sonic_y_cable/y_cable_base.py


    def log_warning(self, msg):
-        self.main_logger.log_warning("y_cable_port {} {}".format(self.port, msg))
+        self._logger.log_warning("y_cable_port {} {}".format(self.port, msg))


Suggest to add something like a colon as a delimiter. E.g, y_cable_port {}: {}. Do this for all log functions.

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

UPDATED

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

xinyulin · 2021-07-20T10:20:47Z

sonic_y_cable/y_cable_base.py

+    FEC_MODE_FC = 2
+
+    # definitions of the modes to be run for loopback mode
+    # on the port/cable


Shall we add LOOPBACK_MODE_NONE if the loopback mode is not enabled?

I will update the PR

xinyulin · 2021-07-20T10:23:19Z

sonic_y_cable/y_cable_base.py

+            mode_value:
+                One of the following predefined constants, the mode to be run for loopback:
+                    LOOPBACK_MODE_NEAR_END
+                    LOOPBACK_MODE_FAR_END


Shall we return LOOPBACK_MODE_NONE if this feature is not enabled?

Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin · 2021-07-22T04:43:42Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_pcs_stats(self, target):


Suggested change

def get_pcs_stats(self, target):

def get_pcs_stats(self, target, clear_on_read=False):

Is it necessary to add clear_on_read to reset the pcs statistics?

if the feature cannot be supported, you can return an error when the caller tries to enable clear_on_read= True;
I believe other vendors might be able to support this, hence it is kept as an argument.

xinyulin · 2021-07-22T04:44:17Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_fec_stats(self, target):


Suggested change

def get_fec_stats(self, target):

def get_fec_stats(self, target, clear_on_read=False):

Is it necessary to add clear_on_read to reset the fec statistics?

if the feature cannot be supported, you can return an error when the caller tries to enable clear_on_read= True;
I believe other vendors might be able to support this, hence it is kept as an argument.

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

lguohan

prgeor · 2021-07-26T16:59:55Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_target_cursor_values(self, lane, target):


Instead of cursor, can we rename to get_target_equalization() as that would be more generic? pre-tap, post-tap, pre-emphasis, pre-cursor, post-cursor all fall under equalization settings.

Let me make a note for this, all vendors will also need to be notfied for this. We can put this in a future PR.

prgeor · 2021-07-26T17:00:44Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def set_target_cursor_values(self, lane, cursor_values, target):


Instead of cursor, can we rename to set_target_equalization() as that would be more generic? pre-tap, post-tap, pre-emphasis, pre-cursor, post-cursor all fall under equalization settings.

Let me make a note for this, all vendors will also need to be notfied for this. We can put this in a future PR

prgeor · 2021-07-26T17:11:04Z

sonic_y_cable/y_cable_base.py

+                a boolean, False, Non-hitless upgrade: it will update the firmware regardless
+                                  the current status, a link flip can be observed during the upgrade.
+        Returns:
+            One of the following predefined constants:


If a vendor doesn't support hitless upgrade, then we should have a return value indicating the failure.

Currently all vendors need to support hitless by default. The firmware error codes are going to be in a future PR as they are under discussion by vendors. Currently the recommendation is if there is an error which is not defined, vendor can return a generic error EEPROM_ERROR, which the API is called for.

prgeor · 2021-07-26T17:14:53Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_firmware_version(self, target):


Can Y-cable function with different targets running different firmware versions?

Yes, depends on vendor. Some vendors have a unique version on all MCU's whereas some could have different versions.

prgeor · 2021-07-26T17:34:47Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_eye_heights(self, target):


NRZ signal has single eye, whereas PAM4 signal has 3 eyes. So we should return something like.

{lane#: eye1, eye2, eye3} for PAM4 where eye1 is for upper eye, eye2 is for middle eye and eye3 for lower eye heights respectively.
{lane#: eye} for NRZ signal

PAM4 is used in QSFP-DD, OSFP

Currently all vendors are on NRZ and that's why we have a single eye value for each lane.

prgeor · 2021-07-27T11:21:03Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_switch_count_target(self, switch_count_type, target, clear_on_read=False):


The API will fail for target=TARGET_NIC but the return type don't indicate the failure.

noted for a further enhancement PR

prgeor · 2021-07-27T11:24:46Z

sonic_y_cable/y_cable_base.py

+        raise NotImplementedError
+
+    def get_switching_mode(self):
+        """


What will this api return if set_switching_mode() is not called? or in other words what is the default mode?

by default the mode has to be manual. There can never be a no mode value. it is either auto or manual

prgeor · 2021-07-27T11:35:25Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_alive_status(self):


Will this API return true if all the API is_link_active() returns true for ALL targets?

No link active is different from get_alive_status. get_alive_status just checks whether all sides of the cable are plugged in or not.

prgeor · 2021-07-27T11:43:04Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def get_autoswitch_hysteresis_timer(self):


Will this return 0 if hystersis timer is not configured or return the default value?

this should return the value corresponding to the eeprom offset where the definition is. All vendors could have different hysteresis_timer values but default is 5 s

prgeor · 2021-07-27T12:39:22Z

sonic_y_cable/y_cable_base.py

+
+        raise NotImplementedError
+
+    def reset(self, target):


Please specify if this API affect the ongoing traffic on the link?

if target is NIC this will reset.

…I support for multiple vendors (#186) Description For multiple Y-Cable vendor support once we do have a mapping from vendor/part number to the appropriate Y-Cable module to load, we need to map appropriate port to a module as well. This PR adds definition for base abstract class. Motivation and Context Basically, the key idea is once we have a port identified as being of a certain vendor and it has been identified to load a certain module, it is then needed to call the correct module/API on each port each time we call the API on the port it is required to maintain this mapping in memory since xcvrd does not want to read/parse this y_cable_vendor_mapping.py file again and again each time we call the Y-Cable API Also note that the module loaded might change during xcvrd running lifetime since cable/transceiver can be changed from one vendor to another. So we need to take this into consideration as well Proposed Solution for this Each module of the Y-Cable vendor can be a class (of each transceiver type) and all we need to do is instantiate the objects of these classes as class instances and these objects will provide the interface of calling the API's for the appropriate vendor Y-Cable. This instantiation will be done inside xcvrd, when xcvrd starts These objects basically emulate Y-Cable instances and whatever action/interaction needs to be done with the Y-Cable the methods of these objects would provide that each vendor in their implementation can inherit from a base class where there will be definitions for all the supported capabilities of the Y-Cable. for vendors the recommended approach in case their subclass implementation does not support a method, is to set the method equal to None. This differentiates it from a method they forgot to implement. Then, the calling code should first check if the method is None before attempting to call it. Design document for the support is sonic-net/SONiC#757 Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

[sonic_y_cable] add abstract class YCableBase required for Y-cable AP…

1522102

…I support for multiple vendors Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

vdahiya12 requested review from jleveque, lguohan and theasianpianist May 5, 2021 05:51

vdahiya12 added the Y-Cable label May 5, 2021

jleveque suggested changes May 5, 2021

View reviewed changes

theasianpianist reviewed May 5, 2021

View reviewed changes

sonic_y_cable/y_cable_base.py Outdated Show resolved Hide resolved

theasianpianist reviewed May 5, 2021

View reviewed changes

vdahiya12 added 2 commits May 6, 2021 05:01

fix comments

5bab021

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

fix missing comments

9fe685a

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

jleveque suggested changes May 6, 2021

View reviewed changes

vdahiya12 added 2 commits May 6, 2021 20:47

fix corrections

d1dc947

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

fix suggestion

998f323

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

theasianpianist reviewed May 6, 2021

View reviewed changes