sfp-refactor 400zr integration by qinchuanares · Pull Request #216 · sonic-net/sonic-platform-common

qinchuanares · 2021-08-30T17:53:33Z

This is a sfp-refactor follow-up work: to add 400ZR driver and high level functions into sfp-refactor. To query the keys in state_DB and write keys in config_DB into the module.

Description

The PR aims to provide 400ZR + SONiC support.

Motivation and Context

To be updated

How Has This Been Tested?

To be updated

Additional Information (Optional)

lgtm-com · 2021-08-30T18:01:15Z

This pull request introduces 3 alerts when merging bbabcc4 into a09f5a3 - view on LGTM.com

new alerts:

3 for Variable defined multiple times

prgeor

The CMIS API would stay for long time, would you please add brief description for each API, about the params and the return value/data. Please also add test cases for QSFP-DD modules. Please address my other comments.

sonic_platform_base/sonic_xcvr/mem_maps/public/cmis.py

sonic_platform_base/sonic_xcvr/api/public/cmis.py

…cmis.py file for ZR application only; 3. separate two dictionaries - sff8024 and cmis_code;

…s from c_cmis.py to cmis.py

prgeor · 2021-10-13T06:51:51Z

@qinchuanares could you fix LGTM warnings and also resolve the merge conflict?

lgtm-com · 2021-10-13T17:33:56Z

This pull request introduces 27 alerts when merging e3cc76a into 2ebd786 - view on LGTM.com

new alerts:

16 for Variable defined multiple times
7 for Except block handles 'BaseException'
2 for Unused local variable
2 for Unused import

…. 3. add function module_firmware_switch to switch between A/B image

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py

sonic_platform_base/sonic_xcvr/api/public/cmis.py

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py

sonic_platform_base/sonic_xcvr/codes/public/cmis_code.py

sonic_platform_base/sonic_xcvr/xcvr_api_factory.py

prgeor

@qinchuanares could you address review comments.

sonic_platform_base/sonic_xcvr/xcvr_eeprom.py

prgeor · 2021-10-07T10:03:10Z

sonic_platform_base/sonic_xcvr/xcvr_api_factory.py

@@ -22,7 +23,7 @@ def _get_id(self):

    def create_xcvr_api(self):
        # TODO: load correct classes from id_mapping file


can you rebase your code with latest master?

sonic_platform_base/sonic_xcvr/xcvr_eeprom.py

prgeor · 2021-10-07T13:41:10Z

sonic_platform_base/sonic_xcvr/fields/xcvr_field.py

+            return bytearray(struct.pack(self.format, int(val * self.scale)))
+        return bytearray(struct.pack(self.format, int(val)))
+
+class ListRegField(RegField):


I do see this field used in CMIS memmap for TRANS_CDB, CDB_LPL, CDB_CMD, CDB_WRITE_MSG, but I don't see you are using it for decode/encode. I see for CDB, you are using write_flexible(). If not using remove this?

@qinchuanares still not resolved. if resolved point me to where ListRegField is used. if not used delete ListRegField

sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py

sonic_platform_base/sonic_xcvr/api/public/cmis.py

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py

prgeor · 2021-10-19T12:06:05Z

This is a sfp-refactor follow-up work: to add 400ZR driver and high level functions into sfp-refactor. To query the keys in state_DB and write keys in config_DB into the module.

Description

The PR aims to provide 400ZR + SONiC support.

Motivation and Context

To be updated

How Has This Been Tested?

To be updated

Additional Information (Optional)

@qinchuanares could you update the above sections of your PR? Also please add link to your HLD in the PR description.

sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py

aravindmani-1 · 2021-10-19T12:31:55Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        rx_power_high_warn = self.xcvr_eeprom.read(consts.RX_POWER_HIGH_WARN)
+        rx_power_low_warn = self.xcvr_eeprom.read(consts.RX_POWER_LOW_WARN)
+        rx_power_dict = {'monitor value lane1': rx_power,
+                        'high alarm': rx_power_high_alarm,


can you rename the dict keys similar to the ones defined(get_rx_power() ) in sff8436?.

aravindmani-1 · 2021-10-19T12:33:27Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        voltage_low_alarm = self.xcvr_eeprom.read(consts.VOLTAGE_LOW_ALARM)
+        voltage_high_warn = self.xcvr_eeprom.read(consts.VOLTAGE_HIGH_WARN)
+        voltage_low_warn = self.xcvr_eeprom.read(consts.VOLTAGE_LOW_WARN)
+        voltage_dict = {'monitor value': voltage,


can you rename the dict keys similar to the ones defined in sff8436?.

aravindmani-1 · 2021-10-19T12:33:46Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        tx_power_low_alarm = self.xcvr_eeprom.read(consts.TX_POWER_LOW_ALARM)
+        tx_power_high_warn = self.xcvr_eeprom.read(consts.TX_POWER_HIGH_WARN)
+        tx_power_low_warn = self.xcvr_eeprom.read(consts.TX_POWER_LOW_WARN)
+        tx_power_dict = {'monitor value lane1': tx_power,


can you rename the dict keys similar to the ones defined in sff8436?.

aravindmani-1 · 2021-10-19T12:33:52Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        tx_power_low_alarm = self.xcvr_eeprom.read(consts.TX_POWER_LOW_ALARM)
+        tx_power_high_warn = self.xcvr_eeprom.read(consts.TX_POWER_HIGH_WARN)
+        tx_power_low_warn = self.xcvr_eeprom.read(consts.TX_POWER_LOW_WARN)
+        tx_power_dict = {'monitor value lane1': tx_power,


can you rename the dict keys similar to the ones defined in sff8436?.

aravindmani-1 · 2021-10-19T12:34:30Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        tx_bias_current_low_alarm = self.xcvr_eeprom.read(consts.TX_BIAS_CURR_LOW_ALARM)
+        tx_bias_current_high_warn = self.xcvr_eeprom.read(consts.TX_BIAS_CURR_HIGH_WARN)
+        tx_bias_current_low_warn = self.xcvr_eeprom.read(consts.TX_BIAS_CURR_LOW_WARN)
+        tx_bias_current_dict = {'monitor value lane1': tx_bias_current,


can you rename the dict keys similar to the ones defined in sff8436?.

aravindmani-1 · 2021-10-19T12:35:09Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        case_temp_low_alarm = self.xcvr_eeprom.read(consts.CASE_TEMP_LOW_ALARM)
+        case_temp_high_warn = self.xcvr_eeprom.read(consts.CASE_TEMP_HIGH_WARN)
+        case_temp_low_warn = self.xcvr_eeprom.read(consts.CASE_TEMP_LOW_WARN)
+        case_temp_dict = {'monitor value': case_temp,


can you rename the dict keys similar to the ones defined in sff8436?.

aravindmani-1 · 2021-10-19T12:35:37Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+            tx_power_high_warn_dict['TX_lane%d' %(bitpos+1)] = bool((tx_power_high_warn >> bitpos) & 0x1)
+            tx_power_low_warn_dict['TX_lane%d' %(bitpos+1)] = bool((tx_power_low_warn >> bitpos) & 0x1)
+
+        tx_power_flag_dict = {'tx_power_high_alarm': tx_power_high_alarm_dict,


can you rename the dict keys similar to the ones defined in sff8436?.

aravindmani-1 · 2021-10-19T12:35:46Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+            rx_power_high_warn_dict['RX_lane%d' %(bitpos+1)] = bool((rx_power_high_warn >> bitpos) & 0x1)
+            rx_power_low_warn_dict['RX_lane%d' %(bitpos+1)] = bool((rx_power_low_warn >> bitpos) & 0x1)
+
+        rx_power_flag_dict = {'rx_power_high_alarm': rx_power_high_alarm_dict,


can you rename the dict keys similar to the ones defined in sff8436?.

please check whether the dict keys defined are similar to existing sff8436/sff8636?

qinchuanares · 2021-10-22T15:44:23Z

@qinchuanares could you fix LGTM warnings and also resolve the merge conflict?

Resolved.

…m cmis; 3. split cmis and ccmis api when creating xcvr_api in xcvr_api_factory; 4. remove print and add logger to collect info for syslog

prgeor

can you please address review comments? please mark the comment as 'resolved' if done so.

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py

sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py

sonic_platform_base/sonic_xcvr/fields/xcvr_field.py

prgeor · 2021-10-25T11:27:05Z

sonic_platform_base/sonic_xcvr/fields/xcvr_field.py

+            return bytearray(struct.pack(self.format, int(val * self.scale)))
+        return bytearray(struct.pack(self.format, int(val)))
+
+class ListRegField(RegField):


@qinchuanares still not resolved. if resolved point me to where ListRegField is used. if not used delete ListRegField

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py

sonic_platform_base/sonic_xcvr/api/public/cmis.py

…only relevant functions. 2. resolved other comments

prgeor · 2021-11-08T08:43:10Z

@qinchuanares closing this PR since all changes moved to PR228

…; fix the get_firmware_version API to sync with download_firmware (sonic-net#216) Signed-off-by: vaibhav-dahiya [email protected] Description This PR refactors the listening of port probes from linkmgr to xcvrd as so that xcvrd does not see timeout messages like these below Sep 27 22:24:37.490921 SONIC WARNING mux#linkmgrd: link_manager/LinkManagerStateMachine.cpp:815 handleMuxWaitTimeout: Ethernet120: xcvrd timed out responding to linkmgrd, current state: (P: Unknown, M: Wait, L: Down) Refactoring to a separate listener thread mode as well as removing the lock(safe because read/write transaction via eeprom for port probe) seems to handle this, and does not have the logs like this one above. as the read write eeprom also has a mutex lock in the platform api call, optoe implementation. https://github.com/Azure/sonic-linux-kernel/blob/889d76e36b3f012d3782a1c5e1587c32e4d1ed11/patch/driver-support-optoe.patch#L717 Motivation and Context refactor xcvrd for listening to port probes, disable calling the get_firmware_api() while a download is in progress. Signed-off-by: vaibhav-dahiya <[email protected]>

400zr integration, add to state_db

bbabcc4

add to config_db

4cd63f4

prgeor requested changes Sep 8, 2021

View reviewed changes

qinchuanares added 6 commits September 22, 2021 23:26

Address PR comments: 1. add descriptions to api functions; 2. open c_…

9165e09

…cmis.py file for ZR application only; 3. separate two dictionaries - sff8024 and cmis_code;

Address PR comments

405688b

add CDB and VDM features

43432e7

add CDB and CDM features, add two files

4939daa

add unit test for functions in CCmisApi

75e192b

1. Add VDM flags; 2. Under api>public, transfer cmis related function…

d187682

…s from c_cmis.py to cmis.py

qinchuanares added 2 commits October 13, 2021 09:21

include functions interfacing with xcvrd in CmisApi

044f060

resolve merge conflict

e3cc76a

qinchuanares added 3 commits October 13, 2021 13:30

1. resolve LGTM warnings. 2. add descriptions in FW related functions…

a92e153

…. 3. add function module_firmware_switch to switch between A/B image

add unit test cases for sonic_xcvr/api/public/cmis.py

ccc320a

add unit test cases for cmisCDB and cmisVDM

f65fbfe

qinchuanares requested a review from prgeor October 14, 2021 17:38

add code coverage in unit test

049e602

andywongarista reviewed Oct 18, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py Outdated Show resolved Hide resolved

andywongarista reviewed Oct 18, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/api/public/cmis.py Outdated Show resolved Hide resolved

andywongarista reviewed Oct 18, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/api/public/cmis.py Outdated Show resolved Hide resolved

andywongarista reviewed Oct 18, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/api/public/c_cmis.py Outdated Show resolved Hide resolved

andywongarista reviewed Oct 18, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/codes/public/cmis_code.py Outdated Show resolved Hide resolved

andywongarista reviewed Oct 18, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/xcvr_api_factory.py Outdated Show resolved Hide resolved

prgeor self-assigned this Oct 19, 2021

prgeor requested changes Oct 19, 2021

View reviewed changes

aravindmani-1 reviewed Oct 19, 2021

View reviewed changes

sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py Outdated Show resolved Hide resolved

aravindmani-1 reviewed Oct 19, 2021

View reviewed changes

qinchuanares closed this Oct 22, 2021

prgeor reopened this Oct 22, 2021

1. move all ZR specific functions to ccmis; 2. ccmis now inherits fro…

b673460

…m cmis; 3. split cmis and ccmis api when creating xcvr_api in xcvr_api_factory; 4. remove print and add logger to collect info for syslog

prgeor requested changes Oct 25, 2021

View reviewed changes

qinchuanares added 2 commits October 27, 2021 14:16

1. split get_transceiver_xxx functions to cmis.py and c_cmis.py with …

44c2412

…only relevant functions. 2. resolved other comments

resolving merge conflicts with master

35093d5

ds952811 mentioned this pull request Nov 8, 2021

[sfp-refactoring] Initial support for CMIS application initialization sonic-net/SONiC#876

Merged

5 tasks

prgeor closed this Nov 8, 2021

		@@ -22,7 +23,7 @@ def _get_id(self):

		def create_xcvr_api(self):
		# TODO: load correct classes from id_mapping file

Conversation

qinchuanares commented Aug 30, 2021

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

lgtm-com bot commented Aug 30, 2021

Uh oh!

prgeor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prgeor commented Oct 13, 2021

Uh oh!

lgtm-com bot commented Oct 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prgeor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prgeor commented Oct 19, 2021

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinchuanares commented Oct 22, 2021

Uh oh!

prgeor left a comment

Choose a reason for hiding this comment