-
Notifications
You must be signed in to change notification settings - Fork 480
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
Add SAI_SWITCH_ATTR_CURRENT_TEMP to get average temperature from sensors #880
Add SAI_SWITCH_ATTR_CURRENT_TEMP to get average temperature from sensors #880
Conversation
inc/saiswitch.h
Outdated
@@ -409,6 +409,17 @@ typedef enum _sai_switch_attr_t | |||
*/ | |||
SAI_SWITCH_ATTR_MAX_TEMP, | |||
|
|||
/** | |||
* @brief The average value of the temperature | |||
* retrieved from the switch sensors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean from all available sensors? not sure if description is good enough, if you have 1 sensor with 10 degree, and 2nd with 100 then average is 55 which maybe ok but system is defenetly not ok at 100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, assuming ASIC has N internal sensors, we wanted the average of the N sensors. I can update the description. This attribute is meant to be interpreted in conjunction with the MAX_TEMP to check for deviation / discard outliers (so there are no false alarms). Alternatively, we can get the list of N values (instead of just MAX_TEMP & CURRENT_TEMP) and let the application interpret ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating description should be fine
Yes, assuming ASIC has N internal sensors, we wanted the average of the N sensors. I can update the description. This attribute is meant to be interpreted in conjunction with the MAX_TEMP to check for deviation / discard outliers (so there are no false alarms). Alternatively, we can get the list of N values (instead of just MAX_TEMP & CURRENT_TEMP) and let the application interpret ?
From: Kamil Cudnik <[email protected]>
Sent: Sunday, October 21, 2018 1:14 PM
To: opencomputeproject/SAI
Cc: N, Padmanabhan; Author
Subject: Re: [opencomputeproject/SAI] Add SAI_SWITCH_ATTR_CURRENT_TEMP to get average temperature from sensors (#880)
[EXTERNAL EMAIL]
Please report any suspicious attachments, links, or requests for sensitive information.
@kcudnik commented on this pull request.
________________________________
In inc/saiswitch.h<#880 (comment)>:
@@ -409,6 +409,17 @@ typedef enum _sai_switch_attr_t
*/
SAI_SWITCH_ATTR_MAX_TEMP,
+ /**
+ * @brief The average value of the temperature
+ * retrieved from the switch sensors
does this mean from all available sensors? not sure if description is good enough, if you have 1 sensor with 10 degree, and 2nd with 100 then average is 55 which maybe ok but system is defenetly not ok at 100
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#880 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AcaNoAYMg85GNtkcqoY4OPKfDYAVGK-Mks5unNVtgaJpZM4XyFKU>.
|
As suggested in the review meeting:
|
**What I did** 1) ASICs have multiple internal thermal sensors. 2) With the help of configuration, a poller is introduced in Orchagent that will periodically retrieve values of these sensors with the help of SAI APIs (opencomputeproject/SAI#880). 3) These retrieved values are being populated to the state DB (In "ASIC_TEMPERATURE_INFO" table). **Why I did it** As part of ASIC Thermal Monitoring HLD.
Retrieve the average of sensors value readings from the ASIC.