Skip to content

sonic_xcvrd: Support for DOM Threshold values for EEPROM dump #29

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.

Sign up for GitHub

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

Merged
jleveque merged 6 commits into from
Jul 2, 2019
Merged

sonic_xcvrd: Support for DOM Threshold values for EEPROM dump #29

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
68687de
sonic_xcvrd: Support for DOM Threshold values for EEPROM dump
sridhar-ravindran May 29, 2019
3c1e40f
Addressing Review comments for sonic_xcvrd: Support for DOM Threshold
sridhar-ravindran Jun 15, 2019
2ab95eb
Sync to Latest Master
sridhar-ravindran Jun 25, 2019
8cc710b
Addressing Review comments
sridhar-ravindran Jun 25, 2019
9f4f2dc
DeCoupling Dom Info from DomThreshold
sridhar-ravindran Jun 26, 2019
af21da2
Merge branch 'master' into master
sridhar-ravindran Jul 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 123 additions & 14 deletions sonic-xcvrd/scripts/xcvrd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,32 @@ def beautify_dom_info_dict(dom_info_dict):
dom_info_dict['tx3power'] = strip_unit_and_beautify(dom_info_dict['tx3power'], POWER_UNIT)
dom_info_dict['tx4power'] = strip_unit_and_beautify(dom_info_dict['tx4power'], POWER_UNIT)

def beautify_dom_threshold_info_dict(dom_info_dict):
dom_info_dict['temphighalarm'] = strip_unit_and_beautify(dom_info_dict['temphighalarm'], TEMP_UNIT)
dom_info_dict['temphighwarning'] = strip_unit_and_beautify(dom_info_dict['temphighwarning'], TEMP_UNIT)
dom_info_dict['templowalarm'] = strip_unit_and_beautify(dom_info_dict['templowalarm'], TEMP_UNIT)
dom_info_dict['templowwarning'] = strip_unit_and_beautify(dom_info_dict['templowwarning'], TEMP_UNIT)

dom_info_dict['vcchighalarm'] = strip_unit_and_beautify(dom_info_dict['vcchighalarm'], VOLT_UNIT)
dom_info_dict['vcchighwarning'] = strip_unit_and_beautify(dom_info_dict['vcchighwarning'], VOLT_UNIT)
dom_info_dict['vcclowalarm'] = strip_unit_and_beautify(dom_info_dict['vcclowalarm'], VOLT_UNIT)
dom_info_dict['vcclowwarning'] = strip_unit_and_beautify(dom_info_dict['vcclowwarning'], VOLT_UNIT)

dom_info_dict['txpowerhighalarm'] = strip_unit_and_beautify(dom_info_dict['txpowerhighalarm'], POWER_UNIT)
dom_info_dict['txpowerlowalarm'] = strip_unit_and_beautify(dom_info_dict['txpowerlowalarm'], POWER_UNIT)
dom_info_dict['txpowerhighwarning'] = strip_unit_and_beautify(dom_info_dict['txpowerhighwarning'], POWER_UNIT)
dom_info_dict['txpowerlowwarning'] = strip_unit_and_beautify(dom_info_dict['txpowerlowwarning'], POWER_UNIT)

dom_info_dict['rxpowerhighalarm'] = strip_unit_and_beautify(dom_info_dict['rxpowerhighalarm'], POWER_UNIT)
dom_info_dict['rxpowerlowalarm'] = strip_unit_and_beautify(dom_info_dict['rxpowerlowalarm'], POWER_UNIT)
dom_info_dict['rxpowerhighwarning'] = strip_unit_and_beautify(dom_info_dict['rxpowerhighwarning'], POWER_UNIT)
dom_info_dict['rxpowerlowwarning'] = strip_unit_and_beautify(dom_info_dict['rxpowerlowwarning'], POWER_UNIT)

dom_info_dict['txbiashighalarm'] = strip_unit_and_beautify(dom_info_dict['txbiashighalarm'], BIAS_UNIT)
dom_info_dict['txbiaslowalarm'] = strip_unit_and_beautify(dom_info_dict['txbiaslowalarm'], BIAS_UNIT)
dom_info_dict['txbiashighwarning'] = strip_unit_and_beautify(dom_info_dict['txbiashighwarning'], BIAS_UNIT)
dom_info_dict['txbiaslowwarning'] = strip_unit_and_beautify(dom_info_dict['txbiaslowwarning'], BIAS_UNIT)

# Update port sfp info in db
def post_port_sfp_info_to_db(logical_port_name, table, stop=threading.Event()):
ganged_port = False
Expand Down Expand Up @@ -156,6 +182,66 @@ def post_port_sfp_info_to_db(logical_port_name, table, stop=threading.Event()):
logger.log_error("This functionality is currently not implemented for this platform")
sys.exit(NOT_IMPLEMENTED_ERROR)

# Update port dom threshold info in db
def post_port_dom_threshold_info_to_db(logical_port_name, table,
stop=threading.Event()):
ganged_port = False
ganged_member_num = 1

physical_port_list = logical_port_name_to_physical_port_list(logical_port_name)
if physical_port_list is None:
logger.log_error("No physical ports found for logical port '%s'"
% logical_port_name)
return PHYSICAL_PORT_NOT_EXIST

if len(physical_port_list) > 1:
ganged_port = True

for physical_port in physical_port_list:
if stop.is_set():
break

if not platform_sfputil.get_presence(physical_port):
continue

port_name = get_physical_port_name(logical_port_name,
ganged_member_num, ganged_port)
ganged_member_num += 1

try:
dom_info_dict = platform_sfputil.get_transceiver_dom_info_dict(physical_port)
if dom_info_dict is not None:
beautify_dom_threshold_info_dict(dom_info_dict)
fvs = swsscommon.FieldValuePairs(
[('temphighalarm', dom_info_dict['temphighalarm']),
('temphighwarning', dom_info_dict['temphighwarning']),
('templowalarm', dom_info_dict['templowalarm']),
('templowwarning', dom_info_dict['templowwarning']),
('vcchighalarm', dom_info_dict['vcchighalarm']),
('vcchighwarning', dom_info_dict['vcchighwarning']),
('vcclowalarm', dom_info_dict['vcclowalarm']),
('vcclowwarning', dom_info_dict['vcclowwarning']),
('txpowerhighalarm', dom_info_dict['txpowerhighalarm']),
('txpowerlowalarm', dom_info_dict['txpowerlowalarm']),
('txpowerhighwarning', dom_info_dict['txpowerhighwarning']),
('txpowerlowwarning', dom_info_dict['txpowerlowwarning']),
('rxpowerhighalarm', dom_info_dict['rxpowerhighalarm']),
('rxpowerlowalarm', dom_info_dict['rxpowerlowalarm']),
('rxpowerhighwarning', dom_info_dict['rxpowerhighwarning']),
('rxpowerlowwarning', dom_info_dict['rxpowerlowwarning']),
('txbiashighalarm', dom_info_dict['txbiashighalarm']),
('txbiaslowalarm', dom_info_dict['txbiaslowalarm']),
('txbiashighwarning', dom_info_dict['txbiashighwarning']),
('txbiaslowwarning', dom_info_dict['txbiaslowwarning'])
])
table.set(port_name, fvs)
else:
return SFP_EEPROM_NOT_READY

except NotImplementedError:
logger.log_error("This functionality is currently not implemented for this platform")
sys.exit(NOT_IMPLEMENTED_ERROR)

# Update port dom sensor info in db
def post_port_dom_info_to_db(logical_port_name, table, stop=threading.Event()):
ganged_port = False
Expand Down Expand Up @@ -183,21 +269,42 @@ def post_port_dom_info_to_db(logical_port_name, table, stop=threading.Event()):
dom_info_dict = platform_sfputil.get_transceiver_dom_info_dict(physical_port)
if dom_info_dict is not None:
beautify_dom_info_dict(dom_info_dict)
fvs = swsscommon.FieldValuePairs([('temperature', dom_info_dict['temperature']),
('voltage', dom_info_dict['voltage']),
('rx1power', dom_info_dict['rx1power']),
('rx2power', dom_info_dict['rx2power']),
('rx3power', dom_info_dict['rx3power']),
('rx4power', dom_info_dict['rx4power']),
('tx1bias', dom_info_dict['tx1bias']),
('tx2bias', dom_info_dict['tx2bias']),
('tx3bias', dom_info_dict['tx3bias']),
('tx4bias', dom_info_dict['tx4bias']),
('tx1power', dom_info_dict['tx1power']),
('tx2power', dom_info_dict['tx2power']),
('tx3power', dom_info_dict['tx3power']),
('tx4power', dom_info_dict['tx4power'])])
beautify_dom_threshold_info_dict(dom_info_dict)
fvs = swsscommon.FieldValuePairs(
[('temperature', dom_info_dict['temperature']),
('voltage', dom_info_dict['voltage']),
('rx1power', dom_info_dict['rx1power']),
('rx2power', dom_info_dict['rx2power']),
('rx3power', dom_info_dict['rx3power']),
('rx4power', dom_info_dict['rx4power']),
('tx1bias', dom_info_dict['tx1bias']),
('tx2bias', dom_info_dict['tx2bias']),
('tx3bias', dom_info_dict['tx3bias']),
('tx4bias', dom_info_dict['tx4bias']),
('tx1power', dom_info_dict['tx1power']),
('tx2power', dom_info_dict['tx2power']),
('tx3power', dom_info_dict['tx3power']),
('tx4power', dom_info_dict['tx4power']),
('temphighalarm', dom_info_dict['temphighalarm']),
Copy link
Copy Markdown
Collaborator

@keboliu keboliu Jun 4, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@sridhar-ravindran sridhar-ravindran Jun 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@keboliu keboliu Jun 19, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@keboliu keboliu Jun 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@sridhar-ravindran sridhar-ravindran Jun 26, 2019

Choose a reason for hiding this comment

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

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.

('temphighwarning', dom_info_dict['temphighwarning']),
('templowalarm', dom_info_dict['templowalarm']),
('templowwarning', dom_info_dict['templowwarning']),
('vcchighalarm', dom_info_dict['vcchighalarm']),
('vcchighwarning', dom_info_dict['vcchighwarning']),
('vcclowalarm', dom_info_dict['vcclowalarm']),
('vcclowwarning', dom_info_dict['vcclowwarning']),
('rxpowerhighalarm', dom_info_dict['rxpowerhighalarm']),
('rxpowerlowalarm', dom_info_dict['rxpowerlowalarm']),
('rxpowerhighwarning', dom_info_dict['rxpowerhighwarning']),
('rxpowerlowwarning', dom_info_dict['rxpowerlowwarning']),
('txbiashighalarm', dom_info_dict['txbiashighalarm']),
('txbiaslowalarm', dom_info_dict['txbiaslowalarm']),
('txbiashighwarning', dom_info_dict['txbiashighwarning']),
('txbiaslowwarning', dom_info_dict['txbiaslowwarning'])
])

table.set(port_name, fvs)

else:
return SFP_EEPROM_NOT_READY

Expand All @@ -220,6 +327,7 @@ def post_port_sfp_dom_info_to_db(stop=threading.Event()):

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)
Copy link
Copy Markdown
Collaborator

@keboliu keboliu Jun 19, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Collaborator

@keboliu keboliu Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Contributor Author

@sridhar-ravindran sridhar-ravindran Jun 26, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@keboliu keboliu Jun 27, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@sridhar-ravindran sridhar-ravindran Jun 27, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@jleveque jleveque Jul 1, 2019

Choose a reason for hiding this comment

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

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.


# Delete port dom/sfp info from db
def del_port_sfp_dom_info_from_db(logical_port_name, int_tbl, dom_tbl):
Expand Down Expand Up @@ -312,6 +420,7 @@ class sfp_state_update_task:
time.sleep(TIME_FOR_SFP_READY_SECS)
post_port_sfp_info_to_db(logical_port, int_tbl)
post_port_dom_info_to_db(logical_port, dom_tbl)
post_port_dom_threshold_info_to_db(logical_port, dom_tbl)
elif value == SFP_STATUS_REMOVED:
logger.log_info("Got SFP removed event")
del_port_sfp_dom_info_from_db(logical_port, int_tbl, dom_tbl)
Expand Down