sonic_xcvrd: Support for DOM Threshold values for EEPROM dump by sridhar-ravindran · Pull Request #29 · sonic-net/sonic-platform-daemons

sridhar-ravindran · 2019-05-29T18:12:53Z

This change is added to dump ChannelMonitor Threshold and Module Monitor Threshold values as part of DOM EEPROM dump output.

Added code change in sff8436.py,sfputilshow.py, xcvrd, sfpshow .. etc

Verified the output in following optics

a) AVAGO
b) Finisar
c) DELL

Seeing the following issue of Values not getting dumped properly from day 1

Some channel monitor values are not displayed correctly.
Will raise a seperate issue for that.

Ethernet124: SFP EEPROM detected
Connector: MPOx12
Encoding: 64B66B
Extended Identifier: Power Class 1(1.5W max)
Extended RateSelect Compliance: QSFP+ Rate Select Version 1
Identifier: QSFP+ or later
Length OM3(2m): 50
Nominal Bit Rate(100Mbs): 103
Specification compliance:
10/40G Ethernet Compliance Code: 40GBASE-SR4
Vendor Date Code(YYYY-MM-DD Lot): 2010-09-01
Vendor Name: AVAGO
Vendor OUI: 00-17-6a
Vendor PN: AFBR-79E4Z-D
Vendor Rev: 01
Vendor SN: QA340092
ChannelMonitorValues:
RX1Power: -16.3827dBm
RX2Power: -infdBm <<<<<<<<<<<<<<<<<<<<<<
RX3Power: -15.5284dBm
RX4Power: -infdBm <<<<<<<<<<<<<<<<<<<<<<<
TX1Bias: 5.9720mA
TX2Bias: 5.3740mA
TX3Bias: 5.3800mA
TX4Bias: 6.0540mA

sridhar-ravindran · 2019-05-29T18:23:17Z

Depends on
sonic-net/sonic-utilities#545
sonic-net/sonic-platform-common#33

sridhar-ravindran · 2019-05-29T18:25:51Z

z9100_50G_breakout_profile.log
Finisar_dell_avago_Eeprom_dom_dump.log

Z9100_UnitTesting.txt
S6100_UnitTesting.txt

keboliu · 2019-06-04T01:26:01Z

sonic-xcvrd/scripts/xcvrd

+                         ('tx2power', dom_info_dict['tx2power']),
+                         ('tx3power', dom_info_dict['tx3power']),
+                         ('tx4power', dom_info_dict['tx4power']),
+                         ('temphighalarm', dom_info_dict['temphighalarm']),


I would suggest separating below to entries into two parts, for the constant entries, we only fetch them one time at the starting of the xcvrd as port_info, for the variable ones fetch them periodically with other DOM entries.

Hi,
Implemented a new function post_port_dom_threshold_info_to_db. This function will be called only during init, New Optic Detection. Dom Info will be called periodically every 1 minute via dom_info_update_task . We are not calling the dom_threshold update here

Thanks
Sridhar.R

is it still reading the threshold value every time when updating the dom info? only not updated to DB? if yes I would like to suggest not to do it because reading EEPROM is time-consuming, don't want to introduce unnecessarily load.

is it still reading the threshold value every time when updating the dom info? only not updated to DB? if yes I would like to suggest not to do it because reading EEPROM is time-consuming, don't want to introduce unnecessarily load.

I think this comment is still valid, I suggest to decouple the threshold reading and voltage/power/temp/bias reading, please refer to my comments in another PR.

Hi @keboliu,

Implemented a new function get_transceiver_dom_threshold_info_dict. This will read only dom threshold.
Removed reading threshold values in existing get_transceiver_dom_info_dict.

Hope this time i have not mis understood your comments.

jleveque · 2019-06-18T22:57:28Z

@sridhar-ravindran: Please resolve conflicts.

@keboliu: Please review new changes.

sonic-xcvrd/scripts/xcvrd

keboliu · 2019-06-19T02:16:00Z

sonic-xcvrd/scripts/xcvrd


        post_port_sfp_info_to_db(logical_port_name, int_tbl, stop)
        post_port_dom_info_to_db(logical_port_name, dom_tbl, stop)
+        post_port_dom_threshold_info_to_db(logical_port_name, dom_tbl, stop)


not quite sure here to use dom_tbl is a good idae, maybe should use int_tbl since the threshold is static if my understanding is correct. @jleveque any comments?

@jleveque @sridhar-ravindran I have the concern to mix the constant threshold values and the variables in one table. would you please share your view?

Hi @keboliu @jleveque
Since the threshold part comes under the dom category, i updated the dom_tbl.

@jleveque I would like to get your view on this.

Hi @keboliu, @jleveque

In Show command,
a) For dumping SFP info eeprom contents we use int_tbl(TRANSCEIVER_INFO)
b) For dumping DOM info eeprom contents we use dom_tbl (TRANSCEIVER_DOM_SENSOR)

My(@sridhar-ravindran) approach is since the thresholds that we are displaying corresponds to DOM data, i save the threshold information in dom_tbl. So dom thresholds will be displaying along with other dom data (sfpshow eeprom --dom)
Note:As @keboliu suggested, we will read the threshold value only once (since it is static)
We will not read threshold value periodically like dom_data.

@keboliu suggests, since the dom threshold data is static, we can save the threshold in int_tbl(TRANSCEIVER_INFO).

a) Then we will be displaying the dom_threshold data along with SFP info (sfpshow eeprom)
(or)
b) We have to read both int_tbl and dom_tbl to display dom_threshold along with other dom_data (sfpshow eeprom --dom)

We need your inputs which approach to use, save dom_threshold data in dom_tbl or int_tbl?

I understand both points here. The thresholds are related to DOM data, so it makes more sense to store in dom_tbl. As long as there isn't an efficiency issue here (i.e., we don't needlessly update the threshold values along with the DOM data, then I'm fine with either approach. From @sridhar-ravindran's most recent reply,

@keboliu suggests, since the dom threshold data is static, we can save the threshold in int_tbl(TRANSCEIVER_INFO).

a) Then we will be displaying the dom_threshold data along with SFP info (sfpshow eeprom)
(or)
b) We have to read both int_tbl and dom_tbl to display dom_threshold along with other dom_data (sfpshow eeprom --dom)

I think it makes more sense to store the threshold values in dom_tbl. That way sfpshow eeprom doesn't display threshold data and also we won't have to read from both int_tbl and dom_tbl to display dom_threshold along with other dom_data.

sridhar-ravindran · 2019-06-25T16:16:28Z

Hi @keboliu
I have rebased to latest master and addressed all the review comments. Please take a look

Thanks
Sridhar.R

sridhar-ravindran · 2019-06-29T03:35:01Z

Hi @keboliu @jleveque
Could you please review and approve this . This is also needed for DOM Threshold values.
Thanks
Sridhar.R

keboliu · 2019-07-01T02:01:16Z

Hi @keboliu @jleveque
Could you please review and approve this . This is also needed for DOM Threshold values.
Thanks
Sridhar.R

I unblocked this PR, however, I hope someone else can give comments especially for #29 (comment)

keboliu

need someone else to review.

sridhar-ravindran · 2019-07-02T01:43:38Z

Hi,
Addressed the merge issue.

[sonic-platform-common] [sonic_sfp] Interpret sff 'int' element =0 as valid value (sonic-net/sonic-platform-common#51) add more error code to get_transceiver_change_event ((sonic-net/sonic-platform-common#50) [sonic_platform_base] support new-platform-api-based daemons ((sonic-net/sonic-platform-common#48) sync change to sonic_platform_base/sonic_sfp and create symbol link ((sonic-net/sonic-platform-common#49) Add parser support for Tx_RxLos,TxFault, PowerControl, ResetStatus in sff8436.py ((sonic-net/sonic-platform-common#45) readd type_abbrv_name in sonic_sfp/sff8436.py ((sonic-net/sonic-platform-common#44) [psu_base] get_status_led() returns current state of the status LED ((sonic-net/sonic-platform-common#39) Fix abbrv name for OSFP ((sonic-net/sonic-platform-common#36) [sff8436] support "Control Bytes" and "Options" ((sonic-net/sonic-platform-common#38) sonic_sfp: avoid possible key error in get_physical_to_logical() ((sonic-net/sonic-platform-common#37) [sonic-platform-daemons] [xcvrd] Enhance xcvrd to handle new system level event/error (sonic-net/sonic-platform-daemons#39) [xcvrd] Support both new platform API and old platform plugins (sonic-net/sonic-platform-daemons#38) [psud] Support both new platform API and old platform plugins (sonic-net/sonic-platform-daemons#37) [syseepromd] Support both new platform API and old platform plugins (sonic-net/sonic-platform-daemons#36) Add missing import statemet (sonic-net/sonic-platform-daemons#32) sonic_xcvrd: Support for DOM Threshold values for EEPROM dump (sonic-net/sonic-platform-daemons#29)

…ic-net#3333) [sonic-platform-common] [sonic_sfp] Interpret sff 'int' element =0 as valid value (sonic-net/sonic-platform-common#51) add more error code to get_transceiver_change_event ((sonic-net/sonic-platform-common#50) [sonic_platform_base] support new-platform-api-based daemons ((sonic-net/sonic-platform-common#48) sync change to sonic_platform_base/sonic_sfp and create symbol link ((sonic-net/sonic-platform-common#49) Add parser support for Tx_RxLos,TxFault, PowerControl, ResetStatus in sff8436.py ((sonic-net/sonic-platform-common#45) readd type_abbrv_name in sonic_sfp/sff8436.py ((sonic-net/sonic-platform-common#44) [psu_base] get_status_led() returns current state of the status LED ((sonic-net/sonic-platform-common#39) Fix abbrv name for OSFP ((sonic-net/sonic-platform-common#36) [sff8436] support "Control Bytes" and "Options" ((sonic-net/sonic-platform-common#38) sonic_sfp: avoid possible key error in get_physical_to_logical() ((sonic-net/sonic-platform-common#37) [sonic-platform-daemons] [xcvrd] Enhance xcvrd to handle new system level event/error (sonic-net/sonic-platform-daemons#39) [xcvrd] Support both new platform API and old platform plugins (sonic-net/sonic-platform-daemons#38) [psud] Support both new platform API and old platform plugins (sonic-net/sonic-platform-daemons#37) [syseepromd] Support both new platform API and old platform plugins (sonic-net/sonic-platform-daemons#36) Add missing import statemet (sonic-net/sonic-platform-daemons#32) sonic_xcvrd: Support for DOM Threshold values for EEPROM dump (sonic-net/sonic-platform-daemons#29)

sonic_xcvrd: Support for DOM Threshold values for EEPROM dump

68687de

This was referenced May 29, 2019

sonic_utilities: Support for DOM Threshold values for EEPROM dump sonic-net/sonic-utilities#545

Merged

sonic_sfp: Support for DOM Threshold values for EEPROM dump sonic-net/sonic-platform-common#33

Merged

keboliu requested changes Jun 4, 2019

View reviewed changes

Addressing Review comments for sonic_xcvrd: Support for DOM Threshold

3c1e40f

keboliu requested changes Jun 19, 2019

View reviewed changes

sridhar-ravindran added 2 commits June 25, 2019 08:09

Sync to Latest Master

2ab95eb

Addressing Review comments

8cc710b

DeCoupling Dom Info from DomThreshold

9f4f2dc

keboliu reviewed Jul 1, 2019

View reviewed changes

keboliu approved these changes Jul 1, 2019

View reviewed changes

jleveque approved these changes Jul 1, 2019

View reviewed changes

Merge branch 'master' into master

af21da2

jleveque merged commit daa3fd9 into sonic-net:master Jul 2, 2019

keboliu mentioned this pull request Aug 14, 2019

[sonic-platform-common][sonic-platform-daemon] Update submodules sonic-net/sonic-buildimage#3333

Merged

jleveque added Enhancement xcvrd labels Jul 10, 2020

Conversation

sridhar-ravindran commented May 29, 2019

Uh oh!

sridhar-ravindran commented May 29, 2019

Uh oh!

sridhar-ravindran commented May 29, 2019

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!

jleveque commented Jun 18, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keboliu Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sridhar-ravindran commented Jun 25, 2019

Uh oh!

sridhar-ravindran commented Jun 29, 2019

Uh oh!

keboliu commented Jul 1, 2019

Uh oh!

keboliu left a comment

Choose a reason for hiding this comment

Uh oh!

sridhar-ravindran commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

keboliu Jun 26, 2019 •

edited

Loading